-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
Only apply LTO to rustdoc at stage 2 #136586
base: master
Are you sure you want to change the base?
Conversation
@bors try @rust-timer queue Just to make sure that it doesn't regress perf. |
This comment has been minimized.
This comment has been minimized.
Only apply LTO to rustdoc at stage 2 It doesn't make much sense at stage 1, and it was broken anyway. This was implemented in rust-lang#135832. The issue with LTO and stage 1 rustdoc was reported [here](https://rust-lang.zulipchat.com/#narrow/channel/326414-t-infra.2Fbootstrap/topic/x.20test.20with.20lto.20.3D.20.22thin.22.20fails.20to.20build.20rustdoc.3F). r? `@onur-ozkan`
If LTO applied to rustdoc is broken at stage 1, won't it still be broken if applied at stage 2...? 🤔 |
It is only "broken" because we also skip LTO in stage 1 for |
So does this mean my |
That seems strange in general, if that is indeed the case... 🤔 |
If you're talking about LTO optimizing librustc_driver.so, then yes, it is currently only performed for stage 2, not lower stages. The CI performance is much more affected by PGO + BOLT (~30%) than LTO (5-10%) though. |
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM other than a non-blocker nit
It doesn't make much sense at stage 1, and it was broken anyway.
046385b
to
cfa3518
Compare
@bors r+ rollup |
perf results will be ready in 20 mins |
Finished benchmarking commit (f02aecb): comparison URL. Overall result: no relevant changes - no action neededBenchmarking 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 may lead to changes in compiler perf. @bors rollup=never Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (primary -1.1%, secondary 0.3%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (primary 2.7%, secondary 3.2%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 778.541s -> 779.409s (0.11%) |
@Kobzol: it's likely unrelated because they're not doc benchmarks, but look at these cycles results what the eff |
Yeah it's most probably unrelated. Probably solar flares. |
…ur-ozkan Only apply LTO to rustdoc at stage 2 It doesn't make much sense at stage 1, and it was broken anyway. This was implemented in rust-lang#135832. The issue with LTO and stage 1 rustdoc was reported [here](https://rust-lang.zulipchat.com/#narrow/channel/326414-t-infra.2Fbootstrap/topic/x.20test.20with.20lto.20.3D.20.22thin.22.20fails.20to.20build.20rustdoc.3F). r? `@onur-ozkan`
💔 Test failed - checks-actions |
There was I think a github actions outage at that time or sth. Or sometimes a job just doesn't get picked up for whatever reason. |
It doesn't make much sense at stage 1, and it was broken anyway. This was implemented in #135832. The issue with LTO and stage 1 rustdoc was reported here.
r? @onur-ozkan