Skip to content

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

New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom
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.
@michaelwoerister michaelwoerister added the C-bug Category: bug label Sep 23, 2019
@michaelwoerister michaelwoerister mentioned this issue Sep 23, 2019
Merged
@alexcrichton alexcrichton mentioned this issue Sep 23, 2019
Merged
@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
@bors
Auto merge of #7417 - alexcrichton:less-hashing, r=Eh2406
99c034a 
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
@alexcrichton
Go back to not hashing RUSTFLAGS in -Cmetadata
be712ff 
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
@jonas-schievink jonas-schievink mentioned this issue Jan 7, 2020
Closed
weihanglo added a commit to weihanglo/cargo that referenced this issue Nov 26, 2024
@weihanglo
test(pgo): ensure PGO works
8089d8d 
This is a regression test to prevent issues like rust-lang#7416.

The test only run on Linux,
as other platforms have different requirements for PGO,
or emit differnt PGO function missing warnings.
weihanglo added a commit to weihanglo/cargo that referenced this issue Nov 26, 2024
@weihanglo
test(pgo): ensure PGO works
c9ce9aa 
This is a regression test to prevent issues like rust-lang#7416.

The test only run on Linux,
as other platforms have different requirements for PGO,
or emit different PGO function missing warnings.
@weihanglo weihanglo mentioned this issue Nov 26, 2024
Merged
weihanglo added a commit to weihanglo/cargo that referenced this issue Nov 26, 2024
@weihanglo
test(pgo): ensure PGO works
02e25d5 
This is a regression test to prevent issues like rust-lang#7416.

The test only run on Linux,
as other platforms have different requirements for PGO,
or emit different PGO function missing warnings.
github-merge-queue bot pushed a commit that referenced this issue Nov 26, 2024
@epage
test(pgo): ensure PGO works (#14859)
917e646 
### What does this PR try to resolve?

This is a regression test to prevent issues like #7416.

The test only run on Linux,
as other platforms have different requirements for PGO,
or emit different PGO function missing warnings.

### How should we test and review this PR?

Not sure how brittle it is. We can optionally run it only on Cargo's CI?

cc #14830
# for free to join this conversation on GitHub. Already have an account? # to comment
Assignees
No one assigned
Labels
C-bug Category: bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Go back to not hashing RUSTFLAGS in -Cmetadata alexcrichton/cargo
3 participants
@alexcrichton @michaelwoerister and others