Change strings copy_if_else to use optional-iterator instead of pair-iterator #9266
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
davidwendt
added
2 - In Progress
|
rerun tests |
Codecov Report
@@ Coverage Diff @@
## branch-21.12 #9266 +/- ##
================================================
- Coverage 10.79% 10.78% -0.01%
================================================
Files 116 116
Lines 18869 19438 +569
================================================
+ Hits 2036 2096 +60
- Misses 16833 17342 +509
Continue to review full report at Codecov.
|
davidwendt
added
3 - Ready for Review
karthikeyann
approved these changes
Sep 22, 2021
jrhemstad
reviewed
Oct 1, 2021
jrhemstad
reviewed
Oct 4, 2021
hyperbolic2346
approved these changes
Oct 5, 2021
mythrocks
reviewed
Oct 5, 2021
| @@ -26,6 +26,7 @@ | |||
| #include <cudf/utilities/type_dispatcher.hpp> | |||
|
|
|||
| #include <rmm/cuda_stream_view.hpp> | |||
| #include <rmm/exec_policy.hpp> | |||
There was a problem hiding this comment.
I'm curious why this change was necessary.
There was a problem hiding this comment.
This was previously included in the include file copy_if_else.cuh which did not use it. Cleaning up the headers there required moving it here where it is actually used.
mythrocks
approved these changes
Oct 5, 2021
|
@gpucibot merge |
rapids-bot bot
pushed a commit
that referenced
this pull request
Oct 13, 2021
This PR changes the `cudf::detail::copy_if_else` utility functions to accept an optional-iterator instead of a pair-iterator. This improves the compile time of source files by generating 4x less kernels since the two input data arrays can each have nulls requiring 4 different pair-iterators to be created to call it. The optional-iterator allows the nulls check to occur at runtime instead of compile time. The changes in this PR are for the callers of `detail::copy_if_else` to provide optional-iterators instead of pair-iterators. This PR is dependent on the changes in #9306 The benchmarks for the effected calling functions showed no significant change in runtime performance using the single optional-iterator over 4 unique pair-iterators. Two additional benchmarks are included cover non-null measurement which this PR impacts the most. Also, the `copy_tests.cu` was renamed `copy_tests.cpp` and the test that launched to the internal `cudf::detail::copy_if_else_kernel` was replaced with one with a data-set large enough to require multiple blocks. Related changes for the strings specific `cudf::strings::detail::copy_if_else` are in #9266 Authors: - David Wendt (https://github.com/davidwendt) Approvers: - Bradley Dice (https://github.com/bdice) - Robert Maynard (https://github.com/robertmaynard) - Jake Hemstad (https://github.com/jrhemstad) URL: #9324
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.
This part of bigger change happening for
copy.cuand specificallycopy_if_else.cuhchanging those to usemake_optional_iteratorinstead ofmake_pair_iterator. This is a piece of that overall change that could be separated into a smaller PR. The main effect here is to reduce the compile size and time ofsegmented_shift.cu.No behavior has changed and the benchmark results for
cudf::shifthas also not changed.