Don't merge CGUs in incremental builds.

[nnethercote]

r? @ghost

[nnethercote]

@bors try @rust-timer queue

[bors]

⌛ Trying commit 819bf71 with merge affbc221b471eba0070f640c6cecc8701bb9d5fd...

[rust-log-analyzer]

The job x86_64-gnu-llvm-14 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

failures:

---- [codegen-units] tests/codegen-units/partitioning/incremental-merging.rs stdout ----

The following items were assigned to wrong codegen units:

fn aaa::foo
  expected: incremental_merging-aaa--incremental_merging-bbb[External] 
  actual:   incremental_merging-aaa[External] 


fn bbb::foo
  expected: incremental_merging-aaa--incremental_merging-bbb[External] 
  actual:   incremental_merging-bbb[External] 
thread '[codegen-units] tests/codegen-units/partitioning/incremental-merging.rs' panicked at 'explicit panic', src/tools/compiletest/src/runtest.rs:2905:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[bors]

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

[rust-timer]

Finished benchmarking commit (affbc221b471eba0070f640c6cecc8701bb9d5fd): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@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.6%	[0.2%, 1.7%]	15
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-26.6%	[-76.0%, -1.4%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-4.0%	[-76.0%, 1.7%]	18

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)	0.5%	[0.5%, 0.5%]	1
Regressions ❌ (secondary)	3.6%	[2.9%, 4.7%]	3
Improvements ✅ (primary)	-3.0%	[-5.5%, -0.4%]	3
Improvements ✅ (secondary)	-0.8%	[-0.8%, -0.8%]	1
All ❌✅ (primary)	-2.1%	[-5.5%, 0.5%]	4

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)	1.5%	[1.2%, 1.9%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-74.8%	[-74.8%, -74.8%]	1
Improvements ✅ (secondary)	-2.6%	[-2.9%, -2.2%]	2
All ❌✅ (primary)	-23.9%	[-74.8%, 1.9%]	3

Binary size

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)	1.7%	[0.0%, 7.0%]	24
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.7%	[0.0%, 7.0%]	24

Bootstrap: 648.127s -> 649.584s (0.22%)

[nnethercote]

The idea here is that the default number of CGUs for incremental builds is 256. This is a high number. Most benchmarks don't have that many CGUs. Only the biggest benchmarks do, which means they tend to do worse with incr-patched results, because CGU merging generally interferes with partial incremental rebuilds. So why not let those larger benchmarks have more CGUs?

The drastic results for incremental opt builds of hyper are because it sets codegen-units = 1 for release builds, which means incremental normally has almost no effect there, and this PR effectively overrides that. So those can be mostly ignored. (There's an argument that we shouldn't measure opt incremental builds in rustc-perf, because it's not a default configuration and probably very rarely used in the real world.)

The big swings for bitmaps are because it has become very noisy :(

The -4.17% walltime for cargo-0.60.0 debug incr-patched: println is the kind of thing I was expecting to see -- a benchmark with more than 256 CGUs seeing an improvement for an incr-patched run due to less CGU merging. But I thought some of the other big cases (such as webrender, cranelift-codegen, and ripgrep) would also see improvements.

A good comparison is this result in #111666 where I reduced the number of CGUs in incremental builds from 256 to 64, and there were some drastic incr-patched regressions on the larger benchmarks. That's the kind of regression this PR's changes would avoid. The incr-patched runs here are pretty basic and don't give much coverage, it's easy to imagine that they're just getting lucky, and that changes to other parts of the code could have larger effects with only 256 CGUs.

[bjorn3]

Currently -Ccodegen-units overrides the 256 default for incr comp. With this change that no longer happens. I think user's intent should be preserved even with incr comp. Maybe they care about runtime perf too and many codegen units is a huge regression (eg in a game) or maybe duplication of inlined functions costs more than not merging saves in reusable cgus. Either way the user should have the option to do what is best for them.

[nnethercote]

Sure, as written I've done this too crudely. If this were to land various other changes would be needed, e.g. documentation. I have changed the PR to draft status to better reflect this.

[michaelwoerister]

To second @bjorn3's comment: I think we should keep having a way for controlling the max number of CGUs for incremental builds. Making incr. comp. respect -Ccodegen-units was introduced as a feature at some point because it allows to keep resource usage in check (there's quite a bit of fixed per-CGU overhead, IIRC).

I'm not against doing experiments in this area, of course. I just want to make sure this is taking into account.

[nnethercote]

Respecting -Ccodegen-units is definitely a good idea. But do people think that defaulting to an unlimited number of CGUs is reasonable?

[michaelwoerister]

I'm not sure about unlimited¹ but I have no objection to increasing the default to a much higher number. If I remember correctly, the current default was introduced in the same PR that made incr. comp. take -Ccodegen-units into account and was basically calibrated towards the then current set of perf.rlo benchmarks. From my point of view, the important thing is that users have the ability to control the number of CGUs if the default doesn't work for them.

Footnotes

1. The initial set of CGUs is still 2 CGUs for each source-level module, right? I could imagine that some macros or other kind of code generation will create lots of small modules, so I'd be a bit worried about not setting an upper bound at all. The bound could be rather high (1000? 2000?) -- but I'd be worried about users discovering that their incr. cache directory is a 100Gb and contains 50000 object files and not quite knowing what to do about that. I think that would be worse than less effective incr. comp. for extremely large crates. ↩

[nnethercote]

Sounds like the simplest path forward would be to lift the limit from 256 CGUs to perhaps 512 or 1024. But now I'm not sure it's worth the effort.