Add `optimize_for_size` variants for stable and unstable sort as well as select_nth_unstable by Voultapher · Pull Request #129587 · rust-lang/rust

Voultapher · 2024-08-25T18:50:23Z

Stable sort uses a simple merge-sort that re-uses the existing - rather gnarly - merge function.
Unstable sort jumps directly to the branchless heapsort fallback.
select_nth_unstable jumps directly to the median_of_medians fallback, which is augmented with a custom tiny smallsort and partition impl.

Some code is duplicated but de-duplication would bring it's own problems. For example swap_if_less is critical for performance, if the sorting networks don't inline it perf drops drastically, however #[inline(always)] is also a poor fit, if the provided comparison function is huge, it gives the compiler an out to only instantiate swap_if_less once and call it. Another aspect that would suffer when making swap_if_less pub, is having to cfg out dozens of functions in in smallsort module.

Part of #125612

r? @Kobzol

…ll as select_nth_unstable - Stable sort uses a simple merge-sort that re-uses the existing - rather gnarly - merge function. - Unstable sort jumps directly to the branchless heapsort fallback. - select_nth_unstable jumps directly to the median_of_medians fallback, which is augmented with a custom tiny smallsort and partition impl. Some code is duplicated but de-duplication would bring it's own problems. For example `swap_if_less` is critical for performance, if the sorting networks don't inline it perf drops drastically, however `#[inline(always)]` is also a poor fit, if the provided comparison function is huge, it gives the compiler an out to only instantiate `swap_if_less` once and call it. Another aspect that would suffer when making `swap_if_less` pub, is having to cfg out dozens of functions in in smallsort module.

rustbot · 2024-08-25T18:50:31Z

r? @scottmcm

rustbot has assigned @scottmcm.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Voultapher · 2024-08-25T18:50:43Z

r? @Kobzol

rustbot · 2024-08-25T18:50:46Z

Could not assign reviewer from: Kobzol.
User(s) Kobzol are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

Kobzol · 2024-08-25T19:03:33Z

r? libs

Voultapher · 2024-08-26T07:36:24Z

One thing that is still outstanding is, how do we document the different implementation behavior?

I think adding even more text to the 10 places that document the current implementation of sort, sort_unstable and select_nth_unstable isn't a good idea. Maybe there could be something in a specific Rust book that mentions the implications of using optimize_for_size.

That said I think with how recent and WIP optimize_for_size still is, I'd say it's not a blocker for this PR.

…into quicksort module

Avoids the code duplication issue and results in smaller binary size, which after all is the purpose of the feature.

Voultapher · 2024-08-29T08:36:34Z

I think the open points have been addressed, if not, please re-open. Good to be merged from my side.

Voultapher · 2024-08-29T08:57:40Z

Would be nice to measure the binary-size impact of this change in a real setting, here are the result of our tool that we also used in the design documents:

slice::sort:

Configuration	Type	Size default (bytes)	Size `optimize_for_size` (bytes)
release	`u64`	5304	602
release	`String`	6630	838
release_lto_thin	`u64`	5317	570
release_lto_thin	`String`	6607	806
release_lto_thin_opt_level_s	`u64`	3748	584
release_lto_thin_opt_level_s	`String`	5448	813

slice::sort_unstable:

Configuration	Type	Size default (bytes)	Size `optimize_for_size` (bytes)
release	`u64`	3308	182
release	`String`	4968	488
release_lto_thin	`u64`	3308	182
release_lto_thin	`String`	4968	488
release_lto_thin_opt_level_s	`u64`	3034	168
release_lto_thin_opt_level_s	`String`	4281	410

slice::select_nth_unstable: index = v.len() / 2

Configuration	Type	Size default (bytes)	Size `optimize_for_size` (bytes)
release	`u64`	4331	3075
release	`String`	6204	4002
release_lto_thin	`u64`	4331	3075
release_lto_thin	`String`	6204	4002
release_lto_thin_opt_level_s	`u64`	3081	2032
release_lto_thin_opt_level_s	`String`	5603	3697

While using it results in slightly slammer binaries, it's not deemed worth it to add yet another sort algorithm to the standard library. select_nth_unstable has bigger binary-size problems.

Also skips stack alloc in stable sort if 16-bit target platform.

This regresses binary-size slightly for normal builds, but the important release_lto_thin_opt_level_s config sees a small improvement in binary-size and a larger types such as string and 1k see 2-3% run-time improvements with this change.

Voultapher · 2024-09-04T18:09:08Z

Pushed a bunch of changes, and updated the table above.

Voultapher · 2024-09-05T11:01:03Z

Some perf results:

cuviper · 2024-09-24T17:19:34Z

LGTM, and thanks for the detailed tables and graphs as well!

@bors r+ rollup=never
(just to be sure of independent perf)

bors · 2024-09-24T17:19:38Z

📌 Commit 5439198 has been approved by cuviper

It is now in the queue for this repository.

bors · 2024-09-24T18:48:11Z

⌛ Testing commit 5439198 with merge 363ae41...

bors · 2024-09-24T21:12:22Z

☀️ Test successful - checks-actions
Approved by: cuviper
Pushing 363ae41 to master...

rust-timer · 2024-09-24T22:30:16Z

Finished benchmarking commit (363ae41): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.3%	[0.2%, 0.3%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[0.2%, 0.3%]	3

Max RSS (memory usage)

Results (primary 5.7%, secondary 2.6%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	5.7%	[4.0%, 9.1%]	3
Regressions ❌ (secondary)	2.6%	[2.0%, 3.1%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	5.7%	[4.0%, 9.1%]	3

Cycles

Results (primary -0.5%, secondary 3.8%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.8%	[2.8%, 4.3%]	5
Improvements ✅ (primary)	-0.5%	[-0.5%, -0.5%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.5%	[-0.5%, -0.5%]	1

Binary size

Results (primary 0.0%, secondary -0.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.0%, 0.8%]	14
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.1%	[-0.1%, -0.0%]	21
Improvements ✅ (secondary)	-0.1%	[-0.2%, -0.1%]	38
All ❌✅ (primary)	0.0%	[-0.1%, 0.8%]	35

Bootstrap: 767.328s -> 768.432s (0.14%)
Artifact size: 340.88 MiB -> 340.84 MiB (-0.01%)

RalfJung · 2024-09-25T08:53:52Z

+#[cfg(not(feature = "optimize_for_size"))]
 pub(crate) mod quicksort;

+#[cfg(feature = "optimize_for_size")]


This likely needs to be #[cfg(any(feature = "optimize_for_size", target_pointer_width = "16"))], see #130818

Good catch.

Voultapher · 2024-09-25T13:27:30Z

Not sure the rust-timer run tells us anything useful here, given that all changes here happened to code behind a libcore feature that isn't being built IIUC.

Kobzol · 2024-10-01T07:21:43Z

I don't think that further investigation is needed. This has seemingly perturbed codegen a little bit, but otherwise it's hidden behind an optional flag. The image regression has flipped back right after.

@rustbot label: +perf-regression-triaged

rustbot assigned scottmcm Aug 25, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 25, 2024

This comment has been minimized.

Sign in to view

rustbot assigned cuviper and unassigned scottmcm Aug 25, 2024

tgross35 reviewed Aug 25, 2024

View reviewed changes

Comment thread library/core/src/slice/sort/select.rs Outdated

Comment thread library/core/src/slice/sort/select.rs Outdated

Comment thread library/core/src/slice/sort/select.rs Outdated

Comment thread library/core/src/slice/sort/select.rs Outdated

Voultapher added 2 commits August 26, 2024 11:17

Reduce code duplication by moving partition_lomuto_branchless_simple …

89c9a29

…into quicksort module

Convert cfg blocks to cfg_if

8756ba5

cuviper reviewed Aug 26, 2024

View reviewed changes

Comment thread library/core/src/slice/sort/select.rs Outdated

Comment thread library/core/src/slice/sort/select.rs Outdated

Comment thread library/core/src/slice/sort/stable/tiny.rs Outdated

Comment thread library/core/src/slice/sort/unstable/quicksort.rs

Voultapher added 2 commits August 27, 2024 10:10

Use last swap optimization in bubblesort

7815d77

Use simpler branchy swap logic in tiny merge sort

00eca77

Avoids the code duplication issue and results in smaller binary size, which after all is the purpose of the feature.

workingjubilee mentioned this pull request Sep 2, 2024

driftsort implementation uses an out-of-range literal on 16-bit targets #129910

Closed

Voultapher added 4 commits September 4, 2024 18:58

Drop bubble_sort

f2d4198

While using it results in slightly slammer binaries, it's not deemed worth it to add yet another sort algorithm to the standard library. select_nth_unstable has bigger binary-size problems.

Shrink heapsort further by combining sift_down loops

adb0e27

Select tiny sorts for 16-bit platforms

a0e4303

Also skips stack alloc in stable sort if 16-bit target platform.

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 24, 2024

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 24, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 24, 2024

bors merged commit 363ae41 into rust-lang:master Sep 24, 2024

rustbot added this to the 1.83.0 milestone Sep 24, 2024

rustbot added the perf-regression Performance regression. label Sep 24, 2024

RalfJung mentioned this pull request Sep 25, 2024

libcore no longer builds for AVR #130818

Closed

RalfJung reviewed Sep 25, 2024

View reviewed changes

rustbot added the perf-regression-triaged The performance regression has been triaged. label Oct 1, 2024

Voultapher mentioned this pull request Dec 18, 2024

[libc] Improve qsort llvm/llvm-project#120450

Merged

the8472 mentioned this pull request Jan 20, 2025

Tracking Issue for optimize_for_size standard library feature #125612

Open

5 tasks

jieyouxu mentioned this pull request Mar 27, 2025

ICE: trimmed_def_paths called, diagnostics were expected but none were emitted. #138979

Closed

Uh oh!

Uh oh!

Conversation

Voultapher commented Aug 25, 2024

Uh oh!

rustbot commented Aug 25, 2024

Uh oh!

Voultapher commented Aug 25, 2024

Uh oh!

rustbot commented Aug 25, 2024

Uh oh!

This comment has been minimized.

Kobzol commented Aug 25, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Voultapher commented Aug 26, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Voultapher commented Aug 29, 2024

Uh oh!

Voultapher commented Aug 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Voultapher commented Sep 4, 2024

Uh oh!

Voultapher commented Sep 5, 2024

Uh oh!

cuviper commented Sep 24, 2024

Uh oh!

bors commented Sep 24, 2024

Uh oh!

bors commented Sep 24, 2024

Uh oh!

bors commented Sep 24, 2024

Uh oh!

rust-timer commented Sep 24, 2024

Overall result: ❌ regressions - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Uh oh!

RalfJung Sep 25, 2024

Choose a reason for hiding this comment

Uh oh!

Voultapher Sep 25, 2024

Choose a reason for hiding this comment

Uh oh!

Voultapher commented Sep 25, 2024

Uh oh!

Kobzol commented Oct 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

Voultapher commented Aug 29, 2024 •

edited

Loading