[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]

I'll r+ it again if you can get rid of the merge commit, please :)

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[marcpems]

ok that should be it

[Kobzol]

Thanks.

@bors r+

[rust-bors]

📌 Commit 66162cb has been approved by Kobzol

It is now in the queue for this repository.

[Zalathar]

@bors yield (enclosing rollup)

[rust-bors]

Auto build was cancelled. Cancelled workflows:

• https://github.com/rust-lang/rust/actions/runs/43350524213

The next pull request likely to be tested is #165897.

[rust-bors]

☀️ Test successful - CI
Approved by: Kobzol
Duration: 3h 22m 54s
Pushing 75bb7a3 to main...

[github-actions]

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing 8eb778d (parent) -> 75bb7a3 (this PR)

Test differences

Show 2 test diffs

2 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 75bb7a374ff3f5363432a17e8000b50a60a4530c --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

1. dist-i686-msvc: 32m 24s -> 50m 15s (+55.1%)
2. dist-riscv64-linux-musl: 1h 2m -> 1h 32m (+48.2%)
3. test-i686-gnu-nopt-2: 1h 39m -> 2h 27m (+48.1%)
4. dist-arm-linux-musl: 1h 16m -> 1h 53m (+48.0%)
5. test-pr-check-1: 39m 26s -> 20m 57s (-46.9%)
6. test-x86_64-msvc-2: 2h 48m -> 1h 30m (-46.1%)
7. test-x86_64-gnu-llvm-22-1: 1h 20m -> 46m 45s (-41.9%)
8. test-i686-gnu-nopt-1: 2h 31m -> 1h 28m (-41.7%)
9. test-x86_64-mingw-2: 2h 54m -> 1h 41m (-41.6%)
10. test-armhf-gnu: 1h 46m -> 1h 2m (-41.6%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

[rust-timer]

Finished benchmarking commit (75bb7a3): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

Results (primary 1.8%)

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

	mean	range	count
Regressions ❌ (primary)	1.8%	[1.8%, 1.8%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.8%	[1.8%, 1.8%]	1

Cycles

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

Binary size

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

Bootstrap: 488.192s -> 483.141s (-1.03%)
Artifact size: 406.14 MiB -> 406.07 MiB (-0.02%)