Update logic around PGO staging #101744

chriswailes · 2022-09-13T00:41:17Z

PGO instrumentation occurs fairly late in the code generation pipeline after many transformations and optimizations are applied. This means that profiles for the same source code can differ if the binary that generated them was compiled with a different compiler or options.

When bootstrapping the Rust compiler this is relevant because it means that profiles that are generated by the Stage 1 compiler may have entries with checksums that don't match the functions in the Stage 2 compiler. This isn't actively harmful, but it may prevent some functions in later stage compilers from receiving profile-guided optimizations.

This commit changes the build system to instrument the Stage 2 and 3 compilers (with one exception discussed below) and to use the provided profile to optimize the compiler at every stage.

Not instrumenting the Stage 1 compiler has two advantages:

It decreases the size of the eventual profile (as there currently isn't a way to provide a separate profile path name to each stage)
It speeds up the bootstrap process; instrumented binaries are slower and if we aren't going to use the profile from the Stage 1 compiler we might as well not generate it

Because the LLVM libraries are compiled only once if they are PGO instrumented then the compiler must be PGO instrumented at every stage, including Stage 1.

CI definitions will need to be modified to use the Stage 2 compiler when generating PGO profiles.

rust-highfive · 2022-09-13T00:41:21Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jyn514 (or someone else) soon.

Please see the contribution instructions for more information.

jyn514

Overall your description makes sense to me, except this bit:

Because the LLVM libraries are compiled only once if they are PGO instrumented then the compiler must be PGO instrumented at every stage, including Stage 1.

Why does the compiler need to be instrumented if LLVM is instrumented? Isn't instrumenting stage 2 enough?

CI definitions will need to be modified to use the Stage 2 compiler when generating PGO profiles.

Can you please do that as part of this pr? I think src/ci/pgo.sh is the place to start looking.

src/bootstrap/compile.rs

jyn514 · 2022-09-14T02:01:40Z

src/bootstrap/compile.rs

@@ -678,13 +678,9 @@ impl Step for Rustc {
                false
            }
        } else if let Some(path) = &builder.config.rust_profile_use {
-            if compiler.stage == 1 {


why was this condition removed? Shouldn't it be the same as above, !link_shared() || stage >= 2?

If a profile is provided there is no harm in using it. If there are checksum missmatches or missing functions then we have the same result as not using one, but if there are signature matches then the Stage1 compiler will be faster and the total build should complete sooner.

Mark-Simulacrum · 2022-09-14T02:25:54Z

Please make sure we run perf to ensure equivalency of results before merging.

chriswailes · 2022-09-20T00:32:48Z

Sorry for taking so long with my response but I wanted to do some additional investigations to see if I could find an alternative way of addressing the issue below.

Overall your description makes sense to me, except this bit:

Because the LLVM libraries are compiled only once if they are PGO instrumented then the compiler must be PGO instrumented at every stage, including Stage 1.

Why does the compiler need to be instrumented if LLVM is instrumented? Isn't instrumenting stage 2 enough?

If the LLVM library is instrumented it needs to link against support libraries for functions related to writing the profile data. When LLVM is linked dynamically that isn't a problem as the profile related symbols can be resolved dynamically as well. However, when LLVM is instrumented and linked statically those symbols need to be resolved at link time.

I have been unable to find a way to get an instrumented libLLVM.a to link against an un-instrumented build of rustc. With the only other alternative being to build a second, uninstrumented, copy of LLVM to link against an un-instrumented Stage1 compiler, building Stage1 with instrumentation as well in this particular case seems like the better choice.

Can you please do that as part of this pr? I think src/ci/pgo.sh is the place to start looking.

Will do.

chriswailes · 2022-09-20T00:54:00Z

Regarding my previous comment, I now realize that several of my points rely on details of how we use the build system and not the build system itself. The bootstrap system doesn't provide functionality for PGO instrumenting the LLVM build and we achieve this by passing LLVM the appropriate flags, so it's entirely possible for others to build an instrumented Rust toolchain with static linkage against a non-instrumented LLVM library.

I think the change is still worthwhile though because with it developers building the toolchain without a PGO instrumented LLVM will get slightly slower builds while without the change those building with PGO instrumented LLVM will have to make a similar change to the bootstrapy system. Providing the profile arguments via RUSTFLAGS won't work as that would use the same profile arguments for all Rust objects, not just the compiler.

chriswailes · 2022-09-26T23:10:02Z

Can you please do that as part of this pr? I think src/ci/pgo.sh is the place to start looking.

I looked at src/ci/pgo.sh and it looks like it's already building the Stage2 compiler. This makes me wonder if the CI is accidentally only collecting the LLVM profiles and not Rust profiles.

It looks like no changes need to be made?

chriswailes · 2022-09-26T23:12:15Z

Please make sure we run perf to ensure equivalency of results before merging.

What's the easiest way to invoke the PGO pipeline locally?

jyn514 · 2022-09-26T23:33:13Z

@bors try @rust-timer queue

rust-timer · 2022-09-26T23:33:14Z

Awaiting bors try build completion.

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

bors · 2022-09-26T23:33:21Z

⌛ Trying commit 2071506c1d60d52e72539f183b819f8554023c20 with merge 591c425928b66f3e73ef8a840c109792df09441a...

bors · 2022-09-27T00:23:17Z

💔 Test failed - checks-actions

jyn514 · 2022-09-27T00:37:19Z

+ cd /checkout/obj
+ RUSTC_PROFILE_MERGED_FILE=/tmp/tmp-pgo/rustc-pgo.profdata
+ /checkout/obj/build/x86_64-unknown-linux-gnu/llvm/bin/llvm-profdata merge -o /tmp/tmp-pgo/rustc-pgo.profdata /tmp/tmp-pgo/rustc-pgo
error: /tmp/tmp-pgo/rustc-pgo: No such file or directory

Kobzol · 2022-09-27T05:56:56Z

@chriswailes You can run

./src/ci/docker/run.sh dist-x86_64-linux

locally to run the dist CI pipeline locally.

On CI, we gather both LLVM and Rust PGO profiles. It's done separately in two runs to avoid the profiling runtimes to clash. I want to revisit this decision soon to see if it's still needed. Removing this two stage approach would shorten dist/perf CI times.

Kobzol · 2022-09-27T08:23:55Z

I wonder what exactly is this PR trying to achieve? 🤔 Currently we only instrument stage 2 compilers with PGO.
The condition

if compiler.stage == 1

means that we only instrument stage 2, since the compiler here is the compiling compiler, not the compiler being compiled.

JohnCSimon · 2022-11-06T02:35:09Z

ping from triage:
@chriswailes

Can you please post your status on this PR?

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

rustbot · 2022-11-16T18:15:59Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

Some changes occurred in src/tools/cargo

cc @ehuss

chriswailes · 2022-11-16T18:20:02Z

Sorry for the delay (I got distracted by other projects) and thanks for the additional context on the compiler object. I've updated the CL to remove that bit as well as add additional config.toml support for PGO configuration options.

rust-timer · 2022-11-17T02:11:24Z

Awaiting bors try build completion.

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

bors · 2022-11-17T02:11:31Z

⌛ Trying commit 257ec3b with merge fa338c9ddc5996f71f5ee1cfd9b6bbc5d5318bf4...

jyn514

How have you tested this change?

jyn514 · 2022-11-17T02:12:31Z

src/bootstrap/compile.rs

-            if compiler.stage == 1 {
-                cargo.rustflag(&format!("-Cprofile-use={}", path));
-                cargo.rustflag("-Cllvm-args=-pgo-warn-missing-function");
+        } else if let Some(path) = &builder.config.llvm_profile_generate {


why are rust_profile_generate and llvm_profile_generate mutually exclusive?

From my understanding this function is only responsible for building Rust crates, not the LLVM libraries. If llvm_profile_generate is set, the code that handles building LLVM will take care of that and then this code will compile the crates appropriately.

This code now makes the original check to see if Rust instrumentation was requested. If not, it will still instrument the Rust crates if the LLVM libraries are instrumented and linkage is set to static, as a way of ensuring that the necessary runtime libraries are present.

bors · 2022-11-17T02:15:34Z

💔 Test failed - checks-actions

rust-log-analyzer · 2022-11-17T07:25:13Z

The job dist-x86_64-linux failed! Check out the build log: (web) (plain)

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

+ python3 /checkout/x.py build --target=x86_64-unknown-linux-gnu --host=x86_64-unknown-linux-gnu --stage 2 library/std --llvm-profile-generate=/tmp/tmp-pgo/llvm-pgo/prof-%p
Building rustbuild
    Finished dev [unoptimized] target(s) in 0.07s

Option 'llvm-profile-generate' does not take an argument
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `1`,
  left: `1`,
 right: `2`: x.py should be run with `--stage 2` on CI, but was run with `--stage 1`', config.rs:1381:21
Build completed unsuccessfully in 0:00:00

chriswailes · 2022-12-01T22:45:46Z

After some experimentation I've confirmed that when compiling both LLVM and the Rust toolchain with PGO instrumentation and static LLVM linkage the build will fail if two different profile paths are used. Do people have a preference on how this is handled by the bootstrap system? Should an error be produced if the provided profile paths differ? Should the compiler silently (or with a warning) use Rust's profile path?

Mark-Simulacrum · 2022-12-02T00:12:41Z

Do we have some idea why it fails? Is this a bug that should be filed with LLVM, for example?

chriswailes · 2022-12-02T19:29:33Z

I don't think it's a bug. I believe it's due to conflicting entries in the two different profiles. I'll reproduce the exact error and post it here.

Mark-Simulacrum · 2022-12-02T21:18:17Z

I guess what it sounds like to me is that when we share a common profile directory, it's likely that we're just clobbering one or both of the pgo data collections which "fixes" the problem but gives us worse performance (less benefit).

I recall that we landed a PR a while back which stopped sharing a profile directory intentionally and it was a significant (several percent) win in terms of performance.

I wonder if it makes sense to "allow" static linkage during pgo collection - presumably we might be able to only do it for the final artifacts produced?

chriswailes · 2022-12-08T23:36:21Z

Looking at the profile here I'm seeing both LLVM and Rust symbols, so I don't think the profiles are clobbering each other. Our entire PGO pipeline is defined here if you'd like to check. It's possible we're making other errors.

jyn514 · 2022-12-08T23:46:41Z

r? @Mark-Simulacrum

bors · 2022-12-10T14:23:07Z

☔ The latest upstream changes (presumably #105525) made this pull request unmergeable. Please resolve the merge conflicts.

Mark-Simulacrum · 2022-12-10T17:22:06Z

Hm, well, I can't say that just having both types of symbols is fully convincing that nothing is getting clobbered. But, I think our CI doesn't use static linkage for LLVM anywhere we do PGO, so it probably is fine to error on this case with a comment that it didn't work but may not be impossible to fix (or something to that effect).

chriswailes · 2022-12-12T18:41:16Z

I'd rather it not error in this configuration as it is one that we currently build and test on occasion. Would a warning be OK?

Mark-Simulacrum · 2022-12-12T20:06:43Z

Hm, well, let's include an error on the specific scenario:

when compiling both LLVM and the Rust toolchain with PGO instrumentation and static LLVM linkage the build will fail if two different profile paths are used

But I'm okay leaving the maybe working scenario as a warning. (Even if it's likely to be useless in practice, because no one reads a full log of pgo output...)

Dylan-DPC · 2023-02-23T13:09:49Z

@chriswailes any updates on this?

Dylan-DPC · 2023-05-16T18:01:25Z

Closing this as inactive. Feel free to reöpen this pr or create a new pr if you get the time to work on this. Thanks

rust-highfive assigned jyn514 Sep 13, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 13, 2022

rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Sep 13, 2022

jyn514 suggested changes Sep 14, 2022

View reviewed changes

jyn514 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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 14, 2022

jyn514 added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 14, 2022

chriswailes force-pushed the pgo-staging branch from 9e82f4f to 2071506 Compare September 26, 2022 22:59

This comment has been minimized.

Sign in to view

chriswailes force-pushed the pgo-staging branch from 2071506 to 7874e91 Compare November 16, 2022 18:15

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 16, 2022

jyn514 reviewed Nov 17, 2022

View reviewed changes

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-review Status: Awaiting review from the assignee but also interested parties. labels Nov 17, 2022

rustbot assigned Mark-Simulacrum and unassigned jyn514 Dec 8, 2022

Mark-Simulacrum added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 10, 2022

Dylan-DPC closed this May 16, 2023

Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 16, 2023

Update logic around PGO staging #101744

Update logic around PGO staging #101744

Conversation

chriswailes commented Sep 13, 2022

rust-highfive commented Sep 13, 2022

jyn514 left a comment • edited Loading

Choose a reason for hiding this comment

jyn514 Sep 14, 2022

Choose a reason for hiding this comment

chriswailes Sep 20, 2022

Choose a reason for hiding this comment

Mark-Simulacrum commented Sep 14, 2022

chriswailes commented Sep 20, 2022

chriswailes commented Sep 20, 2022

chriswailes commented Sep 26, 2022

chriswailes commented Sep 26, 2022

jyn514 commented Sep 26, 2022

rust-timer commented Sep 26, 2022

bors commented Sep 26, 2022

bors commented Sep 27, 2022

jyn514 commented Sep 27, 2022

Kobzol commented Sep 27, 2022 • edited Loading

This comment has been minimized.

Kobzol commented Sep 27, 2022 • edited Loading

JohnCSimon commented Nov 6, 2022

rustbot commented Nov 16, 2022

chriswailes commented Nov 16, 2022

rust-timer commented Nov 17, 2022

bors commented Nov 17, 2022

jyn514 left a comment

Choose a reason for hiding this comment

jyn514 Nov 17, 2022 • edited Loading

Choose a reason for hiding this comment

chriswailes Nov 29, 2022

Choose a reason for hiding this comment

bors commented Nov 17, 2022

rust-log-analyzer commented Nov 17, 2022

chriswailes commented Dec 1, 2022

Mark-Simulacrum commented Dec 2, 2022

chriswailes commented Dec 2, 2022

Mark-Simulacrum commented Dec 2, 2022

chriswailes commented Dec 8, 2022

jyn514 commented Dec 8, 2022

bors commented Dec 10, 2022

Mark-Simulacrum commented Dec 10, 2022

chriswailes commented Dec 12, 2022

Mark-Simulacrum commented Dec 12, 2022

Dylan-DPC commented Feb 23, 2023

Dylan-DPC commented May 16, 2023

jyn514 left a comment •

edited

Loading

Kobzol commented Sep 27, 2022 •

edited

Loading

Kobzol commented Sep 27, 2022 •

edited

Loading

jyn514 Nov 17, 2022 •

edited

Loading