-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Make "Assemble stage1 compiler" orders of magnitude faster (take 2) #97268
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
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
@bors r+ |
📌 Commit 460ad720eacb3c5eb6522ddf2f31826969809e9b has been approved by |
@bors r- Let's hold off until we confirm the permissions strategy here. |
Ok, I updated the PR to:
There is no more special casing @rustbot ready |
I tested this by cherry-picking b2f0e38ffe210a0099faad9b1594feb6fcf0b2da onto #96803 and making sure that |
@bors try |
⌛ Trying commit fb469766fcc0d11aa86736a677fca4a18bf43be9 with merge 4e819be6a73e5fd42d55a1f5d61c69264ae74cbc... |
This comment has been minimized.
This comment has been minimized.
💔 Test failed - checks-actions |
This error is strange. @epage do you know how |
Looks like neither rustup nor rustup-toolchain-install-master are able to install it - they assume the target has
I'm going to go ahead and special-case it. |
@bors try |
⌛ Trying commit 3213672cf8e60bb8440700bfc8e553a81a08de20 with merge bc0fb8974a74c2c48b91bde70bd794ee2998a310... |
☀️ Try build successful - checks-actions |
Ok cool! This should be ready for review then. @rustbot ready |
r=me with comments resolved (let me know if you feel there's a need for another review, but I expect resolution to be pretty non-noteworthy) |
52e52e7
to
3905e37
Compare
This avoids regressions in rustup-toolchain-install-master
This used to take upwards of 5 seconds for me locally. I found that the culprit was copying the downloaded LLVM shared object: ``` [22:28:03] Install "/home/jnelson/rust-lang/rust/build/x86_64-unknown-linux-gnu/ci-llvm/lib/libLLVM-14-rust-1.62.0-nightly.so" to "/home/jnelson/rust-lang/rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/x86_64-unknown-linux-gnu/lib/libLLVM-14-rust-1.62.0-nightly.so" [22:28:09] c Sysroot { compiler: Compiler { stage: 1, host: x86_64-unknown-linux-gnu(x86_64-unknown-linux-gnu) } } ``` It turned out that `install()` used full copies unconditionally. Change it to use `copy()` internally, which uses hard links instead when available. Note that this has a change in behavior: Installing a file will also change permissions on the source, not just the destination, if hard links are used. To avoid changing the behavior on symlinks for existing code, I introduce a new function `copy_internal` which only dereferences symlinks when told to do so.
@bors r=Mark-Simulacrum |
📌 Commit 057eab7 has been approved by |
☀️ Test successful - checks-actions |
Finished benchmarking commit (611e7b9): comparison url. Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results
CyclesResults
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression Footnotes |
This used to take upwards of 5 seconds for me locally. I found that the culprit was copying the downloaded LLVM shared object:
It turned out that
install()
used full copies unconditionally. Change it to try using a hard-link before falling back to copying.I also took the liberty of fixing
x dist llvm-tools
to work even if you don't callx build
previously.