Skip to content
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

Expose stream-ordering in column view APIs #17434

Merged
merged 9 commits into from
Jan 16, 2025

Conversation

shrshi
Copy link
Contributor

@shrshi shrshi commented Nov 25, 2024

Description

Adds stream parameter to

cudf::detail::column_view_base::null_count(begin, end)
cudf::detail::column_view_base::has_nulls(begin, end)

Note: Since stream-ordered prefetching is back-logged, we defer modifying the get_data member functions to accept a stream parameter for now.

Reference:

  1. [FEA] Expose public stream-ordered C++ APIs #13744
  2. [Story] Enabling prefetching of unified memory #16251 (comment)

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Nov 25, 2024
@shrshi shrshi changed the title stream support Expose stream-ordering in column view APIs Nov 25, 2024
@shrshi shrshi added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Nov 25, 2024
@shrshi shrshi marked this pull request as ready for review November 25, 2024 15:37
@shrshi shrshi requested a review from a team as a code owner November 25, 2024 15:37
@davidwendt davidwendt requested a review from vyasr November 25, 2024 17:04
Copy link
Contributor

@karthikeyann karthikeyann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Unit test updates to test default stream usage.

@davidwendt
Copy link
Contributor

Some of these are for the prefetch methods and I'm not sure if they should take a stream or not.
@GregoryKimball @vyasr

@bdice
Copy link
Contributor

bdice commented Dec 2, 2024

Some of these are for the prefetch methods and I'm not sure if they should take a stream or not. @GregoryKimball @vyasr

Yes, we want prefetching to take a stream. The underlying CUDA APIs are stream-ordered. We did not do so in the original design because it would require us to add streams to get_data, which we were not yet ready for at that time. This PR is a big milestone in the streams work, as it touches the very low level column APIs that we originally could not make stream-ordered. I am curious what specific blockers we have overcome to make this possible. We do have a note here about follow-up work once stream ordered get_data methods exist:

* removed once an method for stream-ordered data pointer access is added to

We need to add unit tests before this can be approved.

@vyasr
Copy link
Contributor

vyasr commented Dec 2, 2024

CUDA's prefetching APIs are stream-ordered, and when I first implemented prefetching in libcudf I did so under the assumption that we would eventually want to make it stream-ordered. While I still believe that is the right long-term technical solution, practically speaking it would require a significant amount of work to update all of libcudf to properly use a stream-ordered version of the prefetching APIs for no real benefit since we do not currently anticipate supporting multi-stream UVM use-cases. If that changes we can certainly reprioritize, but for now I would assume that stream-ordered prefetching in libcudf is backlogged for the foreseeable future in favor of higher priority changes.

@shrshi shrshi requested a review from a team as a code owner January 15, 2025 20:28
@github-actions github-actions bot added the CMake CMake build issue label Jan 15, 2025
@shrshi
Copy link
Contributor Author

shrshi commented Jan 16, 2025

/merge

@rapids-bot rapids-bot bot merged commit a4bbd09 into rapidsai:branch-25.02 Jan 16, 2025
111 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake CMake build issue improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants