Convert cudf::concatenate APIs to use spans and device_uvector #7621
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
harrism
added
breaking
harrism
added
3 - Ready for Review
Codecov Report
@@ Coverage Diff @@
## branch-0.19 #7621 +/- ##
===============================================
+ Coverage 81.86% 82.57% +0.70%
===============================================
Files 101 101
Lines 16884 17551 +667
===============================================
+ Hits 13822 14492 +670
+ Misses 3062 3059 -3
Continue to review full report at Codecov.
|
jrhemstad
approved these changes
Mar 23, 2021
|
This change helps more than I expected. Concatenate is significantly faster now, except for concatenating strings columns, where the change seems to be noise. |
|
@harrism Does the perf chart show relative difference? |
|
@vuule yes they are relative. The computation is With my latest commit I have also replaced |
vuule
approved these changes
Mar 24, 2021
… them with vectors
harrism
commented
Mar 24, 2021
Comment on lines
23
to
33
| # these don't exist in the C++, but construction from vector is possible since | ||
| # a host_span can be implicitly constructed from a vector, so we add these for convenience | ||
| cdef device_buffer concatenate_masks "cudf::concatenate_masks"( | ||
| vector[column_view] views | ||
| ) except + | ||
| cdef unique_ptr[column] concatenate_columns "cudf::concatenate"( | ||
| const vector[column_view] columns | ||
| vector[column_view] columns | ||
| ) except + | ||
| cdef unique_ptr[table] concatenate_tables "cudf::concatenate"( | ||
| const vector[table_view] tables | ||
| vector[table_view] tables | ||
| ) except + |
There was a problem hiding this comment.
@shwina these are the same as the original Cython code for concatenate, and the only ones currently used. So one idea is that I could revert my Cython changes in this PR. We could then wait and add host_span in Cython only if we need it... What do you think?
There was a problem hiding this comment.
The Cython definitions are nice to have. I suspect at some point we're going to need them, so let's not delete. Agree about keeping the concat definitions the same though.
shwina
reviewed
Mar 24, 2021
|
@gpucibot merge |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Contributes to #7287
This PR replaces
std::vectorwithhost_spanin public and detailcudf::contatenatefunctions, and replacesrmm::device_vectorwithrmm::device_uvectorin the concatenate implementations.It also strengthens the SFINAE restrictions on
cudf::host_spanandcudf::device_spanso that they cannot be constructed from containers unless the container's value_type is the same as the span's value_type.This PR also