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

Sync par_sort* with the standard library #1156

Merged
merged 1 commit into from
Apr 5, 2024
Merged

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Apr 4, 2024

Our parallel sort methods were already based on the standard library,
but they're out of date. Most of that implementation was moved to a
unified core::slice::sort module, with manual allocation functions
passed from the alloc crate and a simplified Vec.

This PR updates rayon to a unified slice::sort based on core's,
but dropping the manual allocation stuff to just use the real Vec. The
rest of the changes are straightforward parallelization, like using Fn
instead of FnMut, and the extended par_mergesort additions are still
the same as we had before.

Our parallel sort methods were already based on the standard library,
but they're out of date. Most of that implementation was moved to a
unified `core::slice::sort` module, with manual allocation functions
passed from the `alloc` crate and a simplified `Vec`.

This PR updates `rayon` to a unified `slice::sort` based on `core`'s,
but dropping the manual allocation stuff to just use the real `Vec`. The
rest of the changes are straightforward parallelization, like using `Fn`
instead of `FnMut`, and the extended `par_mergesort` additions are still
the same as we had before.
@adamreichold
Copy link
Collaborator

Does this update imply any functional changes or does it really only update the code organisation to match std? Meaning was the implementation itself already functionally equivalent, just the source organised differently?

@cuviper
Copy link
Member Author

cuviper commented Apr 5, 2024

The implementations used to be separate in the standard library too, until rust-lang/rust#104672, and we were roughly in sync with that former code. So the main point here is to make it easier to keep up with improvements, especially because I'm not an expert in sorting algorithms, but I think I understand the parallelization additions well enough. :)

The commit log on core::slice::sort has some performance tuning that sounds like it could help par_sort too, but it's hard to tell. The benchmarks we have under rayon-demo look mostly the same or slightly better on my runs, but there's a lot of variance.

@adamreichold
Copy link
Collaborator

Thanks!

I am mainly asking as to how this should be communicated and this sounds like "just reorganising code to keep in sync with std" not like "pulling fundamental improvements from std which Rayon was missing out on until now."

@cuviper
Copy link
Member Author

cuviper commented Apr 5, 2024

Yeah, that sounds right. I would categorize this under implementation details, and while there may be some incremental improvement, I don't think it would warrant a release note.

@cuviper cuviper added this pull request to the merge queue Apr 5, 2024
Merged via the queue into rayon-rs:main with commit 5623fcf Apr 5, 2024
4 checks passed
@cuviper cuviper deleted the newsort branch April 25, 2024 01:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants