cuda.core: allow updating Buffer deallocation streams - #2602
Conversation
Andy-Jost
added
enhancement
This comment has been minimized.
This comment has been minimized.
| stream = Stream._from_handle(Stream, h_stream) if h_stream else default_stream() | ||
| if not h_stream: | ||
| print( | ||
| "Warning: no deallocation stream was recorded; falling back to " |
There was a problem hiding this comment.
Should it say "no deallocation stream was recorded or the deallocation stream was closed"?
There was a problem hiding this comment.
It's saying no stream is available for ordering the destruction. Not necessarily that a stream was provided and was closed.
There was a problem hiding this comment.
Yes, but wouldn't this warning also appear when the deallocation stream was closed?
There was a problem hiding this comment.
Two points: This warning does not occur if a valid stream is passed to set_deallocation_stream() and that stream is later closed, because the DevicePtrHandle holds an independent stream handle. It can occur if an already closed stream is passed.
The follow-up in #2635 should address the second case: closed streams will be rejected throughout the API, including by set_deallocation_stream(). If the proper checks are in place throughout the codebase, this condition should never occur, which is why I flagged it as an internal error.
There was a problem hiding this comment.
... because the DevicePtrHandle holds an independent stream handle.
What does invoking Stream.close mean then?
There was a problem hiding this comment.
A StreamHandle is a std::shared_ptr<CUstream>. A Python Stream class holds one of those, and Stream.close resets the shared pointer, dropping that reference. Inside a DevicePtrHandle is an independent StreamHandle referring to the deallocation stream. Whatever the user-facing Python Stream object does, the embedded reference cannot be invalidated.
eef14b0 to
575c18f
Compare
Show how to move a Buffer between streams with an event so its eventual deallocation remains correctly ordered.
Keep the deallocation-stream tests aligned with the centralized test helper merged in NVIDIA#2624.
11cb846 to
7b04eac
Compare
| # The free operation runs after the copy on that stream. | ||
| buffer.close() | ||
| buffer = None | ||
| consumer_stream.sync() |
There was a problem hiding this comment.
This looks wrong: you close a buffer then sync a stream that has pending copy for the buffer. This sync could fail with a RuntimeError, couldn't it?
There was a problem hiding this comment.
The buffer in question has stream-ordered deallocation semantics, so closing it places a call to cuMemFreeAsync onto consumer_stream. The idea is that transferring the buffer to the stream "currently" using it will order its free after the work using it.
There was a problem hiding this comment.
Ah. For the other methods for stream-ordered operations stream is a mandatory parameter. That no stream is passed to close here caused me to think it is not an async operation.
| stream = Stream._from_handle(Stream, h_stream) if h_stream else default_stream() | ||
| if not h_stream: | ||
| print( | ||
| "Warning: no deallocation stream was recorded; falling back to " |
There was a problem hiding this comment.
Yes, but wouldn't this warning also appear when the deallocation stream was closed?
This comment has been minimized.
This comment has been minimized.
1 similar comment
|
2 participants
Summary
Closes #2600.
Add
Buffer.set_deallocation_streamso callers can replace the stream that orders eventual deallocation without closing the buffer. This supports transferring an allocation's lifetime to another stream while preserving the existingBufferobject.Changes
Buffer.set_deallocation_streamAPI and generated type stub.DeallocationStreamcapture path.Related Work
mainafter that PR merges.