Support caching Clang invocations with -fprofile-use and -fprofile-instr-use. #952

michaelwoerister · 2021-02-11T10:10:51Z

This PR adds support for caching Clang invocations with -fprofile-use and -fprofile-instr-use, something that will be required for supporting PGOed Rust compiler builds.

The PR only implements support for Clang. I have not had time to look into how GCC works (although from a superficial glance it looks like it might be harder to find out which files to hash exactly). Adding support for Rust should be straightforward though.

I added some unit tests and verified that things work locally, but found it hard to write end-to-end tests because of Clang's PGO setup which requires the external llvm-profdata tool to generate the .profdata file. Let me know if this is something that is required.

In #941 I expressed the concern that hashing these rather big .profdata files would take a long time. In practice this does not seem to be true. In my local tests compiling LLVM with a full cache took about 50 seconds. Caching the file hashes (so that most of the hashing work could be skipped) only reduced the total build time by a couple of seconds or so.

r? @luser

luser

This LGTM. Just two small things I would change.

src/compiler/gcc.rs

michaelwoerister · 2021-03-02T14:42:05Z

Thanks for the review, @luser! I think I addressed your comments.

@glandium, would you maybe be able to spare some time for merging/reviewing this PR? The new functionality is a necessary prerequisite for distributing PGOed versions of rustc.

glandium

Can you also rebase on top of #971 after it's merged? Thank you. (BTW, did you intend your user email not to contain a TLD?)

glandium · 2021-03-10T06:33:11Z

src/compiler/gcc.rs

@@ -169,7 +176,7 @@ counted_array!(pub static ARGS: [ArgInfo<ArgData>; _] = [
    flag!("-fplugin=libcc1plugin", TooHardFlag),
    flag!("-fprofile-arcs", ProfileGenerate),
    flag!("-fprofile-generate", ProfileGenerate),
-    take_arg!("-fprofile-use", OsString, Concatenated, TooHard),
+    take_arg!("-fprofile-use", PathBuf, Concatenated('='), ProfileUse),


-fprofile-use can also be used without a value associated with it. It looks like that's true of -fprofile-instr-use too, and you don't handle those cases. There's also -fno-profile-instr-use, which you might as well add support for while you're here.

-fprofile-use can also be used without a value associated with it.

Indeed. But the documentation does not seem to state what the semantics of that are. I'll have to dig into the source code...

There's also -fno-profile-instr-use, which you might as well add support for while you're here.

I'll look into it.

It looks like -fprofile-use without argument will try to read from ./default.profdata -- and that the implementation here already handles this correctly. I'll add a unit test for that.

glandium · 2021-03-10T06:42:25Z

src/compiler/gcc.rs

@@ -275,6 +283,21 @@ where
                    OsString::from(arg.flag_str().expect("Compilation flag expected"));
            }
            Some(ProfileGenerate) => profile_generate = true,
+            Some(ProfileUse(path)) => {
+                if compiler_kind != CCompilerKind::Clang {
+                    cannot_cache!("-fprofile(-instr)-use caching only supported for Clang");


no need for the (-instr) part.

glandium · 2021-03-10T06:51:22Z

src/compiler/gcc.rs

@@ -275,6 +283,21 @@ where
                    OsString::from(arg.flag_str().expect("Compilation flag expected"));
            }
            Some(ProfileGenerate) => profile_generate = true,
+            Some(ProfileUse(path)) => {
+                if compiler_kind != CCompilerKind::Clang {


Another way you could do this, without adding the compiler kind, is to simply add -fprofile-use to the clang list of arguments (see args::test_multi_search), in which case it would probably be more appropriate to call this ClangProfileUse rather than ProfileUse.

Ah, you can "override" arguments that way? That does sound like the better approach for this case.

glandium · 2021-03-10T06:53:22Z

src/compiler/msvc.rs

@@ -587,7 +587,7 @@ pub fn parse_arguments(
            let mut args = match arg.get_data() {
                Some(SplitDwarf) | Some(TestCoverage) | Some(Coverage) | Some(DoCompilation)
                | Some(Language(_)) | Some(Output(_)) | Some(TooHardFlag) | Some(XClang(_))
-                | Some(TooHard(_)) => cannot_cache!(arg
+                | Some(TooHard(_)) | Some(ProfileUse(_)) => cannot_cache!(arg


it shouldn't be too hard to add support for clang-cl here.

michaelwoerister · 2021-03-10T14:08:00Z

Thanks for the review, @glandium!

Can you also rebase on top of #971 after it's merged? Thank you.

Will do.

(BTW, did you intend your user email not to contain a TLD?)

That was a halfhearted (and probably ineffective) attempt to keep my email address somewhat protected from spammers. It set up git like that years ago and it seems to have stuck :/

michaelwoerister · 2021-03-12T16:07:47Z

OK, I think the version I just pushed addresses your comments, @glandium. And it does look cleaner than the previous version :)

Note: I added "support" for the -fno-profile-* flags by explicitly marking them as TooHardFlag. I can look into making them actually cacheable but I'll only do so if you think it's worth the trouble.

I was a bit surprised to find that GCC::parse_arguments() seems to treat unknown arguments as always cacheable. Is that on purpose? It seems like a somewhat dangerous default.

michaelwoerister · 2021-04-22T13:08:16Z

@glandium, do you have an estimate on when this can be merged? Is there anything I can do to help?

dancing-leaves · 2021-05-07T08:44:58Z

Hi, would love to see these changes upstreamed, tracking this PR for a while. Thanks for the work, and lets rebase and merge it already. 🙂

From what i see currently it's about this:

Can you also rebase on top of #971 after it's merged? Thank you. (BTW, did you intend your user email not to contain a TLD?)

#952 (review) (@michaelwoerister has done that)

FilipAndersson245 · 2021-06-11T19:11:37Z

@glandium Any idea of when/if this pull request can be merged?

mitchhentges · 2022-02-16T20:30:48Z

Hey @michaelwoerister, thanks for the PR, and sorry for the review delay here 😅
Would you mind rebasing this off of main and re-pushing it? I'll gladly kick off the review and landing process if you're able to update this :)

…str-use.

michaelwoerister · 2022-02-18T11:46:58Z

I rebased the PR and tests are passing locally for me.

codecov-commenter · 2022-02-18T11:53:19Z

Codecov Report

Merging #952 (dc28e0e) into main (573c9ba) will increase coverage by 0.00%.
The diff coverage is 90.56%.

@@           Coverage Diff           @@
##             main     #952   +/-   ##
=======================================
  Coverage   57.81%   57.82%           
=======================================
  Files          47       47           
  Lines       12055    12107   +52     
  Branches     2178     2184    +6     
=======================================
+ Hits         6970     7001   +31     
  Misses       3727     3727           
- Partials     1358     1379   +21

Impacted Files	Coverage Δ
src/compiler/clang.rs	`81.60% <87.80%> (+1.90%)`	⬆️
src/compiler/gcc.rs	`77.12% <100.00%> (-1.88%)`	⬇️
src/compiler/msvc.rs	`65.46% <100.00%> (+0.57%)`	⬆️
src/lru_disk_cache/lru_cache.rs	`99.23% <0.00%> (-0.39%)`	⬇️
src/compiler/args.rs	`76.44% <0.00%> (-0.36%)`	⬇️
src/compiler/compiler.rs	`71.90% <0.00%> (-0.24%)`	⬇️
src/compiler/rust.rs	`57.70% <0.00%> (+0.14%)`	⬆️
src/util.rs	`58.84% <0.00%> (+0.88%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 573c9ba...dc28e0e. Read the comment docs.

mitchhentges

Thanks Michael, and sorry about the review delay.
Let's get this landed 😎

michaelwoerister · 2022-02-21T11:55:11Z

Well, that was quick :) Thanks for the review!

bump sccache for linux x86_64 to allow caching while PGO'd This bumps sccache for linux-only jobs to allow sccache work with PGO mozilla/sccache#952 This is improvement: 2h05m -> 1h44m (from 1 run) for `dist-x86_64-linux` rust-lang#133033 (comment) Why still use old release? Commit with improvement was old, so i used some release around. In future, more recent versions can be tested to see if there any improvements, while this one already gives one. aarch64 update shouldn't change much, as there no PGO used. For other OSes i will create separate PRs later. r? infra

sccache supports `-fprofile-use` since mozilla/sccache#952, released as part of sccache v0.3.0, which is available in Ubuntu since 23.04 release. Bug: 40188007 Change-Id: I5bca8350113ee1940096cd53be543abf4eaf03f7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6013745 Reviewed-by: Nico Weber <thakis@chromium.org> Commit-Queue: Henrique Ferreiro <hferreiro@igalia.com> Cr-Commit-Position: refs/heads/main@{#1384244}

michaelwoerister force-pushed the support-clang-profile-use branch 2 times, most recently from 3c5d320 to 1465cec Compare February 11, 2021 10:27

michaelwoerister mentioned this pull request Feb 16, 2021

Support PGO in the compiler's build system rust-lang/rust#79562

Closed

luser approved these changes Mar 1, 2021

View reviewed changes

src/compiler/gcc.rs Outdated Show resolved Hide resolved

src/compiler/gcc.rs Outdated Show resolved Hide resolved

michaelwoerister force-pushed the support-clang-profile-use branch 3 times, most recently from 271b823 to 8ba25dd Compare March 2, 2021 14:25

glandium mentioned this pull request Mar 10, 2021

Fix clippy warnings with new version of clippy #971

Merged

glandium requested changes Mar 10, 2021

View reviewed changes

glandium added this to the 0.2.16 milestone Mar 10, 2021

michaelwoerister force-pushed the support-clang-profile-use branch from 8ba25dd to adc9b39 Compare March 12, 2021 15:56

Mark-Simulacrum mentioned this pull request Oct 4, 2021

Split out LLVM PGO step and use clang 13 to compile LLVM rust-lang/rust#89499

Merged

mitchhentges added the awaiting response label Feb 16, 2022

Support caching Clang invocations with -fprofile-use and -fprofile-in…

dc28e0e

…str-use.

michaelwoerister force-pushed the support-clang-profile-use branch from adc9b39 to dc28e0e Compare February 18, 2022 11:45

mitchhentges mentioned this pull request Feb 18, 2022

Generically assume that unknown flags are PassThrough #1122

Open

mitchhentges approved these changes Feb 18, 2022

View reviewed changes

mitchhentges merged commit e8c9ca0 into mozilla:main Feb 18, 2022

buttercookie42 mentioned this pull request May 31, 2022

scccache breaks PGO builds #1178

Open

Kobzol mentioned this pull request Aug 11, 2022

PGO profile invalidation in -Cprofile-use rust-lang/rust#100397

Open

This was referenced Nov 14, 2024

turn creating 8dot3 names off for windows for speed rust-lang/rust#133033

Open

bump sccache for linux x86_64 to allow caching while PGO'd rust-lang/rust#133076

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support caching Clang invocations with -fprofile-use and -fprofile-instr-use. #952

Support caching Clang invocations with -fprofile-use and -fprofile-instr-use. #952

michaelwoerister commented Feb 11, 2021

luser left a comment

michaelwoerister commented Mar 2, 2021

glandium left a comment

glandium Mar 10, 2021

michaelwoerister Mar 10, 2021

michaelwoerister Mar 10, 2021

glandium Mar 10, 2021

glandium Mar 10, 2021

michaelwoerister Mar 10, 2021

glandium Mar 10, 2021

michaelwoerister commented Mar 10, 2021

michaelwoerister commented Mar 12, 2021

michaelwoerister commented Apr 22, 2021

dancing-leaves commented May 7, 2021 •

edited

Loading

FilipAndersson245 commented Jun 11, 2021

mitchhentges commented Feb 16, 2022

michaelwoerister commented Feb 18, 2022

codecov-commenter commented Feb 18, 2022 •

edited

Loading

mitchhentges left a comment

michaelwoerister commented Feb 21, 2022

Support caching Clang invocations with -fprofile-use and -fprofile-instr-use. #952

Support caching Clang invocations with -fprofile-use and -fprofile-instr-use. #952

Conversation

michaelwoerister commented Feb 11, 2021

luser left a comment

Choose a reason for hiding this comment

michaelwoerister commented Mar 2, 2021

glandium left a comment

Choose a reason for hiding this comment

glandium Mar 10, 2021

Choose a reason for hiding this comment

michaelwoerister Mar 10, 2021

Choose a reason for hiding this comment

michaelwoerister Mar 10, 2021

Choose a reason for hiding this comment

glandium Mar 10, 2021

Choose a reason for hiding this comment

glandium Mar 10, 2021

Choose a reason for hiding this comment

michaelwoerister Mar 10, 2021

Choose a reason for hiding this comment

glandium Mar 10, 2021

Choose a reason for hiding this comment

michaelwoerister commented Mar 10, 2021

michaelwoerister commented Mar 12, 2021

michaelwoerister commented Apr 22, 2021

dancing-leaves commented May 7, 2021 • edited Loading

FilipAndersson245 commented Jun 11, 2021

mitchhentges commented Feb 16, 2022

michaelwoerister commented Feb 18, 2022

codecov-commenter commented Feb 18, 2022 • edited Loading

Codecov Report

mitchhentges left a comment

Choose a reason for hiding this comment

michaelwoerister commented Feb 21, 2022

dancing-leaves commented May 7, 2021 •

edited

Loading

codecov-commenter commented Feb 18, 2022 •

edited

Loading