-
Notifications
You must be signed in to change notification settings - Fork 902
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
Global stream pool #13922
Global stream pool #13922
Conversation
…eature/delta_binary
…eature/delta_binary
* @param count The number of `cuda_stream_view` objects to return. | ||
* @return Vector containing `count` stream views. | ||
*/ | ||
std::vector<rmm::cuda_stream_view> fork_streams(rmm::cuda_stream_view stream, std::size_t count); |
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'm not 100% sure, but can't think of a scenario where caller would not want to keep the streams.
std::vector<rmm::cuda_stream_view> fork_streams(rmm::cuda_stream_view stream, std::size_t count); | |
[[nodiscard]] std::vector<rmm::cuda_stream_view> fork_streams(rmm::cuda_stream_view stream, std::size_t count); |
Co-authored-by: Yunsong Wang <[email protected]>
/ok to test |
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.
🔥
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.
Great work. Just one more missing const
, I think.
* @param count The number of stream views to return. | ||
* @return Vector containing `count` stream views. | ||
*/ | ||
std::vector<rmm::cuda_stream_view> fork_streams(rmm::cuda_stream_view stream, uint32_t count); |
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.
Ultimately this will not be in the cudf namespace, but in rmm. I think there we should consider cudf::stream_view::fork(int num_streams)
and cudf::stream_view::join(std::span<rmm::cuda_stream_view const> other_streams)
.
I'm OK with this for now.
/ok to test |
/merge |
/ok to test |
Description
#13637 added a static stream pool object for use by the Parquet reader. This PR expands upon that by:
cudf::detail
namespace.Checklist