-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
[core] Fix a memory leak due to lineage counting #31488
Conversation
Signed-off-by: Yi Cheng <[email protected]>
@@ -483,6 +483,7 @@ int64_t ReferenceCounter::ReleaseLineageReferences(ReferenceTable::iterator ref) | |||
RAY_CHECK(arg_it->second.on_ref_removed == nullptr); | |||
lineage_bytes_evicted += ReleaseLineageReferences(arg_it); | |||
EraseReference(arg_it); | |||
ReleasePlasmaObject(arg_it); |
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.
The fix looks good to me! I think we just need to move this to go before the above line, though, because the previous line deletes arg_it
.
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.
Actually I think you could also just call DeleteReferenceInternal instead of EraseReference and ReleasePlasmaObject.
Signed-off-by: Yi Cheng <[email protected]>
Signed-off-by: Yi Cheng <[email protected]>
Signed-off-by: Yi Cheng <[email protected]>
Signed-off-by: Yi Cheng <[email protected]>
RAY_CHECK(arg_it->second.on_ref_removed == nullptr); | ||
lineage_bytes_evicted += ReleaseLineageReferences(arg_it); | ||
EraseReference(arg_it); | ||
DeleteReferenceInternal(arg_it, nullptr); |
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.
Should we rewrite the docstring for these 2 APIs? (EraseReference, DeleteReferenceInternal).
From reading the docstring, it is not clear why EraseReference doesn't work but DeleteReferenceInternal does from this code
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.
I realize that DeleteReferenceInternal also doesn't work. It need to be
ReleasePlasmaObject
EraseReference
I'm not very familiar with this part so I'm going to leave it here.
Signed-off-by: Yi Cheng <[email protected]>
Assume two objects A and B and B depends on A. If A got delete first, it's won't be released because we need to preserve it for B. The bug here is that even B is released, A won't be released and thus memory leak. This PR fixed this issue by force delete the object when the ref goes to 0.
Signed-off-by: Yi Cheng [email protected]
Why are these changes needed?
Assume two objects A and B and B depends on A. If A got delete first, it's won't be released because we need to preserve it for B.
The bug here is that even B is released, A won't be released and thus memory leak.
This PR fixed this issue by force delete the object when the ref goes to 0.
Related issue number
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.