Skip to content

opt-dist: rebuild rustc when doing static LLVM builds #143898

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ognevny
Copy link
Contributor

@ognevny ognevny commented Jul 13, 2025

when building LLVM it's obvious that in case of shared build rustc doesn't need to be recompiled, but with static builds it would be better to compile rustc again to ensure we linked proper library.
maybe I didn't understand the pipeline correctly, but it was strange for me to see that in Stage 5 LLVM is built while rustc is not

r? @Kobzol

try-job: dist-x86_64-msvc
try-job: dist-x86_64-linux

@rustbot
Copy link
Collaborator

rustbot commented Jul 13, 2025

Kobzol is not on the review rotation at the moment.
They may take a while to respond.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 13, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 13, 2025

Some changes occurred in src/tools/opt-dist

cc @Kobzol

when building LLVM it's obvious that in case of shared build rustc doesn't need to be recompiled,
but with static builds it would be better to compile rustc again to ensure we linked proper library.
maybe I didn't understand the pipeline correctly, but it was strange for me to see that in Stage 5
LLVM is built while rustc is not
@ognevny ognevny force-pushed the opt-dist-rustc-rebuild branch from 8d3a40a to 32edd53 Compare July 13, 2025 20:05
@ognevny
Copy link
Contributor Author

ognevny commented Jul 13, 2025

reverted stage 2 change as code comment is pretty self-explanatory

@Kobzol
Copy link
Member

Kobzol commented Jul 13, 2025

@bors2 try

but it was strange for me to see that in Stage 5 LLVM is built while rustc is not

In stage 5 we shouldn't be building neither rustc nor LLVM. Bootstrap already skips building LLVM if it exists on disk, which is why we don't need to pass anything like avoid_llvm_rebuild.

rust-bors bot added a commit that referenced this pull request Jul 13, 2025
opt-dist: rebuild rustc when doing static LLVM builds

when building LLVM it's obvious that in case of shared build rustc doesn't need to be recompiled, but with static builds it would be better to compile rustc again to ensure we linked proper library.
maybe I didn't understand the pipeline correctly, but it was strange for me to see that in Stage 5 LLVM is built while rustc is not

<!-- homu-ignore:start -->
<!--
If this PR is related to an unstable feature or an otherwise tracked effort,
please link to the relevant tracking issue here. If you don't know of a related
tracking issue or there are none, feel free to ignore this.

This PR will get automatically assigned to a reviewer. In case you would like
a specific user to review your work, you can assign it to them by using

    r? <reviewer name>
-->
<!-- homu-ignore:end -->

r? `@Kobzol`

try-job: dist-x86_64-msvc
try-job: dist-x86_64-linux
@rust-bors
Copy link

rust-bors bot commented Jul 13, 2025

⌛ Trying commit 32edd53 with merge 666996a

To cancel the try build, run the command @bors2 try cancel.

@ognevny
Copy link
Contributor Author

ognevny commented Jul 13, 2025

In stage 5 we shouldn't be building neither rustc nor LLVM. Bootstrap already skips building LLVM if it exists on disk, which is why we don't need to pass anything like avoid_llvm_rebuild.

this happens only if BOLT stage is run. otherwise right after llvm cleanup at stage 2 stage 5 starts

@ognevny
Copy link
Contributor Author

ognevny commented Jul 13, 2025

you can refer to this GHA job to see how the pipeline works on MSYS2 environment

@Kobzol
Copy link
Member

Kobzol commented Jul 13, 2025

In stage 5 we shouldn't be building neither rustc nor LLVM. Bootstrap already skips building LLVM if it exists on disk, which is why we don't need to pass anything like avoid_llvm_rebuild.

this happens only if BOLT stage is run. otherwise right after llvm cleanup at stage 2 stage 5 starts

Right, and on Windows we don't use BOLT, so there LLVM gets rebuilt in the final dist step.

But rustc indeed wasn't rebuilt in the final stage though. Which means that we have been using a non-LTO-optimized LLVM build on Windows this whole time? Lol.

Could you please post some comparison benchmarks with/without this change?

@ognevny
Copy link
Contributor Author

ognevny commented Jul 13, 2025

Could you please post some comparison benchmarks with/without this change?

sorry, I had literally no skill at good benchmarking, and I need to ensure that I have "clean" environment on my machine...

if no one have time, I can try to do so tomorrow by the evening, I guess

@rust-bors
Copy link

rust-bors bot commented Jul 14, 2025

☀️ Try build successful (CI)
Build commit: 666996a (666996a7659a64c146e4765a1bba6ddc563819d6, parent: 56835d7ac14da9f966e1ff39fd9ffd2e29b764d1)

@Kobzol
Copy link
Member

Kobzol commented Jul 14, 2025

With/without LLVM PGO should be a bit difference, around 10% or something, that should be relatively easy to measure. Doing e.g. hyperfine "cargo +56835d7ac14da9f966e1ff39fd9ffd2e29b764d1 build --release" "cargo+666996a7659a64c146e4765a1bba6ddc563819d6 build --release" --prepare "rmdir /s /q target" on a small project, e.g. serde, should be enough, I think.

You can install these commits using https://github.com/kennytm/rustup-toolchain-install-master.

@ognevny
Copy link
Contributor Author

ognevny commented Jul 14, 2025

thanks for the hint! will do soon

@Kobzol
Copy link
Member

Kobzol commented Jul 14, 2025

I wanted to get more hands-on with Rust on Windows for unrelated reasons, so I rebooted into Windows and tried it there (a horrible experience, tbh :D). I found a 10% performance difference.. but in the wrong direction, lol.

D:\pg\rust\serde>hyperfine "cargo +56835d7ac14da9f966e1ff39fd9ffd2e29b764d1 build --release" "cargo +666996a7659a64c146e4765a1bba6ddc563819d6 build --release" --prepare "rmdir /s /q target"
Benchmark 1: cargo +56835d7ac14da9f966e1ff39fd9ffd2e29b764d1 build --release
  Time (mean ± σ):      9.466 s ±  0.483 s    [User: 87.788 s, System: 2.484 s]
  Range (min … max):    8.749 s … 10.174 s    10 runs

Benchmark 2: cargo +666996a7659a64c146e4765a1bba6ddc563819d6 build --release
  Time (mean ± σ):     10.369 s ±  0.216 s    [User: 98.415 s, System: 2.568 s]
  Range (min … max):   10.111 s … 10.748 s    10 runs

Summary
  cargo +56835d7ac14da9f966e1ff39fd9ffd2e29b764d1 build --release ran
    1.10 ± 0.06 times faster than cargo +666996a7659a64c146e4765a1bba6ddc563819d6 build --release

Let's see if you can reproduce.. Btw I confirmed that stage2 rustc was now rebuilt on MSVC during the final stage (it wasn't before).

@ognevny
Copy link
Contributor Author

ognevny commented Jul 14, 2025

You can install these commits using https://github.com/kennytm/rustup-toolchain-install-master.

I can't download cargo with it:

rustup-toolchain-install-master 56835d7ac14da9f966e1ff39fd9ffd2e29b764d1 --component=cargo -i x86_64-pc-windows-msvc
toolchain `56835d7ac14da9f966e1ff39fd9ffd2e29b764d1` is already installed

@Kobzol
Copy link
Member

Kobzol commented Jul 14, 2025

I don't think you need cargo, you can just use whatever you have locally. But if you want it, delete the toolchain first (rustup toolchain remove 56835d7ac14da9f966e1ff39fd9ffd2e29b764d1), and then install it again.

@ognevny
Copy link
Contributor Author

ognevny commented Jul 14, 2025

$ hyperfine "cargo +56835d7ac14da9f966e1ff39fd9ffd2e29b764d1 build --release" "cargo +666996a7659a64c146e4765a1bba6ddc563819d6 build --release" --prepare "cargo clean" --runs 5
Benchmark 1: cargo +56835d7ac14da9f966e1ff39fd9ffd2e29b764d1 build --release
  Time (mean ± σ):     10.883 s ±  0.491 s    [User: 77.163 s, System: 5.557 s]
  Range (min … max):   10.260 s … 11.633 s    5 runs

Benchmark 2: cargo +666996a7659a64c146e4765a1bba6ddc563819d6 build --release
  Time (mean ± σ):     11.376 s ±  0.291 s    [User: 82.654 s, System: 5.692 s]
  Range (min … max):   11.085 s … 11.855 s    5 runs

Summary
  cargo +56835d7ac14da9f966e1ff39fd9ffd2e29b764d1 build --release ran
    1.05 ± 0.05 times faster than cargo +666996a7659a64c146e4765a1bba6ddc563819d6 build --release

@ognevny
Copy link
Contributor Author

ognevny commented Jul 14, 2025

I'm trying to compile ruff now, but I can't share the results soon as I got to go

@ognevny
Copy link
Contributor Author

ognevny commented Jul 14, 2025

compiler with my change is still slower...

$ hyperfine "cargo +56835d7ac14da9f966e1ff39fd9ffd2e29b764d1 build --release" "cargo +666996a7659a64c146e4765a1bba6ddc563819d6 build --release" --prepare "cargo clean" --runs 5
Benchmark 1: cargo +56835d7ac14da9f966e1ff39fd9ffd2e29b764d1 build --release
  Time (mean ± σ):     382.541 s ±  6.358 s    [User: 3822.163 s, System: 264.707 s]
  Range (min … max):   375.242 s … 389.466 s    5 runs

Benchmark 2: cargo +666996a7659a64c146e4765a1bba6ddc563819d6 build --release
  Time (mean ± σ):     388.287 s ±  0.990 s    [User: 3978.029 s, System: 264.682 s]
  Range (min … max):   387.549 s … 390.011 s    5 runs

Summary
  cargo +56835d7ac14da9f966e1ff39fd9ffd2e29b764d1 build --release ran
    1.02 ± 0.02 times faster than cargo +666996a7659a64c146e4765a1bba6ddc563819d6 build --release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants