-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Fixes a memory leak during model unloading #3868
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this related to #3777. If so, be sure to link it to this PR.
It fixes that specific invalid read but I still see the issue described in #3777 even with this fix. |
47742c1
to
2964705
Compare
So the order between instance destruction and rate limiter removing the instance is arbitrary? |
I am not sure what you mean by arbitrary. The TritonModelInstance destruction will not remove the entry of the ModelInstanceContext from the RateLimiter. If we remove the context before destroying the instance, backend thread waits over the condition variable that gets destroyed with the context. This can lead to undefined behaviour. So, it is not arbitrary. Removing context from rate limiter should happen after instance destruction(backend thread is stopped and joined) as is happening in this case. The context is a wrapper over TritonModelInstance. |
c9a37e5
to
d42e93a
Compare
Before