Skip to content

Use radix sort for all fixed-width-types in cudf::sort - #19208

Merged
rapids-bot[bot] merged 11 commits into
NVIDIA:branch-25.08from
davidwendt:refactor-inplace-sort
Jun 26, 2025
Merged

Use radix sort for all fixed-width-types in cudf::sort#19208
rapids-bot[bot] merged 11 commits into
NVIDIA:branch-25.08from
davidwendt:refactor-inplace-sort

Conversation

@davidwendt

Copy link
Copy Markdown
Contributor

Description

Refactors internal dispatch function to use cub radix sort for all fixed-width types in cudf::sort
Also adds a benchmark for cudf::sort

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 self-assigned this Jun 24, 2025
@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 Jun 24, 2025
@copy-pr-bot

copy-pr-bot Bot commented Jun 24, 2025

Copy link
Copy Markdown

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@github-actions github-actions Bot added the CMake CMake build issue label Jun 24, 2025
@davidwendt

Copy link
Copy Markdown
Contributor Author

/ok to test

@davidwendt

Copy link
Copy Markdown
Contributor Author

/ok to test

@davidwendt

Copy link
Copy Markdown
Contributor Author

/ok to test

@davidwendt

Copy link
Copy Markdown
Contributor Author

Performance improvement similar to those for sort_ordered. The integer ones are unchanged since thrust was calling radix sort for us. The float and timestamp now use radix sort as well.

## [0] NVIDIA RTX A6000

|    T    |  stable  |  nulls  |  num_rows  |  cols  |   Ref Time |   Cmp Time |          Diff |   %Diff |
|---------|----------|---------|------------|--------|------------|------------|---------------|---------|
|   I32   |    0     |    0    |   262144   |    1   |  83.939 us |  79.200 us |     -4.740 us |  -5.65% |
|   I32   |    1     |    0    |   262144   |    1   |  84.394 us |  78.708 us |     -5.686 us |  -6.74% |
|   I32   |    0     |    0    |  2097152   |    1   | 218.597 us | 213.289 us |     -5.309 us |  -2.43% |
|   I32   |    1     |    0    |  2097152   |    1   | 218.998 us | 213.994 us |     -5.004 us |  -2.28% |
|   I32   |    0     |    0    |  16777216  |    1   |   1.198 ms |   1.184 ms |    -14.235 us |  -1.19% |
|   I32   |    1     |    0    |  16777216  |    1   |   1.186 ms |   1.184 ms |     -2.359 us |  -0.20% |
|   I32   |    0     |    0    |  67108864  |    1   |   4.537 ms |   4.540 ms |      2.572 us |   0.06% |
|   I32   |    1     |    0    |  67108864  |    1   |   4.534 ms |   4.540 ms |      6.389 us |   0.14% |
|   F32   |    0     |    0    |   262144   |    1   | 207.022 us | 153.256 us |    -53.767 us | -25.97% |
|   F32   |    1     |    0    |   262144   |    1   | 207.423 us | 153.565 us |    -53.857 us | -25.96% |
|   F32   |    0     |    0    |  2097152   |    1   | 961.763 us | 655.231 us |   -306.531 us | -31.87% |
|   F32   |    1     |    0    |  2097152   |    1   | 962.108 us | 655.494 us |   -306.615 us | -31.87% |
|   F32   |    0     |    0    |  16777216  |    1   |   6.700 ms |   4.394 ms |  -2306.049 us | -34.42% |
|   F32   |    1     |    0    |  16777216  |    1   |   6.709 ms |   4.395 ms |  -2314.116 us | -34.49% |
|   F32   |    0     |    0    |  67108864  |    1   |  26.987 ms |  17.252 ms |  -9734.893 us | -36.07% |
|   F32   |    1     |    0    |  67108864  |    1   |  26.956 ms |  17.256 ms |  -9700.540 us | -35.99% |
| time_ms |    0     |    0    |   262144   |    1   | 189.160 us | 128.811 us |    -60.348 us | -31.90% |
| time_ms |    1     |    0    |   262144   |    1   | 186.618 us | 129.041 us |    -57.577 us | -30.85% |
| time_ms |    0     |    0    |  2097152   |    1   |   2.021 ms | 566.269 us |  -1455.118 us | -71.99% |
| time_ms |    1     |    0    |  2097152   |    1   |   2.017 ms | 565.946 us |  -1451.536 us | -71.95% |
| time_ms |    0     |    0    |  16777216  |    1   |  18.572 ms |   3.961 ms | -14610.808 us | -78.67% |
| time_ms |    1     |    0    |  16777216  |    1   |  18.566 ms |   3.962 ms | -14604.384 us | -78.66% |
| time_ms |    0     |    0    |  67108864  |    1   |  83.250 ms |  15.612 ms | -67637.807 us | -81.25% |
| time_ms |    1     |    0    |  67108864  |    1   |  83.240 ms |  15.612 ms | -67627.840 us | -81.24% |

@davidwendt
davidwendt marked this pull request as ready for review June 25, 2025 16:37
@davidwendt
davidwendt requested review from a team as code owners June 25, 2025 16:37

@bdice bdice 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.

Really nice, so glad to see this. No concerns from my side aside from naming. I'd like everything labeled "faster sort" to be labeled as "radix sort" or a variation thereof. I think it's a lot clearer to readers.

Comment thread cpp/benchmarks/sort/sort.cpp Outdated

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.

Maybe we can rename this and related files to radix_sort or sort_radix? I think the word "faster" isn't especially helpful to readers.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok. I'm working on one more follow on PR for this and I'm trying not to collide the names too much.
I like sort_radix too. Mainly because it puts the file closer alphabetically to the other sort source files otherwise the rank files get in between them.

@davidwendt davidwendt added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Jun 25, 2025
Comment thread cpp/src/sort/radix_sort_column.cu
@jrhemstad

Copy link
Copy Markdown
Contributor

This is cool. I'm impressed at how much faster the radix sort path is even with the extra materialization to initialize the float_pair array and convert it back.

@mhaseeb123 mhaseeb123 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.

LGTM. Some minor comments

Comment thread cpp/src/sort/radix_sort_column.cu
Comment thread cpp/src/sort/radix_sort_column.cu Outdated
Comment thread cpp/benchmarks/sort/sort.cpp Outdated
Comment thread cpp/CMakeLists.txt
Comment on lines +687 to +688
src/sort/sort_column.cu
src/sort/sort_radix.cu

@ttnghia ttnghia Jun 26, 2025

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.

This makes me think that alongside with column and table now we have some "radix" data structure to sort 😃

@davidwendt

Copy link
Copy Markdown
Contributor Author

/merge

@rapids-bot
rapids-bot Bot merged commit b2a88f9 into NVIDIA:branch-25.08 Jun 26, 2025
@davidwendt
davidwendt deleted the refactor-inplace-sort branch June 26, 2025 15:40
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.

5 participants