-
Notifications
You must be signed in to change notification settings - Fork 288
[cudax] Make stream in async_buffer optional #6416
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
[cudax] Make stream in async_buffer optional #6416
Conversation
This comment has been minimized.
This comment has been minimized.
| _CCCL_HIDE_FROM_ABI async_buffer(const async_buffer& __other) | ||
| : __buf_(__other.memory_resource(), __other.stream(), __other.size()) |
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.
| _CCCL_HIDE_FROM_ABI async_buffer(const async_buffer& __other) | |
| : __buf_(__other.memory_resource(), __other.stream(), __other.size()) | |
| _CCCL_HIDE_FROM_ABI explicit async_buffer(const async_buffer& __other) | |
| : __buf_(__other.memory_resource(), __other.stream(), __other.size()) |
I would like to make this constructor explicit, to avoid accidental coppies
| _CCCL_HIDE_FROM_ABI constexpr void set_stream(stream_ref __new_stream) | ||
| //! @warning This does not synchronize between \p __new_stream and the current stream. It is the user's responsibility | ||
| //! to ensure proper stream order going forward | ||
| _CCCL_HIDE_FROM_ABI constexpr void set_stream(const ::cuda::std::optional<::cuda::stream_ref>& __new_stream) |
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.
What's the point in taking the stream as optinal?
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 had the same question as @davebayer here. Do we even need to literally use an optional<T> here at all? Can't we just use a sentinel value of cudaStream_t{0} ?
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 problem is cudaStream_t{0} is a valid stream. We have ~0 casted to cudaStream_t that is recognized as invalid stream, but I think something like buffer.set_stream(cuda::invalid_stream) seems more convoluted than optional.
For me buffer.set_stream(cuda::std::nullopt) was the most intuitive way to say the buffer should have no stream dependency. Especially since we already return optional from buffer.stream(). Maybe there is some better way to express it, but I don't think it should be some special cudaStream_t value or a special stream.
🥳 CI Workflow Results🟩 Finished in 59m 07s: Pass: 100%/42 | Total: 4h 03m | Max: 27m 46s | Hits: 96%/20774See results here. |
|
After some discussions I am closing this one. In the future if we still want similar functionality we will use |
We store a stream in
async_bufferto express dependencies the buffer should respect when its destructor is run. But sometimes it would be nice to just say a buffer no longer has any dependencies. For example:In the above case stream might get destroyed before
bufor there might be some work inserted into the stream that is not related to it and it would unnecessarily delay the deallocation.For these cases this PR changes the type stored in the buffer from
stream_reftocuda::std::optional<stream_ref>along with the getter and setter for it. This way in the above case user can callbuf.set_stream(cuda::std::nullopt)and buffer destructor will callresource.deallocate_sync()instead ofresource.deallocate()in that case.The side effect of that change is that whenever user wants to query the stream from a buffer they will need to remember to unpack it from an optional, but I think it's fine.
Currently all ways to create an
async_buffertake a stream that gets stored in the buffer. We could provide buffer construction API withno_initargument that does not take a stream and the buffer starts with the stream set tonullopt, but this need some thought first