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

Move template parameter to function parameter in cudf::detail::left_semi_anti_join #8914

Merged

Conversation

davidwendt
Copy link
Contributor

The semi_join.cu takes about 6 minutes to compile on my Linux 18.04 desktop when doing a full build of libcudf. The join_kind template parameter used internally in cudf::detail::left_semi_anti_join for left_semi_join and left_anti_join APIs is not used in a constexpr or to pass to any other templated function. This PR moves the template parameter to a runtime parameter on the detail functions reducing the compile time for semi_join.cu by ~2x.

Another improvement includes un-inlining the is_trivial_join utility function to reduce the compile time for files that include join_common_utils.hpp.

Finally, the device vector used as a gather map in detail::left_semi_anti_join was wrapped with a column_view in order to call detail::gather without iterators. This allowed not including the heavy gather.cuh. This improved the compile time about 10% and reduced the object file semi_join.cu.o size by 2x.

@davidwendt davidwendt added 2 - In Progress Currently a work in progress libcudf Affects libcudf (C++/CUDA) code. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jul 30, 2021
@davidwendt davidwendt self-assigned this Jul 30, 2021
@davidwendt davidwendt requested a review from a team as a code owner July 30, 2021 16:27
@codecov
Copy link

codecov bot commented Jul 30, 2021

Codecov Report

Merging #8914 (be7eab5) into branch-21.10 (18f7c01) will decrease coverage by 0.08%.
The diff coverage is n/a.

❗ Current head be7eab5 differs from pull request most recent head 2270b90. Consider uploading reports for the commit 2270b90 to get more accurate results
Impacted file tree graph

@@               Coverage Diff                @@
##           branch-21.10    #8914      +/-   ##
================================================
- Coverage         10.67%   10.59%   -0.09%     
================================================
  Files               110      116       +6     
  Lines             18271    19040     +769     
================================================
+ Hits               1951     2017      +66     
- Misses            16320    17023     +703     
Impacted Files Coverage Δ
python/cudf/cudf/__init__.py 0.00% <ø> (ø)
python/cudf/cudf/core/__init__.py 0.00% <ø> (ø)
python/cudf/cudf/core/column/categorical.py 0.00% <ø> (ø)
python/cudf/cudf/core/column/column.py 0.00% <ø> (ø)
python/cudf/cudf/core/column/lists.py 0.00% <ø> (ø)
python/cudf/cudf/core/column/numerical.py 0.00% <ø> (ø)
python/cudf/cudf/core/column/string.py 0.00% <ø> (ø)
python/cudf/cudf/core/column/struct.py 0.00% <ø> (ø)
python/cudf/cudf/core/dataframe.py 0.00% <ø> (ø)
python/cudf/cudf/core/frame.py 0.00% <ø> (ø)
... and 75 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 8e9f0aa...2270b90. Read the comment docs.

@vyasr
Copy link
Contributor

vyasr commented Jul 30, 2021

In #8815 I also created a join_common_utils.cuh header for code that's common for normal and conditional joins in the CUDA layers. A significant chunk of that file is also inline functions. I doubt they'll have as significant of an impact, but perhaps it makes sense to break some of those out of the header as well so that they don't get recompiled for both sets of join TUs? I believe at least a few of them are not templated.

@davidwendt
Copy link
Contributor Author

In #8815 I also created a join_common_utils.cuh header for code that's common for normal and conditional joins in the CUDA layers. A significant chunk of that file is also inline functions. I doubt they'll have as significant of an impact, but perhaps it makes sense to break some of those out of the header as well so that they don't get recompiled for both sets of join TUs? I believe at least a few of them are not templated.

Yes, thanks. I see there are 3 host-only utility functions that could be moved to a .cpp or .cu file. Perhaps I could add a join_utils.cpp for these and the is_trivial_join utility as well.

@github-actions github-actions bot added the CMake CMake build issue label Jul 30, 2021
Copy link
Contributor

@hyperbolic2346 hyperbolic2346 left a comment

Choose a reason for hiding this comment

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

Do we know the run-time impact of these changes? My only concern here is that we may trade compile time performance for run-time performance.

@davidwendt
Copy link
Contributor Author

Do we know the run-time impact of these changes? My only concern here is that we may trade compile time performance for run-time performance.

Running benchmarks/JOIN_BENCH did not show any change in runtime performance for these changes.

@davidwendt davidwendt added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Aug 2, 2021
Copy link
Member

@harrism harrism 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 @davidwendt .

@harrism
Copy link
Member

harrism commented Aug 4, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 29b5f9a into rapidsai:branch-21.10 Aug 4, 2021
@davidwendt davidwendt deleted the joinkind-tparam-to-param branch August 5, 2021 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team 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