Tweak CGU sorting in a couple of places. #113872

nnethercote · 2023-07-20T00:07:33Z

In base.rs, tweak how the CGU size interleaving works. Since #113777, it's much more common to have multiple CGUs with identical sizes. With the existing code these same-sized items ended up in the opposite-to-desired order due to the stable sorting. The code now starts with a reverse sort (like is done in partitioning.rs) which gives the behaviour we want. This doesn't matter much for perf, but makes profiles in samply look more like what we expect.

In partitioning.rs, we can use sort_by_key instead of sort_by_cached_key because CGU::size_estimate() is cheap. (There is an identical CGU sort earlier in that function that already uses sort_by_key.)

r? @pnkfelix

In `base.rs`, tweak how the CGU size interleaving works. Since rust-lang#113777, it's much more common to have multiple CGUs with identical sizes. With the existing code these same-sized items ended up in the opposite-to-desired order due to the stable sorting. The code now starts with a reverse sort (like is done in `partitioning.rs`) which gives the behaviour we want. This doesn't matter much for perf, but makes profiles in `samply` look more like what we expect. In `partitioning.rs`, we can use `sort_by_key` instead of `sort_by_cached_key` because `CGU::size_estimate()` is cheap. (There is an identical CGU sort earlier in that function that already uses `sort_by_key`.)

nnethercote · 2023-07-20T00:12:44Z

The base.rs change is a bit obscure. Here's what a samply profile is supposed to look like:

Here's what we were getting:

(Ignore the vertical black line in each screenshot, it's meaningless.)

CGUs 08 and 09 have the same size, and CGUs 10-14 also all have the same size, and you can see that the CGU ordering for those groups is the opposite of what it should be.

This wasn't a problem prior to #113777, because the merging was worse and identically-sized CGUs were rare. But with better merging, identically-sized CGUs are more common.

nnethercote · 2023-07-20T00:13:00Z

I don't expect any perf changes, but just in case:

@bors try @rust-timer queue

bors · 2023-07-20T00:13:08Z

⌛ Trying commit 8c31219 with merge 07e48462bce954c97552b30b9317a151b397ab48...

bors · 2023-07-20T01:29:12Z

☀️ Try build successful - checks-actions
Build commit: 07e48462bce954c97552b30b9317a151b397ab48 (07e48462bce954c97552b30b9317a151b397ab48)

rust-timer · 2023-07-20T02:45:44Z

Finished benchmarking commit (07e48462bce954c97552b30b9317a151b397ab48): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

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
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.3%	[-0.3%, -0.3%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.3%	[-0.3%, -0.3%]	1

Max RSS (memory usage)

Results

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)	6.1%	[5.7%, 6.6%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.0%	[-3.3%, -0.9%]	6
Improvements ✅ (secondary)	-1.1%	[-1.1%, -1.1%]	1
All ❌✅ (primary)	0.0%	[-3.3%, 6.6%]	8

Cycles

Results

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)	-	-	0
Improvements ✅ (primary)	-3.1%	[-4.9%, -1.3%]	10
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-3.1%	[-4.9%, -1.3%]	10

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 648.656s -> 645.616s (-0.47%)

nnethercote · 2023-07-20T04:27:08Z

Performance effects are somewhere between neutral and a miniscule improvement.

pnkfelix · 2023-07-26T00:12:52Z

@bors r+ rollup

bors · 2023-07-26T00:12:54Z

📌 Commit 8c31219 has been approved by pnkfelix

It is now in the queue for this repository.

…nkfelix Tweak CGU sorting in a couple of places. In `base.rs`, tweak how the CGU size interleaving works. Since rust-lang#113777, it's much more common to have multiple CGUs with identical sizes. With the existing code these same-sized items ended up in the opposite-to-desired order due to the stable sorting. The code now starts with a reverse sort (like is done in `partitioning.rs`) which gives the behaviour we want. This doesn't matter much for perf, but makes profiles in `samply` look more like what we expect. In `partitioning.rs`, we can use `sort_by_key` instead of `sort_by_cached_key` because `CGU::size_estimate()` is cheap. (There is an identical CGU sort earlier in that function that already uses `sort_by_key`.) r? `@pnkfelix`

…nkfelix Tweak CGU sorting in a couple of places. In `base.rs`, tweak how the CGU size interleaving works. Since rust-lang#113777, it's much more common to have multiple CGUs with identical sizes. With the existing code these same-sized items ended up in the opposite-to-desired order due to the stable sorting. The code now starts with a reverse sort (like is done in `partitioning.rs`) which gives the behaviour we want. This doesn't matter much for perf, but makes profiles in `samply` look more like what we expect. In `partitioning.rs`, we can use `sort_by_key` instead of `sort_by_cached_key` because `CGU::size_estimate()` is cheap. (There is an identical CGU sort earlier in that function that already uses `sort_by_key`.) r? ``@pnkfelix``

…nkfelix Tweak CGU sorting in a couple of places. In `base.rs`, tweak how the CGU size interleaving works. Since rust-lang#113777, it's much more common to have multiple CGUs with identical sizes. With the existing code these same-sized items ended up in the opposite-to-desired order due to the stable sorting. The code now starts with a reverse sort (like is done in `partitioning.rs`) which gives the behaviour we want. This doesn't matter much for perf, but makes profiles in `samply` look more like what we expect. In `partitioning.rs`, we can use `sort_by_key` instead of `sort_by_cached_key` because `CGU::size_estimate()` is cheap. (There is an identical CGU sort earlier in that function that already uses `sort_by_key`.) r? ```@pnkfelix```

compiler-errors · 2023-07-26T15:16:47Z

possibly failed in rollup #114084: #114084 (comment) ?

not so sure

@bors r-

nnethercote · 2023-07-27T02:12:12Z

possibly failed in rollup #114084: #114084 (comment) ?

not so sure

@bors r-

This PR is really small; I've done much bigger similar changes recently without problem.
There are some warnings in the output relating to CGUs but they are only warnings, and likely pre-existing.
Print omitted frames count for short backtrace mode #112843 definitely caused errors in that rollup. I think this PR copped some auxiliary blame because of the warnings, which show up earlier in the output than the errors?

So I think it's worth trying this one again.

@bors r=pnkfelix

bors · 2023-07-27T02:12:13Z

📌 Commit 8c31219 has been approved by pnkfelix

It is now in the queue for this repository.

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#113872 (Tweak CGU sorting in a couple of places.) - rust-lang#114053 (CI: fix CMake installation for 32/64 bit `dist` Linux) - rust-lang#114075 (inline format!() args from rustc_codegen_llvm to the end (4)) - rust-lang#114081 (`desugar_doc_comments` cleanups) - rust-lang#114082 (add stable NullaryOp) - rust-lang#114098 (replace atty crate with std's IsTerminal) - rust-lang#114102 (Dont pass `-Zwrite-long-types-to-disk=no` for `ui-fulldeps --stage=1`) r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned pnkfelix Jul 20, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 20, 2023

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 20, 2023

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 20, 2023

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 26, 2023

workingjubilee mentioned this pull request Jul 26, 2023

Rollup of 4 pull requests #114083

Closed

matthiaskrgr mentioned this pull request Jul 26, 2023

Rollup of 5 pull requests #114084

Closed

workingjubilee mentioned this pull request Jul 26, 2023

Rollup of 5 pull requests #114085

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 26, 2023

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 27, 2023

matthiaskrgr mentioned this pull request Jul 27, 2023

Rollup of 7 pull requests #114116

Merged

bors merged commit c3cd051 into rust-lang:master Jul 27, 2023
11 checks passed

rustbot added this to the 1.73.0 milestone Jul 27, 2023

nnethercote deleted the tweak-cgu-sorting branch July 27, 2023 23:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tweak CGU sorting in a couple of places. #113872

Tweak CGU sorting in a couple of places. #113872

nnethercote commented Jul 20, 2023

nnethercote commented Jul 20, 2023 •

edited

Loading

nnethercote commented Jul 20, 2023

This comment has been minimized.

bors commented Jul 20, 2023

bors commented Jul 20, 2023

This comment has been minimized.

rust-timer commented Jul 20, 2023

nnethercote commented Jul 20, 2023

pnkfelix commented Jul 26, 2023

bors commented Jul 26, 2023

compiler-errors commented Jul 26, 2023

nnethercote commented Jul 27, 2023

bors commented Jul 27, 2023

Tweak CGU sorting in a couple of places. #113872

Tweak CGU sorting in a couple of places. #113872

Conversation

nnethercote commented Jul 20, 2023

nnethercote commented Jul 20, 2023 • edited Loading

nnethercote commented Jul 20, 2023

This comment has been minimized.

bors commented Jul 20, 2023

bors commented Jul 20, 2023

This comment has been minimized.

rust-timer commented Jul 20, 2023

Overall result: ✅ improvements - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

nnethercote commented Jul 20, 2023

pnkfelix commented Jul 26, 2023

bors commented Jul 26, 2023

compiler-errors commented Jul 26, 2023

nnethercote commented Jul 27, 2023

bors commented Jul 27, 2023

nnethercote commented Jul 20, 2023 •

edited

Loading