[marcpems]

View all comments

Fix the sequence for LLVM’s PGO training with rust when LLVM is statically linked. Delivers ~14% compile time improvements for x64 build's (using rust v1.94.1 and IronRDP as a test project). Also (using LLM to generate) added a test validation step that checks the correct PGO training is taking place for each of the build flavours.
This does not impact builds for Linux +Mac targets as LLVM is not statically linked. Neither does it directly impact Windows on Arm builds as PGO is disabled on those productions.

Details:
When rustc and llvm are linked statically the opt-dist now rebuilds rustc with the existing rustc PGO profile while generating LLVM instrumentation build. When LLVM is a shared component the existing behaviour remains, avoiding the additional rustc build. Static linked llvm needs to be rebuilt to use the instrumented LLVM before a profile will be correctly generated.

The rest adds a regression test covering Linux and Windows hosts, both fast-try-build settings, and registers opt-dist in the normal bootstrap test suite. The snapshot updates reflect that new test-suite entry.

Impact on the release-build wall-clock time for x64 is expected to be around 5-10 mins, keeping it well within the limits for a standard runner.

UPDATE (23rd Oct)
I re-ran this using v1.99.1 to validate on latest. Applying this to 1.99.1 and using IronRDP again as a measure, the compilation wall time reduction was 12.9% and CPU time was down by 13.7%. Tests and additional correctness passed.
The cost of release-build was about the same (using a 16-core test machine)

LLM disclosure: GPT-6 Astra was used to...

• point out the absence of PGO page hits on release rustc builds
• identify the cause of the PGO misses
• suggest a new test scenario to validate
• run multiple perf comparison runs to validate impact and robustness of the fix.
  Full investigation results available here: https://github.com/marcpems/IronRDP-ci-benchmark/blob/experiment/x64-pgo-comparison/ci-benchmark/X64-PROFILE-ONLY.md

r? @Ciura
r? @kennykerr
r? @manbearian

[rustbot]

Error: Failed to set assignee to ciura: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

Failed to set assignee to kennykerr: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

Failed to set assignee to manbearian: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #triagebot on Zulip.

[rustbot]

Some changes occurred in src/tools/opt-dist

cc @Kobzol

[rustbot]

Error: Failed to set assignee to ciura: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #triagebot on Zulip.

[kennykerr]

Hey @wesleywiser @dpaoliello @ChrisDenton - could one of you take a look at this, or point it to the right reviewer?

I've been talking with Marcus, who works on the Windows team and has been digging into Rust and LLVM build performance on Windows. This PR comes out of that work. On Windows x64, LLVM is statically linked into rustc, and the step that collects LLVM's PGO profile never rebuilds rustc against the instrumented LLVM. As a result, the shipped LLVM is "PGO optimized" with an almost empty profile. Marcus measured about a 14% compile-time improvement on x64 with the fix.

Any help getting this reviewed would be much appreciated - it's a meaningful win for Rust on Windows, and there's more of this kind of work coming.

[mamoreau-devolutions]

👋 I just wanted to point out that the huge difference in Rust build performance was found through benchmarks I did on IronRDP, so I'm really looking forward to this big win for Windows Rust build performance!

[wesleywiser]

r? @Kobzol since you've been most heavily involved with PGO builds but anyone in @rust-lang/bootstrap could review 🙂

[rustbot]

Kobzol is not on the review rotation at the moment.
They may take a while to respond.

[wesleywiser]

Seems like there should also be a change to the workflows/actions configuration for x86_64-pc-windows-msvc to enable using opt-dist during builds right?

I think it would be helpful to push a change that enables this so we can do a try build and test this end-to-end even if we don't ultimately land that as part of this PR.

View changes since this review

[ChrisDenton]

Just for future reference, it's usually best not to use r? unless a reviewer told you to or you otherwise know which reviewer to select. An appropriate reviewer will be assigned automatically.

That said, I am enthusiastic about the work being done here!

[Kobzol]

Hmm, that is strange, I think that LLVM was linked statically on Windows for a long time (i.e. no change was made there), and we did Windows benchmarks when we switched to opt-dist to confirm that PGO was still applied. Either we missed that one half of PGO was not being done in those benchmarks, or something regressed in the meantime.

Regarding the fix, I suspect that the whole pipeline is kind of ingrained on the notion of using dynamically linked LLVM. I wonder if we should have a separate pipeline for statically linked LLVM, because even with your fix, the first PGO optimized rustc is built unnecessarily, as it is immediately deleted and rebuilt from scratch. Or maybe we could change the build order, and optimize LLVM first.

In any case, I'm fine with this as a hotfix, modulo some nits.

Let's remove the tests, I think that they are kind of useless, because they are not testing the thing that actually gets run in CI or in opt-dist (it is of course very hard to test this quickly).

Let's check if Linux perf is unaffected, and do a try build to provide artifacts for checking Windows perf. locally:

@bors try jobs=dist-x86_64-linux,dist-x86_64-msvc

Seems like there should also be a change to the workflows/actions configuration for x86_64-pc-windows-msvc to enable using opt-dist during builds right?

We are already doing that (and have been doing it for the past N years :) ).

View changes since this review

[rust-bors]

💔 Test for 4475788 failed: CI. Failed job:

• try - dist-x86_64-msvc (web logs, enhanced plaintext logs)

[marcpems]

Hey @Kobzol, can you kick the bot for the msvc run again please. I missed a one line for the dry-run that should be fixed now

[Kobzol]

@bors try jobs=dist-x86_64-linux,dist-x86_64-msvc

[rust-bors]

☀️ Try build successful (CI)
Build commit: 51a081a (51a081aa501de11aa3168f7a846d785b39fc540a)
Base parent: dc44e7f (dc44e7f2c436b6041742e138b9c2176f093196d5)

[Kobzol]

@rust-timer build 51a081a

[rust-timer]

Missing artifact for sha 51a081aa501de11aa3168f7a846d785b39fc540a (https://ci-artifacts.rust-lang.org/rustc-builds/51a081aa501de11aa3168f7a846d785b39fc540a/rustc-nightly-aarch64-unknown-linux-gnu.tar.xz); not built yet, try again later.

[Kobzol]

Hmm, crap.

@bors try jobs=dist-x86_64-linux,dist-x86_64-msvc,dist-aarch64-linux @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 51dae83 (51dae83933ec0602516d385a07cea7c692d15261)
Base parent: dc44e7f (dc44e7f2c436b6041742e138b9c2176f093196d5)

[rust-timer]

Finished benchmarking commit (51dae83): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

Results (primary -0.1%, secondary -0.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.6%	[0.5%, 0.8%]	4
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.6%	[-0.8%, -0.4%]	6
Improvements ✅ (secondary)	-0.6%	[-0.6%, -0.6%]	1
All ❌✅ (primary)	-0.1%	[-0.8%, 0.8%]	10

Cycles

Results (primary 0.2%, secondary 0.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.7%	[0.4%, 1.0%]	8
Regressions ❌ (secondary)	0.9%	[0.5%, 2.0%]	5
Improvements ✅ (primary)	-0.8%	[-1.1%, -0.5%]	4
Improvements ✅ (secondary)	-0.8%	[-1.5%, -0.4%]	4
All ❌✅ (primary)	0.2%	[-1.1%, 1.0%]	12

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 487.351s -> 488.714s (0.28%)
Artifact size: 405.87 MiB -> 405.93 MiB (0.01%)

[marcpems]

@Kobzol, @wesleywiser anything further needed here?

[Kobzol]

Can you please check if https://ci-artifacts.rust-lang.org/rustc-builds/51dae83933ec0602516d385a07cea7c692d15261/rust-nightly-x86_64-pc-windows-msvc.tar.xz has the expected performance win? You can also use https://crates.io/crates/rustup-toolchain-install-master and install the 51dae83933ec0602516d385a07cea7c692d15261 commit on Windows.

[marcpems]

@Kobzol
No, that build does not show the perf improvements.

sccache returned cache hits on pretty much everything in llvm. The only thing that changed was profdata and changes there didn't trigger a cache miss.
There is a reference to profdata in the cmake call but the sccache only includes the file name in the hash calculation, not the file content.

I raised issue 2964 for sccache (mozilla/sccache#2964) and provided a PR here mozilla/sccache#2962

There are two possible workarounds

• clear the cache and re-run, but I expect this will fail against the 360m limit on the standard runner
• add something like this to force the profdata into the hash:
  let extra_files = std::env::var_os("SCCACHE_EXTRAFILES");

self.cmd = self.cmd.env("SCCACHE_EXTRAFILES", std::env::join_paths(path)?);

The cleanest solution it to merge the upstream, adopt a new sccache here and have it just work, but maybe you have a preference?

[Kobzol]

I don't really understand what you are talking about, tbh. Are you saying that when we rebuild PGO optimized rustc for the second time, it gets cached and it doesn't relink to LLVM? What does that have to do with sccache's handling of -fprofile-instr-use?

[marcpems]

The cache hits happen during C/C++ compilation of LLVM in the final profile-use build, before linking. The stats in the job log (https://triage.rust-lang.org/gha-logs/rust-lang/rust/121229977135) cover all stages but show that stale cache hits are happening for 100% of all C++ and C files so the llvm source is not recompiled with the new profdata.

So we have the new profile now, but old cached objects are overriding the compile step because the profile content change is not causing a cache miss due to the issue in sccache, so the result is effectively the same binary produced.

From the build log:
##[group]sccache stats
...
Cache hits 10817
Cache hits (c [msvc]) 212
Cache hits (c++ [msvc]) 10341
Cache hits (rust) 264
Cache misses 88
Cache misses (rust) 88
Cache hits rate 99.19 %
Cache hits rate (c [msvc]) 100.00 %
Cache hits rate (c++ [msvc]) 100.00 %
Cache hits rate (rust) 75.00 %

I did a local test adding the profile file to SCCACHE_EXTRAFILES and it correctly causes a cache miss, and then perf improvement is seen when using the produced binaries.

So options are either forcing the profdata file content into the cache decision by using SCCACHE_EXTRAFILES, causing a full LLVM cache refresh (I'm not sure how or if the build machine can cope), or wait for the sccache fix.
I will prep a PR for the SCCACHE_EXTRAFILES change as well.

LMK which way you want to go.
@Kobzol

[marcpems]

I made a separate PR for the cache issue workaround: #165138
This PR I kept the tests as the cache handling is a little more complex. I can remove again if that's appropriate.

@Kobzol

[Kobzol]

I still have no idea what do you mean. We only use/call llvm_pgo_optimize once in the build pipeline when BOLT is disabled. No previous -fprofile-use should be set when building LLVM itself. How could that cause repeated sccache hits? Are you saying that sccache on Windows/MSVC completely ignores -fprofile-use, and treats it as if the flag wasn't passed?

[marcpems]

@Kobzal yes sccache on Windows/MSVC ignores the contents of the profdata files, and just looks at changes in the file name when creating and comparing cache hash values. So if we changed the name of the data file it would cause a cache miss, but changing the contents of the profdata wont. So recompilation with profile data optimization changes alone skips the actual recompile and we get cached binaries with no PGO perf improvement.

The PR I raised for sccache fixes the busted parameter but will require merge + update of Rust to that new version.

The other Rust PR uses the SCCACHE_EXTRAFILES environment variable to force the profdata files into the cache check step.

I propose to merge the two Rust PRs and run a test again to validate the cache miss occurs and the perf improvement is seen.

[Kobzol]

@Kobzal yes sccache on Windows/MSVC ignores the contents of the profdata files, and just looks at changes in the file name when creating and comparing cache hash values. So if we changed the name of the data file it would cause a cache miss, but changing the contents of the profdata wont. So recompilation with profile data optimization changes alone skips the actual recompile and we get cached binaries with no PGO perf improvement.

I understand the problem that you are describing. What I don't understand is how we trigger that problem.

As far as I am aware, we should be compiling LLVM:

• Without any PGO flags (once)
• With PGO instrumentation (once)
• With PGO optimization (once)

So I don't see where does sccache come in. Where does the first (and second) compilation of a PGO-optimized LLVM happen in opt-dist when static linking is used, which could trigger the bug that you are describing?

[marcpems]

steps 1 & 2 (the first two build steps in your list) are not relevant for this issue - these together produce the new profdata we want for the final step. The cache already has profile-optimized binaries from an old run so when the third step - recompile with PGO optimized - is call with the same pathname + new profdata, the cache is hit and the old binaries are returned skipping the optimized build completely.

That's the issue, we need to fix the cache to include the profdata file content (as was intended) in the cache hash generation to cause a cache miss. I guess over time, as source files change the optimizations will get picked up for every file that has a cache miss.
Also, the earlier compile steps produce a different cache key (hash) because the compilation parameters differ specifically by the PGO use flag, so these are independent cache sets.

A quick fix might be to just clear the cache (not sure how to do that), but then this issue may come back if any future change to how the profdata is generated that only changed the data content.
@Kobzol