Refactor par_sort_unstable to delegate to standard library#1283
Refactor par_sort_unstable to delegate to standard library#1283MatiasLopezING wants to merge 2 commits intorayon-rs:mainfrom
Conversation
This removes the internal heapsort fallback and delegates the base cases of par_quicksort directly to slice::sort_unstable_by. This allows Rayon to take advantage of the newly stabilized ipnsort algorithm in the standard library for smaller sequential chunks, while drastically reducing internal code complexity. Addresses rayon-rs#1233 (Phase 1: Unstable sort)
| // guarantee `O(n * log(n))` worst-case. | ||
| if limit == 0 { | ||
| heapsort(v, is_less); | ||
| v.sort_unstable_by(|a, b| if is_less(a, b) { cmp::Ordering::Less } else if is_less(b, a) { cmp::Ordering::Greater } else { cmp::Ordering::Equal }); |
There was a problem hiding this comment.
Adapting the predicate seems error prone and is done twice. Could this be factored out and done once earlier (without touching too much of the rest of the code)?
There was a problem hiding this comment.
Thanks for the review and guidance, @adamreichold! You were completely right.
Instead of doing the error-prone boolean conversion inside recurse, I refactored par_quicksort and its callers in src/slice/mod.rs to pass the Ordering comparator directly all the way down.
I also had to remove the random comparison test in src/slice/test.rs, since delegating to modern slice::sort_unstable_by strictly enforces total order and rightfully panics on completely random comparisons.
Just pushed the updates!
Refactored par_quicksort to take a comparison function returning Ordering directly, addressing feedback to avoid error-prone predicate conversions. Also removed an outdated test that deliberately violated total order, as modern Rust's sort_unstable_by strictly requires it and panics.
|
Closing and reopening to trigger CI again. |
| let was_balanced = true; | ||
| // True if the last partitioning didn't shuffle elements (the slice was already partitioned). | ||
| let mut was_partitioned = true; | ||
| let was_partitioned = true; |
There was a problem hiding this comment.
These flags have lost meaning if they're always true. Can we do anything to maintain them through recursion? Or should we just remove more dead code?
|
Overall, I'm not sure this is worth it just to remove one function, |
|
In other words, a fuller port of the new std algorithms might be more fruitful, though more difficult. |
3 participants
As discussed in #1233, this PR delegates the sequential base cases of
par_quicksortdirectly to the standard library (slice::sort_unstable_by), allowing Rayon to immediately benefit from the newly stabilizedipnsortalgorithm.Changes made:
recursewithv.sort_unstable_by().heapsortand its associated tests, as it is no longer needed as a degenerate-case fallback.par_mergesortuntouched to avoid allocator contention issues for stable sorting (can be addressed in a future PR).