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

[REVIEW] Move template param to member var to improve compile of hash/groupby.cu #6835

Merged
merged 4 commits into from
Nov 30, 2020

Conversation

davidwendt
Copy link
Contributor

@davidwendt davidwendt commented Nov 23, 2020

The compile time/size of cpp/src/groupby/hash/groupby.cu is one of the top offenders for building libcudf.

Current top 5 slowest compiles:

1813479 CMakeFiles/cudf_base.dir/src/sort/sort.cu.o
1805726 CMakeFiles/cudf_base.dir/src/sort/stable_sort.cu.o
1091904 CMakeFiles/cudf_base.dir/src/stream_compaction/drop_duplicates.cu.o
 979061 CMakeFiles/cudf_base.dir/src/groupby/hash/groupby.cu.o  <----
 685116 CMakeFiles/cudf_join.dir/src/join/semi_join.cu.o
...

The two sort.cu files may be improved in a later PR. The drop_duplicates.cu is being addressed in #6822

The simple change here is to compute_single_pass_aggs functor defined here:

template <bool skip_rows_with_nulls, typename Map>
struct compute_single_pass_aggs {

The skip_rows_with_nulls template parameter is set to avoid calling (and inlining) cudf::bit_is_set(). This function is minimal compared to the cudf::detail::aggregate_row function that must be inlined twice to accommodate this template parameter. Simply changing this to a member variable means we still do not incur an extra call to cudf::bit_is_set() when appropriate but also means we generate half as much device code for this specific function. The cudf::detail::aggregate_row code is quite significant.

This change reduces the compile time for hash/groupby.cu from 16 minutes to 9 minutes. This moves it out of the top 5 (for now). This also reduces the size of the libcudf_base.so by ~5MB.

There is no functional changes to any logic. The gbenchmark/GROUPBY_BENCH shows no change in performance.

@davidwendt davidwendt requested a review from a team as a code owner November 23, 2020 21:59
@davidwendt davidwendt self-assigned this Nov 23, 2020
@davidwendt davidwendt added 3 - Ready for Review Ready for review by team libcudf Affects libcudf (C++/CUDA) code. labels Nov 23, 2020
@davidwendt davidwendt added this to PR-WIP in v0.17 Release via automation Nov 23, 2020
@davidwendt davidwendt moved this from PR-WIP to PR-Needs review in v0.17 Release Nov 23, 2020
@davidwendt
Copy link
Contributor Author

Error 137 is "out of memory"

@codecov
Copy link

codecov bot commented Nov 24, 2020

Codecov Report

Merging #6835 (57a6f5b) into branch-0.17 (45bd967) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           branch-0.17    #6835   +/-   ##
============================================
  Coverage        81.94%   81.94%           
============================================
  Files               96       96           
  Lines            16164    16164           
============================================
  Hits             13246    13246           
  Misses            2918     2918           

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 45bd967...57a6f5b. Read the comment docs.

v0.17 Release automation moved this from PR-Needs review to PR-Reviewer approved Nov 24, 2020
@harrism harrism removed the request for review from nvdbaranec November 24, 2020 04:07
@harrism
Copy link
Member

harrism commented Nov 24, 2020

@nvdbaranec is out this week so removing his review request.

@davidwendt
Copy link
Contributor Author

rerun tests

@davidwendt davidwendt merged commit 76799a1 into rapidsai:branch-0.17 Nov 30, 2020
v0.17 Release automation moved this from PR-Reviewer approved to Done Nov 30, 2020
@davidwendt davidwendt deleted the compile-groupby branch November 30, 2020 16:31
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 libcudf Affects libcudf (C++/CUDA) code.
Projects
No open projects
v0.17 Release
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants