Stabilize -Zprofile-sample-use

[zamazan4ik]

View all comments

Tracking issue: #155668

Stabilization report

Summary

Sample Profile-Guided Optimization (Sample PGO or SPGO) is an alternative way to perform feedback-directed optimization (FDO). Rustc already supports Instrumented PGO (with the -Cprofile-generate / -Cprofile-use flags). Downside of the instrumented approach include that it requires a separate compilation and that the instrumentation has significant runtime overhead. SPGO instead uses the output of external profilers like perf during the compilation process to perform more aggressive compiler optimizations. This approach is described in this paper.

I propose stabilizing -Zprofile-sample-use as -Cprofile-sample-use

More information can be found in the updated by the PR "Profile-guided Optimization" guide or Clang PGO guide.

These flags are documented as:

• profile-sample-use:

This flag specifies the profiling data file to be used for sample-based
profile-guided optimization (SPGO). The flag takes a mandatory argument which
is the path to a valid .prof file. See the chapter on
[profile-guided optimization] for more information.
The -Zdebuginfo-for-profiling flag can be used to
improve the quality of the profiling data.

For more details about the flag and their usage - check the Profile-guided Optimization guide change.

What is stabilized

One compiler flag: -Zprofile-sample-use.

What isn't stabilized

I think this is the right section to compare Sample-based PGO (SPGO) implementation in Rustc vs its "big brother" - Sample-based PGO in Clang.

Besides -Zprofile-sample-use in Rustc / -fprofile_sample_use in Clang and -Zdebug-info-for-profiling in Rustc (which we decided to not stabilize at the moment) / -fdebug-info-for-profiling, -fno-debug-info-for-profiling in Clang, Clang additionally supports the following SPGO-related switches:

• -fpseudo-probe-for-profiling, -fno-pseudo-probe-for-profiling flags. According to the Clang's PGO guide, this switch is optional for SPGO. This switch has originals from an extension of SPGO that is called "Context-sensitive Sample PGO with Pseudo-Instrumentation" or simply "CSSPGO". Here is original RFC for the thing, also I can link some LLVM commits/discussions about the topic. This flag is not required for regular SPGO - it's just an improvement idea over regular SPGO, and could be added later to the Rustc in a different process (initially to unstable, than later promoted to stable). But that's another story and we can consider it later.
• -f[no-]unique-internal-linkage-names switch is also mentioned in the Clang PGO guide. I don't think that the switch is applicable to Rustc. Correct me if I am wrong pls.
• -fsample-profile-use-profi, -fno-sample-profile-use-profi switch. This switch is also marked as optional in the Clang PGO guide. This switch is an attempt to improve some inaccuracies in SPGO profile with some heuristics. SPGO in Rustc can be easily stabilized without this flag, since it's just a non-critical for regular SPGO usage heuristic. If we decide to add support for this switch to Rustc too - we can do in a separate activity without blocking with stabilization process. Check Introducing Profi into Rustc for better Sample PGO quality #156898 for more details.
• -fprofile-sample-accurate, -fauto-profile-accurate, -fno-profile-sample-accurate switch. This switch is not mentioned even by the Clang PGO guide :) This flag resolves this issue/feature request from LLVM upstream in SPGO use case. According to the description from Clang: "Specifies that the sample profile is accurate. If the sample profile is accurate, callsites without profile samples are marked as cold. Otherwise, treat callsites without profile samples as if we have no profile". Since we don't specify the flag in Rustc, branches without profile samples are now considered as branches without a profile and optimized as regular release code. Having support for this in Rustc would be definitely a nice addition to be on par with Clang, since there are good use cases for that. But I do not think that this could be a blocker for stabilization of SPGO in Rustc without this functionality - we can add it later. As a proof that SPGO in Rustc works completely okay without it you can use Rust-for-Linux bench results with SPGO via AutoFDO.

I am not aware about any other SPGO-related flags in Clang.

We definitely have a gap in SPGO-related flags in Rustc compared to Clang, but none of these gaps is a blocker for stabilization -Zprofile-sample-use right now. However, it would be nice to resolve these gaps later: add them in unstable form, test, and later stabilize them to be on par with Clang from SPGO optimization perspective. Right now all the flags above are missing in Rustc even in the unstable form. Stabilization of -Zprofile-sample-use does not prevent adding all missing SPGO related features later.

Design

RFC history

No RFC was created for these options. All original discussions for Unstable were done in the original Unstable PR: #87918

Post-RFC changes

What other user-visible changes have occurred since the RFC was accepted? Describe both changes that the lang team accepted (and link to those decisions) as well as changes that are being presented to the team for the first time in this stabilization report.

Compared to the unstable (RFC-like) state, I've made the following changes:

• I extended the Rustc's PGO guide with Sample-based PGO information and instructions, how to use it. This change will resolve Extend Profile-Guided Optimization guide with Sampling PGO #117023 . My changes are highly-inspired / cautiously copy-pasted (only needed parts) from the Clang guide. From licensing perspective it should be fine. If it's a problem in any way - I can do some rewording (but I would like to avoid such things). I decided to do some documentation copy due to Sample-based PGO incompatibilities between Clang and Rustc (Clang supports more options at very least). I believe that current version is more user-friendly and easier to use, compared to just referring to the Clang PGO guide.
• I changed help message and corresponding documentation for -Zdebug_info_for_profiling switch to be in the same way as Clang already has. It's kinda difficult to describe clearly, what the option does without exposing too much LLVM details - that's why I linked the documentation to the corresponding LLVM pass in the Reference for this option.

Key points

No arguments were raised during stabilization discussion of the feature in any place yet, including Zulip discussion: #t-compiler > Stabilizing Sample PGO (SPGO): `-Zprofile-sample-use`

Nightly extensions

I am not aware of any other unstable SPGO-related switches.

Doors closed

What doors does this stabilization close for later changes to the language? E.g., does this stabilization make any other RFCs, lang experiments, or known in-flight proposals more difficult or impossible to do later?

• Removing Sample-based PGO support from Rustc will be harder. But the technology itself is used on large scales in other ecosystems like Clang (heavily-used in big tech companies internally), and Rust-for-Linux already started to use it even with Rustc. I don't think will be a need to remove it in near future.
• Renaming flags will be harder. But current naming is done to be consistent with Clang. Clang proved robustness of this naming, so it shouldn't be a concern either.

No other proposals/experiments/etc. are affected.

Feedback

Call for testing

Call for testing wasn't done - it was slightly discussed #t-compiler > Stabilizing Sample PGO (SPGO): `-Zprofile-sample-use` @ 💬. However, no negative feedback was received for stabilization of this feature.

Right now this feature is already tested personally by me (local experiments with assembly changes verification before/after applying SPGO on an Intel-based (with LBR) Linux machine in some sample apps with llvm-profgen tool and by Rust-for-Linux project in this patch. Additionally, this feature was in unstable state for 5 years (since 2021) with no concerns (due to no bugs or no users - who knows. At least it was implemented for a reason 5 years ).

I think that's enough verification for such kind of feature.

Nightly use

The only publicly-known user of this feature is Rust-for-Linux project (see this commit). Besides that, no other users were found on GitHub via GitHub search for "-Zprofile-sample-use" query: almost all found entries are various copies of the Unstable book with the documentation for the option, and other places are related to the Linux kernel. Two found issues are related to the tracking issue of these two flags, and corresponding tracking issue in Rust-for-Linux.

I am not personally aware of any closed-source users of this feature. Probably Google (Sampled-based PGO biggest user at least for C++) and other big techs use it somewhere internally too but it's just a guess.

Implementation

Major parts

• Enable AutoFDO. #87918 - unstable original implementation
• Stabilize -Zprofile-sample-use #155942 - this PR, stabilization with some minor changes

No significant developments on the Rustc side - just propagating in a proper way SPGO profile to the LLVM part of the compiler.

Coverage

• For -Cprofile-sample-use we have only UI tests. We haven't implemented e2e tests since it's will be hard to add them to the current test suite (see the comment)

Breaking changes

No breaking changes are expected from this stabilization.

History

• Enable AutoFDO. #87918 - initial implementation
• Rename -Zdebuginfo-for-profiling switch #156887 - renaming -Zdebug_info_for_profiling into -Zdebuginfo_for_profiling + adding more tests for the feature (left for the history)
• Tracking Issue for profile-sample-use and debug-info-for-profiling #155668 - tracking issue

Acknowledgments

• Michael Benfield - author of the original PR for Unstable
• Jakub Beránek - the "PGO guy" in Rustc and cargo-pgo author
• Miguel Ojeda - Rust-for-Linux maintainer (they already use Sample-based PGO in Rust-for-Linux)
• Alexander Zaitsev - me

I am not aware of any person, who is against stabilization of these two flags.

Open items

I am not aware of any open issue, that is a blocker for stabilization of this feature.

List of SPGO-related things, which are not stabilization blockers in my opinion:

• Include llvm-profgen in llvm-tools component #155525 - this could improve SPGO UX with Rustc, but it's not a strict requirement - an externally-installed llvm-profgen can be used instead (I've tested it locally by using llvm-profgen-21 for SPGO with Rustc 1.95, which is LLVM 22-based

[rustbot]

r? @JohnTitor

rustbot has assigned @JohnTitor.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: @ehuss, compiler
• @ehuss, compiler expanded to 74 candidates
• Random selection from 22 candidates

[ojeda]

This change shall be adopted by the Rust-for-Linux dev team (if my change will be accepted), but they will need to do some changes anyway due to stabilization - replace -Z... with -C (see current patch) - so it shouldn't be an issue at all.

That is fine, yes -- we are accustomed to flag name changes on stabilization etc. :)

Rust-for-Linux already started to use it even with Rustc.

(...)

The only publicly-known user of this feature is Rust-for-Linux project (see this commit).

Just a quick clarification: it is not a commit yet. While we plan to land it during this cycle, i.e. for v7.2, it has not seen use in mainline Linux yet.

Thanks for this!

@rustbot label +A-rust-for-linux

[JohnTitor]

@rustbot reroll

[folkertdev]

So, this flag has no testing whatsoever? Having a run-make test seems like a good idea?

Also could you update the stabilization report with some of the content of the documentation you updated, so that it actually describes what these flags do?

Also the reference https://doc.rust-lang.org/reference/ is not relevant here, it does not document rustc flags.

View changes since this review

[zamazan4ik]

So, this flag has no testing whatsoever? Having a run-make test seems like a good idea?

Yep, you are right - just didn't know which test suite is a good idea to run. I've just ran ./x test tests/run-make - all the tests completed successfully. Added a note about this to the stabilization report.

Also could you update the stabilization report with some of the content of the documentation you updated, so that it actually describes what these flags do?

Added to the stabilization report into the Summary section.

Also the reference https://doc.rust-lang.org/reference/ is not relevant here, it does not document rustc flags.

Got it - removed the Reference mention from the stabilization report.

[ojeda]

Yep, you are right - just didn't know which test suite is a good idea to run. I've just ran ./x test tests/run-make - all the tests completed successfully. Added a note about this to the stabilization report.

I think @folkertdev was trying to say that we should add tests for the flags in the PR, i.e. not that we should run the existing tests (which will be done anyway).

Ideally, depending on what can be tested, there should be UI, LLVM and assembly tests, and I imagine @folkertdev was mentioning run-make ones to e.g. check the .prof file etc.

[folkertdev]

I kind of think the only relevant test here is that it works end-to-end, i.e. use run-make to actually use this feature by simulating a user interaction. Some files that might have useful bits of code to use:

• https://github.com/rust-lang/rust/blob/main/tests/run-make/cross-lang-lto-pgo-smoketest-clang/rmake.rs
• https://github.com/rust-lang/rust/blob/main/tests/run-make/pgo-branch-weights/rmake.rs
• https://github.com/rust-lang/rust/blob/main/tests/run-make/pgo-use/rmake.rs

[ojeda]

The DWARF discriminators being added is something that would be good to test in an LLVM test. Similarly, the .loc .. discriminator in the assembly one, no? These sorts of tests are important to verify we are actually doing something expected at each level. We do the same kind of thing for other "similar" flags (e.g. 2d47622).

(For UI, perhaps the error emitted about mixing the flags, passing a wrong file and things like that. But I am not sure what is usually done or not done for those.)

[folkertdev]

Fair enough, we can test that LLVM does what it promises too. If there are interaction with other flags then that should be tested as well.

For future reference: the stabilization report should contain information like this. Like, what even is this feature, and how do we validate that it works.

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[zamazan4ik]

Thanks for the clarification! I need some time to prepare all the requested changes about providing proper tests and moving flag rename to a dedicated PR.

[zamazan4ik]

error: 1 diagnostics reported in JSON output but not expected in test file

To be honest, I don't understand what does it mean. I generated the error message with --bless option and added it, but the tests are failing anyway with this error. Would be nice if anyone can explain me, what I am doing wrong here

[rust-rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rust-rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

[folkertdev]

We finally made it. I've found some final small things. Can you also rebase this on current main to see if CI hits anything?

View changes since this review

[folkertdev]

This isn't really part of the stabilization. Both the old and new descriptions are kind of vague to me. Do you still want to keep the updated text? (fine by me if so)

[zamazan4ik]

Yeah, I would like to keep this small update in this PR even if it's not directly related for the stabilization. By this change, I wanted to keep the flag description in sync with the same flag for Clang - https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-fdebug-info-for-profiling . Since PGO (and especially Sampling PGO) coverage across the ecosystem is really small, I wanted to reduce any kind of difference between two compilers in a place where they describe the same thing - so less potential questions would be raised by people in the future.

Please don't get me wrong - I am also not that happy with this explanation. But I think it's better than we had before. I hope in the future we will try to get rid of this flag completely (and integrate into some debuginfo level by default, as was previously lightly discussed somewhere in Zulip) but it's not yet done.

[folkertdev]

these flag names come out of the blue. This paragraph above should introduce them

rustc supports profile guided optimization with two different kinds of
profiling.

[zamazan4ik]

Agree.

I think instead of repeating the flags for Instrumentation/Sampling PGO we can move this "Differences between Instrumentation and Sampling" after the "Profiling with Sampling" section and before "Further reading". In this case, the order will be kinda natural: at first we describe Instrumentation PGO (with corresponding compiler flags), then Sampling PGO (with its flags too - already included), and then highlight some differences between them.

What do you think about this approach? If yes, I'll do it and rebase the PR.

[folkertdev]

@rustbot author

[folkertdev]

@bors r+ rollup

[rust-bors]

📋 This PR cannot be approved because it currently has the following labels: needs-fcp, S-waiting-on-t-compiler.

[folkertdev]

Hmm shouldn't the bot remove that? anyhow

@bors r+ rollup

[rust-bors]

📋 This PR cannot be approved because it currently has the following label: S-waiting-on-t-compiler.

[folkertdev]

...

@bors r+ rollup

[rust-bors]

📌 Commit 01a5987 has been approved by folkertdev

It is now in the queue for this repository.