Skip to content

Use cub radix sort directly for all fixed-width-types in cudf::sorted_order #19227

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 4 commits into
base: branch-25.08
Choose a base branch
from
Open

Use cub radix sort directly for all fixed-width-types in cudf::sorted_order #19227

wants to merge 4 commits into from

Conversation

davidwendt
Copy link
Contributor

@davidwendt davidwendt commented Jun 26, 2025 •
edited
Loading

Description

Refactors the cudf::sorted_order logic to remove the separate stable/unstable path for faster sort (now called sorted-order-radix).
This makes the code more consistent with the cudf::sort logic as well.

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 26, 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 26, 2025
@copy-pr-bot copy-pr-bot
Copy link

copy-pr-bot bot commented Jun 26, 2025

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 26, 2025
@davidwendt
Copy link
Contributor Author

/ok to test

@davidwendt davidwendt added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Jun 27, 2025
@davidwendt davidwendt marked this pull request as ready for review June 27, 2025 14:31
@davidwendt davidwendt requested review from a team as code owners June 27, 2025 14:31
@davidwendt davidwendt requested review from bdice and vuule June 27, 2025 14:31
ttnghia
ttnghia approved these changes Jun 27, 2025
View reviewed changes
Copy link
Contributor

@ttnghia ttnghia left a comment

Choose a reason for hiding this comment

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

I wonder if there is any benchmark showing performance change?

@davidwendt
Copy link
Contributor Author

I wonder if there is any benchmark showing performance change?

This is more reorganizing code. The thrust::sort_by_key calls were calling cub radix sort for us so there is no real change to performance. The performance changes were covered in #19137 (comment) and #19136 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shrshi shrshi shrshi approved these changes

@ttnghia ttnghia ttnghia approved these changes

@mhaseeb123 mhaseeb123 mhaseeb123 approved these changes

@bdice bdice Awaiting requested review from bdice bdice is a code owner automatically assigned from rapidsai/cudf-cpp-codeowners

@vuule vuule Awaiting requested review from vuule vuule is a code owner automatically assigned from rapidsai/cudf-cpp-codeowners

Assignees

@davidwendt davidwendt

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@davidwendt @shrshi @ttnghia @mhaseeb123