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.

Sign up for GitHub

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

Jump to bottom

Enable PGO for LLVM on x86_64-unknown-linux-gnu in CI #87940

Closed
wants to merge 1 commit into from
Closed

Enable PGO for LLVM on x86_64-unknown-linux-gnu in CI #87940

wants to merge 1 commit into from

Conversation

Mark-Simulacrum
Copy link
Member

Particularly for optimized builds, rustc spends a good amount of time in LLVM
builds, so it is hopeful that this will be worthwhile.

We can likely afford to spend the extra cycles building LLVM 1-2 times per CI
build today on x86_64-unknown-linux-gnu: the builder finishes in around 50
minutes, whereas our long pole is around 2-3 hours. For try builds this is less
clear, but we will see what the impact is in this PR.

r? @ghost for now while we run perf -- and I still need to polish the code into less ad-hoc reuse of the rust pgo options for LLVM.

@Mark-Simulacrum
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 11, 2021
@bors
Copy link
Contributor

bors commented Aug 11, 2021

⌛ Trying commit b3771e03a433c2cce572d4f039e891d759243343 with merge e0143fd4632a9cb683a86475c0ef60f1e05399d2...

@bors
Copy link
Contributor

bors commented Aug 11, 2021

💔 Test failed - checks-actions

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 11, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Mark-Simulacrum
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Aug 11, 2021

⌛ Trying commit 56493a33fe1013ea02fc39fff2f608526d945668 with merge 9bb62366c0301aee13828ecc85dd5f45fc117919...

@bors
Copy link
Contributor

bors commented Aug 11, 2021

☀️ Try build successful - checks-actions
Build commit: 9bb62366c0301aee13828ecc85dd5f45fc117919 (9bb62366c0301aee13828ecc85dd5f45fc117919)

@rust-timer
Copy link
Collaborator

Queued 9bb62366c0301aee13828ecc85dd5f45fc117919 with parent 362e0f5, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (9bb62366c0301aee13828ecc85dd5f45fc117919): comparison url.

Summary: This change led to significant mixed results 🤷 in compiler performance.

  • Small improvement in instruction counts (up to -0.8% on full builds of keccak)
  • Very large regression in instruction counts (up to 71.1% on full builds of regression-31157)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

@rustbot rustbot added perf-regression Performance regression. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Aug 12, 2021
@Mark-Simulacrum
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 12, 2021
@bors
Copy link
Contributor

bors commented Aug 12, 2021

⌛ Trying commit e78681221bdd84dc8a8015722ac88cda6ead5ada with merge 5a8fff39d29259612bca8313679d03695e0c4fed...

@Mark-Simulacrum
Copy link
Member Author

@bors try- try

@bors
Copy link
Contributor

bors commented Aug 12, 2021

⌛ Trying commit 5ab3c65c01c791610b60d12e3092f69608cc057c with merge 1c2981c1e35355691fecc49c175621e7b7170641...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Aug 12, 2021

💔 Test failed - checks-actions

@Mark-Simulacrum
Copy link
Member Author

@bors retry

@bors
Copy link
Contributor

bors commented Aug 13, 2021

⌛ Trying commit 926931d9603931dbdc1d77b14549c99a92074025 with merge 148f4b24096abff7e78ff2df8dbbfe1c13a2718d...

@bors
Copy link
Contributor

bors commented Aug 13, 2021

☀️ Try build successful - checks-actions
Build commit: 148f4b24096abff7e78ff2df8dbbfe1c13a2718d (148f4b24096abff7e78ff2df8dbbfe1c13a2718d)

@Mark-Simulacrum
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 15, 2021
@bors
Copy link
Contributor

bors commented Aug 15, 2021

⌛ Trying commit c5dbe5882e753f72a065a8a7b3386bffab1d3f37 with merge b8cc94067a73f2558b2ee1050882b7bdc0545a8b...

@bors
Copy link
Contributor

bors commented Aug 15, 2021

☀️ Try build successful - checks-actions
Build commit: b8cc94067a73f2558b2ee1050882b7bdc0545a8b (b8cc94067a73f2558b2ee1050882b7bdc0545a8b)

@rust-timer
Copy link
Collaborator

Queued b8cc94067a73f2558b2ee1050882b7bdc0545a8b with parent dfe5fd0, future comparison URL.

bjorn3
bjorn3 reviewed Aug 15, 2021
View reviewed changes
src/ci/docker/host-x86_64/dist-x86_64-linux/Dockerfile Outdated
Comment on lines 86 to 87
RUN curl -LS -o perf.zip https://github.com/rust-lang/rustc-perf/archive/$PERF_COMMIT.zip
RUN unzip perf.zip && mv rustc-perf-$PERF_COMMIT rustc-perf
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may want to do this in a single RUN command removing perf.zip afterwards to reduce the image size. Docker creates a new layer for each RUN command AFAIK.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (b8cc94067a73f2558b2ee1050882b7bdc0545a8b): comparison url.

Summary: This change led to significant mixed results 🤷 in compiler performance.

  • Very large improvement in instruction counts (up to -6.5% on incr-patched: b9b3e592dd cherry picked builds of style-servo)
  • Large regression in instruction counts (up to 2.3% on full builds of match-stress-enum)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Aug 15, 2021
@Mark-Simulacrum
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 15, 2021
@bors
Copy link
Contributor

bors commented Aug 15, 2021

⌛ Trying commit 754c39c7ccd4c8a89ba1b6789d563fa38b62b362 with merge 9b6451fdf3633e1c4602c1645ca577abd8efdb32...

@bors
Copy link
Contributor

bors commented Aug 15, 2021

☀️ Try build successful - checks-actions
Build commit: 9b6451fdf3633e1c4602c1645ca577abd8efdb32 (9b6451fdf3633e1c4602c1645ca577abd8efdb32)

@rust-timer
Copy link
Collaborator

Queued 9b6451fdf3633e1c4602c1645ca577abd8efdb32 with parent c0490a2, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (9b6451fdf3633e1c4602c1645ca577abd8efdb32): comparison url.

Summary: This change led to significant mixed results 🤷 in compiler performance.

  • Very large improvement in instruction counts (up to -6.6% on incr-patched: b9b3e592dd cherry picked builds of style-servo)
  • Small regression in instruction counts (up to 0.4% on incr-unchanged builds of diesel)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 15, 2021
@Mark-Simulacrum
PGO for LLVM builds on x86_64-unknown-linux-gnu in CI
dd9a970 
This shows up to 5% less instruction counts on multiple benchmarks, and up to
19% wins on the -j1 wall times for rustc self-compilation.

We can afford to spend the extra cycles building LLVM essentially once more for
the x86_64-unknown-linux-gnu CI build today. The builder finishes in around 50
minutes on average, and this adds just 10 more minutes. Given the sizeable
improvements in compiler performance, this is definitely worth it.
@Mark-Simulacrum
Copy link
Member Author

Going to close this in favor of a new PR to ease discussion & review, given the large number of comments here.

@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Aug 16, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjorn3 bjorn3 bjorn3 left review comments

Assignees
No one assigned
Labels
perf-regression Performance regression. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Mark-Simulacrum @rust-timer @bors @rust-log-analyzer @bjorn3 @rustbot