Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

llava : fix memory leaks in minicpmv #9879

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tc-mb
Copy link
Contributor

@tc-mb tc-mb commented Oct 14, 2024

I found that the code I previously implemented might have memory leaks. It would be difficult to detect this problem if I just executed the code once, but this bug was discovered by supporting the sever mode.
This pr is used to fix all known memory leak issues. After relatively long sever mode testing, it was determined that minicpmv had no other memory leak problems.

Copy link
Collaborator

@ngxson ngxson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also note that I'm refactoring all this parts in #9687 to be benefit of cpp automatic (de)allocation. In the future, we will almost never need to manually free an image.

examples/llava/clip.cpp Outdated Show resolved Hide resolved
examples/llava/clip.cpp Outdated Show resolved Hide resolved
@tc-mb
Copy link
Contributor Author

tc-mb commented Oct 16, 2024

Please also note that I'm refactoring all this parts in #9687 to be benefit of cpp automatic (de)allocation. In the future, we will almost never need to manually free an image.

Cool, I am a staff of the MiniCPM-V team. if you need to refactor the MiniCPM-V series, feel free ask me for help.

@ngxson ngxson changed the title fix memory leaks in minicpmv llava : fix memory leaks in minicpmv Oct 16, 2024
Copy link
Collaborator

@ngxson ngxson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I'm re-running the CI. Will merge once it passes

@woojh3690
Copy link

Any update?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants