-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Move locking from bootstrap.py to rust bootstrap, using fd-lock #98373
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
16ae306
to
be41698
Compare
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.
This is great, thanks! I have a bad feeling CI will fail because of the new dependencies, but r=me otherwise. If it does fail, we should probably have a discussion with Mark / infra team about whether the dependencies are ok.
be41698
to
7b66649
Compare
Oh, last thing - can you see what the compile times for bootstrap before and after this PR? You can clear only the bootstrap artifacts (and not the stage0 toolchain) with |
On my system, 16.39s before, 16.83s after. I think the new dependencies build almost entirely in parallel with other things. |
I'm sad we need this many dependencies added, but I suppose they're all fast to compile. I am a little worried that this is moving the locking into rustbuild, which means that (for example) submodule management is no longer covered by it -- probably amongst several other similar things. Are we confident those behave well when racing two different builds? |
Submodule handling is being moved out of python in #97513, happy to wait until that's merged to land this. The only other major things are downloading the bootstrap toolchain and building rustbuild. Downloading the toolchain is mostly atomic: we download the files to a temporary directory, then atomically rename them to somewhere in build/cache. Extracting is not atomic but the chance you try and extract the same destination file at once seems vanishingly small - IMO the worst scenario in practice for both of these is doing extra work, which seems ~fine given that this is already best effort. Cargo already has its own locks for building rustbuild, I'm not worried about that. |
☔ The latest upstream changes (presumably #97513) made this pull request unmergeable. Please resolve the merge conflicts. |
7b66649
to
5a30316
Compare
Rebased. |
@bors r+ rollup=iffy |
📌 Commit 5a30316 has been approved by |
☀️ Test successful - checks-actions |
Finished benchmarking commit (0e21a27): comparison url. Instruction count
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 |
Helps with #94829.