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

Replace device_vector with device_uvector in null_mask #7715

Merged
merged 1 commit into from
Mar 25, 2021

Conversation

harrism
Copy link
Member

@harrism harrism commented Mar 25, 2021

Replaces remaining device_vector instances in null_mask.cu with device_uvector. Change the interface of segmented_count_[un]set_bits to take host_span instead of std::vector.

@harrism harrism requested a review from a team as a code owner March 25, 2021 00:03
@harrism harrism requested review from trxcllnt and jrhemstad March 25, 2021 00:03
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Mar 25, 2021
@harrism harrism added 3 - Ready for Review Ready for review by team breaking Breaking change improvement Improvement / enhancement to an existing function labels Mar 25, 2021
@harrism harrism self-assigned this Mar 25, 2021
@@ -53,7 +53,7 @@ void set_null_mask(bitmask_type *bitmask,
* @param[in] stream CUDA stream used for device memory operations and kernel launches.
*/
std::vector<size_type> segmented_count_set_bits(bitmask_type const *bitmask,
std::vector<size_type> const &indices,
host_span<size_type const> indices,
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume the host_span being passed by value here now is not a performance concern since it's much smaller than a vector (just a wrapper around a raw pointer)? Perhaps more for my edification than anything else: is there any particular reason to prefer not using the reference in this case?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think Vukasin covered this in his Better Code presentation on spans, but in general one of the points of views is you pass by value, though I don't remember why. It's just a pointer and a size. We don't pass column_view by value because they are not true views. There is an open issue for that one...

Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

Looked fine to me, just one small question.

@harrism harrism removed the 3 - Ready for Review Ready for review by team label Mar 25, 2021
@harrism
Copy link
Member Author

harrism commented Mar 25, 2021

@gpucibot merge

@ttnghia
Copy link
Contributor

ttnghia commented Mar 25, 2021

Rerun tests.

Previous CI test error:

ImportError: /workspace/python/cudf/cudf/_lib/concat.cpython-38-x86_64-linux-gnu.so: undefined symbol: 
_ZN4cudf11concatenateENS_9host_spanIKNS_11column_viewELm18446744073709551615EEEPN3rmm2mr22device_memory_resourceE

@codecov
Copy link

codecov bot commented Mar 25, 2021

Codecov Report

Merging #7715 (3b89beb) into branch-0.19 (7871e7a) will increase coverage by 0.66%.
The diff coverage is n/a.

❗ Current head 3b89beb differs from pull request most recent head 71b7fc6. Consider uploading reports for the commit 71b7fc6 to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.19    #7715      +/-   ##
===============================================
+ Coverage        81.86%   82.53%   +0.66%     
===============================================
  Files              101      101              
  Lines            16884    17453     +569     
===============================================
+ Hits             13822    14404     +582     
+ Misses            3062     3049      -13     
Impacted Files Coverage Δ
python/cudf/cudf/core/column/decimal.py 92.95% <0.00%> (-1.92%) ⬇️
python/cudf/cudf/core/column/lists.py 90.00% <0.00%> (-1.40%) ⬇️
python/cudf/cudf/core/column/numerical.py 94.83% <0.00%> (-0.20%) ⬇️
python/cudf/cudf/core/column/column.py 87.61% <0.00%> (-0.15%) ⬇️
python/cudf/cudf/utils/utils.py 85.36% <0.00%> (-0.07%) ⬇️
python/cudf/cudf/io/feather.py 100.00% <0.00%> (ø)
python/cudf/cudf/utils/ioutils.py 78.71% <0.00%> (ø)
python/cudf/cudf/comm/serialize.py 0.00% <0.00%> (ø)
python/cudf/cudf/_fuzz_testing/io.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/column/struct.py 100.00% <0.00%> (ø)
... and 42 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3136124...71b7fc6. Read the comment docs.

@rapids-bot rapids-bot bot merged commit a9b4705 into rapidsai:branch-0.19 Mar 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants