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

Use stream allocator adaptor for hash join table #9704

Merged

Conversation

PointKernel
Copy link
Member

Depends on NVIDIA/cuCollections#119

This PR replaces the default hash join allocator with the corresponding rmm::mr::stream_allocator_adaptor. It accommodates new cuco::allocator APIs that don't take stream as input argument.

@PointKernel PointKernel added 3 - Ready for Review Ready for review by team CMake CMake build issue tech debt improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Nov 16, 2021
@PointKernel PointKernel self-assigned this Nov 16, 2021
@PointKernel PointKernel requested review from a team as code owners November 16, 2021 21:20
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Nov 16, 2021
@PointKernel
Copy link
Member Author

Current CMake changes are dedicated to CI tests only. Will update the main branch git tag once NVIDIA/cuCollections#119 is merged.

@codecov
Copy link

codecov bot commented Nov 16, 2021

Codecov Report

Merging #9704 (bcc615f) into branch-22.02 (967a333) will decrease coverage by 0.04%.
The diff coverage is 5.79%.

Impacted file tree graph

@@               Coverage Diff                @@
##           branch-22.02    #9704      +/-   ##
================================================
- Coverage         10.49%   10.44%   -0.05%     
================================================
  Files               119      119              
  Lines             20305    20422     +117     
================================================
+ Hits               2130     2133       +3     
- Misses            18175    18289     +114     
Impacted Files Coverage Δ
python/cudf/cudf/__init__.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/_base_index.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/column/column.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/column/string.py 0.00% <ø> (ø)
python/cudf/cudf/core/frame.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/groupby/groupby.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/index.py 0.00% <ø> (ø)
python/cudf/cudf/core/indexed_frame.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/multiindex.py 0.00% <0.00%> (ø)
python/cudf/cudf/io/json.py 0.00% <ø> (ø)
... and 8 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 7ac8aac...bcc615f. Read the comment docs.

@harrism harrism changed the title Use stream alloactor adaptor for hash join table Use stream allocator adaptor for hash join table Nov 16, 2021
@karthikeyann karthikeyann added the 5 - DO NOT MERGE Hold off on merging; see PR for details label Nov 22, 2021
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.

Looks good.
Holding this until dependent PR is merged.

cpp/cmake/thirdparty/get_cucollections.cmake Outdated Show resolved Hide resolved
@jrhemstad jrhemstad added 0 - Waiting on Author Waiting for author to respond to review and removed 5 - Merge After Dependencies 5 - DO NOT MERGE Hold off on merging; see PR for details 3 - Ready for Review Ready for review by team labels Dec 2, 2021
@PointKernel PointKernel removed the 0 - Waiting on Author Waiting for author to respond to review label Dec 2, 2021
@jrhemstad jrhemstad added the 5 - Ready to Merge Testing and reviews complete, ready to merge label Dec 2, 2021
@harrism harrism added 3 - Ready for Review Ready for review by team and removed 5 - Ready to Merge Testing and reviews complete, ready to merge labels Dec 3, 2021
@@ -21,7 +21,7 @@ function(find_and_configure_cucollections)
cuco 0.0
GLOBAL_TARGETS cuco::cuco
CPM_ARGS GITHUB_REPOSITORY NVIDIA/cuCollections
GIT_TAG f0eecb203590f1f4ac4a9f1700229f4434ac64dc
GIT_TAG 6433e8ad7571f14cc5384051b049029c60dd1ce0
Copy link
Member

Choose a reason for hiding this comment

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

If the dependency PR is already merged, shouldn't this GIT_TAG be removed and just depend on a release branch?

Copy link
Member Author

Choose a reason for hiding this comment

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

cuco doesn't have a release yet thus we choose to fetch a specific commit instead of the main branch (dev).

cpp/src/join/join_common_utils.hpp Outdated Show resolved Hide resolved
@jrhemstad jrhemstad requested a review from harrism December 6, 2021 17:41
@jrhemstad jrhemstad added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels Dec 6, 2021
@jrhemstad
Copy link
Contributor

@harrism @karthikeyann please re-review.

@harrism
Copy link
Member

harrism commented Dec 6, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 3b93f5c into rapidsai:branch-22.02 Dec 6, 2021
@PointKernel PointKernel deleted the hash-join-adaptor-allocator branch March 31, 2022 18:25
rapids-bot bot pushed a commit that referenced this pull request Apr 12, 2022
The `concurrent_unordered_multimap` is no longer used in libcudf. It has been replaced by `cuco::static_multimap`. The majority of the refactoring was done in PRs #8934 and #9704. A similar effort is in progress for `concurrent_unordered_map` and `cuco::static_map` in #9666 (and may depend on porting some optimizations from libcudf to cuco -- need to look into this before doing a direct replacement).

This partially resolves issue #10401.

cc: @PointKernel @vyasr

Authors:
  - Bradley Dice (https://github.com/bdice)

Approvers:
  - Yunsong Wang (https://github.com/PointKernel)
  - Vyas Ramasubramani (https://github.com/vyasr)
  - Jake Hemstad (https://github.com/jrhemstad)

URL: #10642
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge 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.

4 participants