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

Build the first LLVM without LTO in try builds #113779

Merged
merged 1 commit into from Jul 27, 2023

Conversation

Kobzol
Copy link
Contributor

@Kobzol Kobzol commented Jul 17, 2023

Currently, we perform three LLVM builds in the Linux x64 dist builder, which is used for try builds:

  1. "Normal" LLVM - takes ~5s to compile thanks to sccache, but ~8 minutes to link because of ThinLTO
  2. PGO instrumented LLVM - same timings as 1)
  3. PGO optimized LLVM - takes about 20 minutes to build

When I tried to disable LTO for build 1), it suddenly takes only about a minute to build, because the linking step is much faster. The first LLVM doesn't really need LTO all that much. Without it, it will be a bit slower to build rustc in two subsequent steps, but it seems that the ~7 minutes saved on linking it do win that back.

Btw, we can't use the host LLVM for build 1), because this LLVM then builds rustc in PGO instrumented mode, and we need the same compiler when later PGO optimizing rustc. And we want to use our in-house LLVM for that I think.

@rustbot
Copy link
Collaborator

rustbot commented Jul 17, 2023

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 17, 2023
@Kobzol
Copy link
Contributor Author

Kobzol commented Jul 17, 2023

@bors try

@bors
Copy link
Contributor

bors commented Jul 17, 2023

⌛ Trying commit 3547fcc1f5346573d75b005185a8ef5b68219b85 with merge 770278c62068bf371f2fb8132362ae1d2ddbbfe2...

@bors
Copy link
Contributor

bors commented Jul 17, 2023

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

1 similar comment
@bors
Copy link
Contributor

bors commented Jul 17, 2023

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

@Kobzol
Copy link
Contributor Author

Kobzol commented Jul 17, 2023

The try build took 1h 8m 9s, which is ~3-4 minutes faster than recent try builds (e.g. https://github.com/rust-lang-ci/rust/actions/runs/5572195042/jobs/10177957839). The space to improve try build duration is getting quite narrow :) Still I think that this is worth it, as I don't really see any downside to it.

@Mark-Simulacrum
Copy link
Member

@bors r+

I guess the subsequent rustc builds are slower if we do this, which is why the gains are less significant... but seems worth it.

@bors
Copy link
Contributor

bors commented Jul 17, 2023

📌 Commit 3547fcc1f5346573d75b005185a8ef5b68219b85 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 17, 2023
@bors
Copy link
Contributor

bors commented Jul 18, 2023

⌛ Testing commit 3547fcc1f5346573d75b005185a8ef5b68219b85 with merge 64ed0e522144c53331cd897d41d103834d57c597...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jul 18, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 18, 2023
@Kobzol
Copy link
Contributor Author

Kobzol commented Jul 18, 2023

Fixed support for msvc.

@Kobzol
Copy link
Contributor Author

Kobzol commented Jul 27, 2023

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Jul 27, 2023

📌 Commit 9c373e3 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 27, 2023
@bors
Copy link
Contributor

bors commented Jul 27, 2023

⌛ Testing commit 9c373e3 with merge 0be1152...

@bors
Copy link
Contributor

bors commented Jul 27, 2023

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 0be1152 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 27, 2023
@bors bors merged commit 0be1152 into rust-lang:master Jul 27, 2023
12 checks passed
@rustbot rustbot added this to the 1.73.0 milestone Jul 27, 2023
@Kobzol Kobzol deleted the try-build-no-lto branch July 27, 2023 21:47
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (0be1152): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

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 open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.6% [0.6%, 0.6%] 1
Improvements ✅
(primary)
-0.6% [-0.6%, -0.6%] 3
Improvements ✅
(secondary)
-0.5% [-0.6%, -0.5%] 5
All ❌✅ (primary) -0.6% [-0.6%, -0.6%] 3

Max RSS (memory usage)

Results

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.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-3.9% [-3.9%, -3.9%] 1
Improvements ✅
(secondary)
-2.4% [-2.5%, -2.3%] 2
All ❌✅ (primary) -3.9% [-3.9%, -3.9%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 652.516s -> 654.49s (0.30%)

@rustbot rustbot added the perf-regression Performance regression. label Jul 28, 2023
@lqd
Copy link
Member

lqd commented Jul 28, 2023

I’m not seeing this change being limited to try builds, but the perf results should be noise.

@Kobzol
Copy link
Contributor Author

Kobzol commented Jul 28, 2023

Sorry, the title should have said x64 dist linux builds, not just try builds. I would believe that the compile changes are noise, but the bootstrap change seems a bit worrying (although I saw similar bootstrap noise already).

I wonder if PGO profiles for Rustc can somehow get worse, when they are gathered with a non LTO optimized LLVM.

@lqd
Copy link
Member

lqd commented Jul 28, 2023

0.3% on bootstrap is not uncommon I think. Here’s 0.26 on the PR merged around this PR, and it doesn’t change the compiler #113298 (comment).

@Kobzol
Copy link
Contributor Author

Kobzol commented Jul 28, 2023

Ok, that looks reasonable.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Jul 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants