-
Notifications
You must be signed in to change notification settings - Fork 5.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
[Core][Streaming Generator] Fix memory leak from the end of object stream object #38152
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.
Feels like there are cpp unit tests missing here to verify that the ref counting works as expected
@rkooo567 did you re-run the Serve repro to verify it's fixed? I'll kick off a long run on workspaces using these wheels too just to be sure. |
Good catch... Adding it now. Btw, is it possible to add that long-running test as a regular long running test or some sort of release tests? |
Unit tests are added |
…ream object (ray-project#38152) --------- Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com> Co-authored-by: Edward Oakes <ed.nmi.oakes@gmail.com>
…ream object (ray-project#38152) --------- Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com> Co-authored-by: Edward Oakes <ed.nmi.oakes@gmail.com> Signed-off-by: NripeshN <nn2012@hw.ac.uk>
…ream object (ray-project#38152) --------- Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com> Co-authored-by: Edward Oakes <ed.nmi.oakes@gmail.com> Signed-off-by: harborn <gangsheng.wu@intel.com>
…ream object (ray-project#38152) --------- Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com> Co-authored-by: Edward Oakes <ed.nmi.oakes@gmail.com>
…ream object (ray-project#38152) --------- Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com> Co-authored-by: Edward Oakes <ed.nmi.oakes@gmail.com> Signed-off-by: e428265 <arvind.chandramouli@lmco.com>
…ream object (ray-project#38152) --------- Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com> Co-authored-by: Edward Oakes <ed.nmi.oakes@gmail.com> Signed-off-by: Victor <vctr.y.m@example.com>
Why are these changes needed?
ray/src/ray/core_worker/core_worker.h
Line 446 in 4304e45
Currently, the objects are deleted from the memory store manually when the reference is removed (via
__dealloc__
fromObjectRef
class here;ray/python/ray/includes/object_ref.pxi
Line 62 in 4304e45
This causes the leak from streaming generator because it doesn't actually create a Cython object ref class for the end of stream.
This PR fixes the issue by manually deleting the objects from memory store when the ref of streaming generator objects goes to 0.
In the long term, we should fix this by modifying reference_count.cc to actually delete the object from the memory store. I will follow up with this, but I chose the current solution as a short term because we should merge this by 2.6.3 (which we try to branch cut by Sun)
Related issue number
Closes #38089
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.