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

Refactor group_nunique.cu to use nullate::DYNAMIC for reduce-by-key functor #11482

Merged

Conversation

davidwendt
Copy link
Contributor

Description

Refactored the group_nunique.cu source to use the nullate::DYNAMIC for the equal operator and the unique-iterator. This improves the compile time by almost 2x without much change to performance by reducing the number of calls to thrust::reduce_by_key.

Found while investigating compile issues for #11437

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@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 Aug 5, 2022
@davidwendt davidwendt self-assigned this Aug 5, 2022
@davidwendt davidwendt requested a review from a team as a code owner August 5, 2022 19:02
@davidwendt
Copy link
Contributor Author

davidwendt commented Aug 5, 2022

The benchmark from #11472 was used to check the change in performance.

# groupby_nunique

## [0] NVIDIA RTX A6000

|  T  |  num_rows  |  null_frequency  |   Ref Time |   Ref Noise |   Cmp Time |   Cmp Noise |       Diff |   %Diff | 
|-----|------------|------------------|------------|-------------|------------|-------------|------------|---------|
| I32 |    2^12    |        0         | 342.972 us |       1.60% | 345.004 us |       2.03% |   2.032 us |   0.59% | 
| I32 |    2^16    |        0         | 520.081 us |       0.33% | 516.265 us |       0.61% |  -3.816 us |  -0.73% |
| I32 |    2^20    |        0         |   2.630 ms |       0.50% |   2.653 ms |       0.44% |  22.267 us |   0.85% |
| I32 |    2^24    |        0         |  58.545 ms |       0.06% |  58.617 ms |       0.05% |  72.704 us |   0.12% |
| I32 |    2^12    |       0.5        | 420.704 us |       0.30% | 424.737 us |       0.30% |   4.033 us |   0.96% |
| I32 |    2^16    |       0.5        | 657.260 us |       0.62% | 664.688 us |       0.49% |   7.428 us |   1.13% |
| I32 |    2^20    |       0.5        |   2.633 ms |       0.29% |   2.656 ms |       0.31% |  23.650 us |   0.90% |
| I32 |    2^24    |       0.5        |  48.216 ms |       0.09% |  48.363 ms |       0.11% | 147.083 us |   0.31% |
| I64 |    2^12    |        0         | 400.970 us |       0.40% | 407.797 us |       0.35% |   6.827 us |   1.70% |
| I64 |    2^16    |        0         | 583.841 us |       0.28% | 587.126 us |       0.42% |   3.284 us |   0.56% |
| I64 |    2^20    |        0         |   3.725 ms |       0.38% |   3.751 ms |       0.37% |  25.333 us |   0.68% |
| I64 |    2^24    |        0         |  77.178 ms |       0.15% |  77.230 ms |       0.17% |  51.759 us |   0.07% |
| I64 |    2^12    |       0.5        | 463.510 us |       0.50% | 466.201 us |       0.82% |   2.691 us |   0.58% |
| I64 |    2^16    |       0.5        | 714.861 us |       0.51% | 710.432 us |       0.35% |  -4.429 us |  -0.62% |
| I64 |    2^20    |       0.5        |   2.960 ms |       0.50% |   2.960 ms |       0.40% |  -0.151 us |  -0.01% |
| I64 |    2^24    |       0.5        |  56.646 ms |       0.10% |  56.683 ms |       0.06% |  36.585 us |   0.06% |

All diffs are less than 2%

Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

I have to say, I love nullate::DYNAMIC. Such a nice solution.

@codecov
Copy link

codecov bot commented Aug 5, 2022

Codecov Report

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

❗ Current head 2fd552a differs from pull request most recent head 94eb78a. Consider uploading reports for the commit 94eb78a to get more accurate results

@@               Coverage Diff               @@
##             branch-22.10   #11482   +/-   ##
===============================================
  Coverage                ?   86.47%           
===============================================
  Files                   ?      144           
  Lines                   ?    22856           
  Branches                ?        0           
===============================================
  Hits                    ?    19765           
  Misses                  ?     3091           
  Partials                ?        0           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@davidwendt
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 5681656 into rapidsai:branch-22.10 Aug 5, 2022
@davidwendt davidwendt deleted the compile-groupby-nunique branch August 5, 2022 21:39
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 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