opt-dist: rebuild rustc when doing static LLVM builds

[ognevny]

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]

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

[rustbot]

Some changes occurred in src/tools/opt-dist

cc @Kobzol

[ognevny]

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

[Kobzol]

@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]

⌛ Trying commit 32edd53 with merge 666996a…

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

[ognevny]

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]

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

[Kobzol]

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]

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]

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

[Kobzol]

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]

thanks for the hint! will do soon

[Kobzol]

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]

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]

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]

$ 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]

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

[ognevny]

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

[Kobzol]

Hmm, kinda hard to say what happens there. Maybe completely avoid skipping anything, and just build the whole of rustc and libstd from scratch, and we can try again.

[ognevny]

Hmm, kinda hard to say what happens there. Maybe completely avoid skipping anything, and just build the whole of rustc and libstd from scratch, and we can try again.

you mean remove --keep-stage-std 1?

[Kobzol]

Yeah, and maybe also --keep-stage.

[ognevny]

Yeah, and maybe also --keep-stage.

done

[Kobzol]

@bors2 try

[rust-bors]

⌛ Trying commit 4805489 with merge a094db8…

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

[rust-bors]

☀️ Try build successful (CI)
Build commit: a094db8 (a094db85f18d96658ed83010f55e8a24a68a26a0, parent: cccf075eba88363269e8589ebb8d40874cc542d8)