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

Stabilize support for Profile-guided Optimization #61268

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@michaelwoerister
Copy link
Contributor

commented May 28, 2019

This PR makes profile-guided optimization available via the -C profile-generate / -C profile-use pair of commandline flags and adds end-user documentation for the feature to the rustc book. The PR thus ticks the last two remaining checkboxes of the stabilization tracking issue.

From the tracking issue:

Profile-guided optimization (PGO) is a common optimization technique for ahead-of-time compilers. It works by collecting data about a program's typical execution (e.g. probability of branches taken, typical runtime values of variables, etc) and then uses this information during program optimization for things like inlining decisions, machine code layout, or indirect call promotion.

If you are curious about how this can be used, there is a rendered version of the documentation this PR adds available here.

r? @alexcrichton
cc @rust-lang/compiler

@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

Dear @rust-lang/compiler, please add your check mark below.
@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

commented May 28, 2019

Team member @michaelwoerister has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

Show resolved Hide resolved src/doc/rustc/src/profile-guided-optimization.md
Show resolved Hide resolved src/doc/rustc/src/profile-guided-optimization.md Outdated
Show resolved Hide resolved src/doc/rustc/src/profile-guided-optimization.md

- It is recommended to use *absolute paths* for the argument of
`-Cprofile-generate` and `-Cprofile-use`. Cargo can invoke `rustc` with
varying working directories, meaning that `.profraw` files might show up

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton May 28, 2019

Member

Hm this isn't quite right I think, don't .profraw files show up in the cwd by default? When compiling with profile-use, however, I think you definitely want to use absolute paths.

That being said absolute paths I think are still always your best bet.

This comment has been minimized.

Copy link
@michaelwoerister

michaelwoerister May 29, 2019

Author Contributor

True, things would still work for the "generate" case. I'll re-word but keep the recommendation for using absolute paths.

Show resolved Hide resolved src/doc/rustc/src/profile-guided-optimization.md Outdated
Show resolved Hide resolved src/doc/rustc/src/profile-guided-optimization.md
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented May 28, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:0dcd936a:start=1559056887674898160,finish=1559056888433409042,duration=758510882
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
$ export AWS_ACCESS_KEY_ID=AKIA46X5W6CZEJZ6XT55
---
travis_time:start:test_codegen
Check compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:20:41] 
[01:20:41] running 143 tests
[01:20:44] i..iii.....iii..iiii.....i......................i...i................i......i.........ii.i..i..i.ii. 100/143
[01:20:46] test result: ok. 113 passed; 0 failed; 30 ignored; 0 measured; 0 filtered out
[01:20:46] 
[01:20:46]  finished in 4.696
[01:20:46] travis_fold:end:test_codegen
---
travis_time:start:test_assembly
Check compiletest suite=assembly mode=assembly (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:20:48] 
[01:20:48] running 9 tests
[01:20:48] iiiiiiiii
[01:20:48] 
[01:20:48]  finished in 0.158
[01:20:48] travis_fold:end:test_assembly

---
travis_time:start:test_debuginfo
Check compiletest suite=debuginfo mode=debuginfo-gdb+lldb (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:21:04] 
[01:21:04] running 122 tests
[01:21:30] .iiiii...i.....i..i...i..i.i.i..i.ii..i.i.....i..i....i..........iiii..........i...ii...i.......ii.i 100/122
[01:21:35] .i.i......iii.i.....ii
[01:21:35] 
[01:21:35]  finished in 30.513
[01:21:35] travis_fold:end:test_debuginfo

---
[01:45:25] 
[01:45:25] stdout ----
[01:45:25] 
[01:45:25] running 1 test
[01:45:25] test /checkout/src/doc/rustc/src/profile-guided-optimization.md - Profile_Guided_Optimization::Usage (line 43) ... FAILED
[01:45:25] failures:
[01:45:25] 
[01:45:25] 
[01:45:25] ---- /checkout/src/doc/rustc/src/profile-guided-optimization.md - Profile_Guided_Optimization::Usage (line 43) stdout ----
[01:45:25] error: expected one of `!`, `.`, `::`, `;`, `?`, `{`, `}`, or an operator, found `component`
[01:45:25]  --> /checkout/src/doc/rustc/src/profile-guided-optimization.md:44:8
[01:45:25] 3 | rustup component add llvm-tools-preview
[01:45:25]   |        ^^^^^^^^^ expected one of 8 possible tokens here
[01:45:25] 
[01:45:25] error[E0425]: cannot find value `rustup` in this scope
---
[01:45:25] 
[01:45:25] error: aborting due to 2 previous errors
[01:45:25] 
[01:45:25] For more information about this error, try `rustc --explain E0425`.
[01:45:25] thread '/checkout/src/doc/rustc/src/profile-guided-optimization.md - Profile_Guided_Optimization::Usage (line 43)' panicked at 'couldn't compile the test', src/librustdoc/test.rs:320:13
[01:45:25] 
[01:45:25] 
[01:45:25] failures:
[01:45:25] failures:
[01:45:25]     /checkout/src/doc/rustc/src/profile-guided-optimization.md - Profile_Guided_Optimization::Usage (line 43)
[01:45:25] test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
[01:45:25] 
[01:45:25] 
[01:45:25] stderr ----
[01:45:25] stderr ----
[01:45:25] 
[01:45:25] 
[01:45:25] 
[01:45:25] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:45:25] Build completed unsuccessfully in 0:36:45
[01:45:25] make: *** [check] Error 1
[01:45:25] Makefile:48: recipe for target 'check' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:354b0800
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Tue May 28 17:07:05 UTC 2019
---
travis_time:end:094dae8e:start=1559063227275306941,finish=1559063227280379075,duration=5072134
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:00e441d8
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:0d57b450
travis_time:start:0d57b450
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:0985bd58
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@bors

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

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

@michaelwoerister michaelwoerister force-pushed the michaelwoerister:stabilize-pgo branch from e8b88e9 to f334f2c May 29, 2019

@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

@alexcrichton, I removed the -Ccodegen-units=1 flags from the examples. What do you think of defaulting to one codegen unit (with the option to opt out) when compiling with PGO? I think that would remove a common footgun. And, perhaps more importantly, it removes ThinLTO from the equation which can only make things better/more reliable.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented May 29, 2019

I mentioned above that I wouldn't switch CGUs by default just yet, but I would also expect ThinLTO to take PGO data into account to know what to inline, but I could very well be wrong about that!

@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

I mentioned above that I wouldn't switch CGUs by default just yet, but I would also expect ThinLTO to take PGO data into account to know what to inline, but I could very well be wrong about that!

OK, let's keep the two settings independent then. ThinLTO should indeed take PGO data into account since PGO adds regular cold and inlinehint annotation to functions.

This is mostly my general slight mistrust in (Thin)LTO speaking. It's just one more step where things can go wrong (and have done so in the past). I personally would always default to one CGU for any kind of production code. But that's a topic for another discussion :)

@bors

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

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

@michaelwoerister michaelwoerister force-pushed the michaelwoerister:stabilize-pgo branch from f334f2c to 96f0086 May 30, 2019

@rfcbot

This comment has been minimized.

Copy link

commented Jun 4, 2019

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

@rfcbot

This comment has been minimized.

Copy link

commented Jun 14, 2019

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.

The RFC will be merged soon.

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2019

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

@michaelwoerister michaelwoerister force-pushed the michaelwoerister:stabilize-pgo branch from 96f0086 to b7fe2ca Jun 21, 2019

@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

@bors r=alexcrichton (right?)

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

📌 Commit b7fe2ca has been approved by alexcrichton

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.