Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PGO cannot be used via cargo because of how RUSTFLAGS are handled. #7416

Closed
michaelwoerister opened this issue Sep 23, 2019 · 4 comments · Fixed by #7417
Closed

PGO cannot be used via cargo because of how RUSTFLAGS are handled. #7416

michaelwoerister opened this issue Sep 23, 2019 · 4 comments · Fixed by #7417
Labels
C-bug Category: bug

Comments

@michaelwoerister
Copy link
Member

Problem

PGO works in two phases:

  1. Compile an instrumented binary of your program that enables collecting profile data. This is done via RUSTFLAGS=-Cprofile-generate.
  2. Compile an optimized binary that makes use of the profile data collected in step (1). This is done via RUSTFLAGS=-Cprofile-use.

In order for this to work, the symbol names within the two program versions must match up because LLVM generates profiling data in terms of symbol names.

However, since #6503, RUSTFLAGS are fed into the -Cmetadata argument to rustc. This causes symbol names to differ because one version has -Cprofile-generate and the other has -Cprofile-use in their RUSTFLAGS.

I think I would prefer not feeding RUSTFLAGS into -C metadata at all. It does not seem right to me that symbol names are affected by random command line arguments to the compiler.

@alexcrichton
Copy link
Member

I've proposed a revert of #6503 at #7417 to fix this.

@michaelwoerister out of curiosity was this the cause of why y'all weren't seeing great wins for PGO in Firefox?

@michaelwoerister
Copy link
Member Author

@alexcrichton I have confirmed that this issue prevents Firefox from getting any PGO wins, yes. However, I don't know yet if it is the only issue (e.g. GNU ld seems to have a bug that sometimes causes instrumented builds to not record anything 😱).

I have some microbenchmarks that show a 10-15% win with PGO but have not been able to replicate that with the regex benchmark suite, which is my next bigger test case.

@alexcrichton
Copy link
Member

Oh dear that must have been absolutely gnarly to figure this out, sorry about that!

@michaelwoerister
Copy link
Member Author

It's exactly the kind of issue I expected to encounter :)

bors added a commit that referenced this issue Sep 30, 2019
Go back to not hashing `RUSTFLAGS` in `-Cmetadata`

This is a moral revert of #6503 but not a literal code revert. This
switches Cargo's behavior to avoid hashing compiler flags into
`-Cmetadata` since we've now had multiple requests of excluding flags
from the `-Cmetadata` hash: usage of `--remap-path-prefix` and PGO
options. These options should only affect how the compiler is
invoked/compiled and not radical changes such as symbol names, but
symbol names are changed based on `-Cmetadata`. Instead Cargo will still
track these flags internally, but only for reinvoking rustc, and not for
caching separately based on rustc flags.

Closes #7416
@bors bors closed this as completed in f3c92ed Sep 30, 2019
alexcrichton added a commit to alexcrichton/cargo that referenced this issue Sep 30, 2019
This is a moral revert of rust-lang#6503 but not a literal code revert. This
switches Cargo's behavior to avoid hashing compiler flags into
`-Cmetadata` since we've now had multiple requests of excluding flags
from the `-Cmetadata` hash: usage of `--remap-path-prefix` and PGO
options. These options should only affect how the compiler is
invoked/compiled and not radical changes such as symbol names, but
symbol names are changed based on `-Cmetadata`. Instead Cargo will still
track these flags internally, but only for reinvoking rustc, and not for
caching separately based on rustc flags.

Closes rust-lang#7416
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants
@alexcrichton @michaelwoerister and others