Skip to content

Remove unneeded temporary device vector for strings scatter specialization - #7409

Merged
rapids-bot[bot] merged 9 commits into
NVIDIA:branch-0.19from
davidwendt:benchmark-strings-copy
Feb 23, 2021
Merged

Remove unneeded temporary device vector for strings scatter specialization#7409
rapids-bot[bot] merged 9 commits into
NVIDIA:branch-0.19from
davidwendt:benchmark-strings-copy

Conversation

@davidwendt

Copy link
Copy Markdown
Contributor

The specialization logic for scatter on strings column includes building a temporary device_vector of string_view objects for the source column. The builtin iterators for an input column_view will work for string_view and so this extra device_vector is not required.

The utilities for creating a device_vector for string_views is also changed to use device_uvector instead. This also removed an unnecessary parameter as well as other minor changes.

I also added a gbenchmark for scatter that includes strings. Removing the extra device_vector showed a small 10-15% performance improvement.

@davidwendt davidwendt added 3 - Ready for Review Ready for review by team libcudf Affects libcudf (C++/CUDA) code. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Feb 18, 2021
@davidwendt davidwendt self-assigned this Feb 18, 2021
@davidwendt
davidwendt requested review from a team as code owners February 18, 2021 16:33
@github-actions github-actions Bot added the CMake CMake build issue label Feb 18, 2021
@codecov

codecov Bot commented Feb 18, 2021

Copy link
Copy Markdown

Codecov Report

❗ No coverage uploaded for pull request base (branch-0.19@580f9a2). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff               @@
##             branch-0.19    #7409   +/-   ##
==============================================
  Coverage               ?   82.23%           
==============================================
  Files                  ?      101           
  Lines                  ?    17060           
  Branches               ?        0           
==============================================
  Hits                   ?    14029           
  Misses                 ?     3031           
  Partials               ?        0           

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 580f9a2...9a699b7. Read the comment docs.

Comment thread cpp/include/cudf/strings/detail/utilities.hpp Outdated

@kkraus14 kkraus14 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

cmake lgtm

@harrism harrism removed the 3 - Ready for Review Ready for review by team label Feb 23, 2021
@harrism

harrism commented Feb 23, 2021

Copy link
Copy Markdown
Contributor

@gpucibot merge

@rapids-bot
rapids-bot Bot merged commit 43b44e1 into NVIDIA:branch-0.19 Feb 23, 2021
@davidwendt
davidwendt deleted the benchmark-strings-copy branch February 23, 2021 14:03
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.

5 participants