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

Tracking issue for release notes of #124032: Replace sort implementations #129661

Closed
1 of 4 tasks
rustbot opened this issue Aug 27, 2024 · 11 comments
Closed
1 of 4 tasks

Tracking issue for release notes of #124032: Replace sort implementations #129661

rustbot opened this issue Aug 27, 2024 · 11 comments
Labels
relnotes Marks issues that should be documented in the release notes of the next release. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Milestone

Comments

@rustbot
Copy link
Collaborator

rustbot commented Aug 27, 2024

This issue tracks the release notes text for #124032.

  • Issue is nominated for the responsible team (and T-release nomination is removed).
  • Proposed text is drafted by team responsible for underlying change.
  • Issue is nominated for release team review of clarity for wider audience.
  • Release team includes text in release notes/blog posts.

Release notes text:

The section title will be de-duplicated by the release team with other release notes issues.
Prefer to use the standard titles from previous releases.
More than one section can be included if needed.

# Compatibility Notes
- [Replace sort implementations](https://github.com/rust-lang/rust/pull/124032)

Release blog section (if any, leave blank if no section is expected):

@rustbot rustbot added I-release-nominated Nominated for the release team. relnotes Marks issues that should be documented in the release notes of the next release. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Aug 27, 2024
@workingjubilee workingjubilee added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. I-libs-nominated Nominated for discussion during a libs team meeting. I-libs-api-nominated Nominated for discussion during a libs-api team meeting. T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed I-release-nominated Nominated for the release team. labels Aug 27, 2024
@workingjubilee
Copy link
Member

cc @Voultapher @orlp if you want to have input on the relnotes here.

@joshtriplett
Copy link
Member

Sketch for the compatibility aspect (not covering the details of the performance improvement):

The implementations of sort functions (sort, sort_by, sort_by_cached_key, sort_by_key, sort_unstable, sort_unstable_by, sort_unstable_by_key) now have higher performance. These functions may now panic if the implementation of Ord or the supplied comparison function is not a total order.

@saethlin saethlin removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 27, 2024
@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.82.0, 1.81.0 Aug 27, 2024
@Voultapher
Copy link
Contributor

select_nth_unstable, select_nth_unstable_by and select_nth_unstable_by_key are also affected. Though we expect to see less impactful performance differences on x86-64, Arm however should see larger improvements as it sees larger improvements for the new unstable partition code.

@Voultapher
Copy link
Contributor

Regarding how to communicate the performance improvements, saying ~2x for both stable and unstable sort is somewhat based in reality, however I'd much prefer not to mention a single number x or % improvement, as that depends on the input pattern, input length and input type, and we see in our testing changes ranging from no change at all, all the way up to 17x faster execution. I'd rather we say that the performance improved significantly and link to the relevant section in the design documents that explains our methodology and results for evaluating the change in performance slice::sort and slice::sort_unstable.

@the8472
Copy link
Member

the8472 commented Aug 28, 2024

Do we have any source of randomness in the unstable sort to ensure it's really unstable (like hashmaps)? If not the compat note could also mention that the order changed in case someone accidentally relied on it.

@workingjubilee
Copy link
Member

No, we do not, it's deterministic within a version.

@workingjubilee
Copy link
Member

...and yes, it was in fact the case that someone asserted on this order in a test and the test was regressed as a result.

@orlp
Copy link
Contributor

orlp commented Aug 29, 2024

No, we do not, it's deterministic within a version.

We do right now. I don't know if we actually guarantee this, and perhaps we could in the future use randomness for e.g. pivot selection. It's a bit weird because the current stable "current implementation" notes mention determinism, but.. well, it's in the "current implementation" section, not general guarantees.

@Voultapher
Copy link
Contributor

Should probably use these permalinks specifically:

It can be useful to have the ability to fix typos. I plan on keeping these links stable.

@Amanieu
Copy link
Member

Amanieu commented Sep 3, 2024

This has been included in the release notes and libs is satisfied with the wording, closing.

@Amanieu Amanieu closed this as completed Sep 3, 2024
@dtolnay dtolnay removed I-libs-nominated Nominated for discussion during a libs team meeting. I-libs-api-nominated Nominated for discussion during a libs-api team meeting. labels Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

10 participants