Move locking from bootstrap.py to rust bootstrap, using fd-lock#98373
Move locking from bootstrap.py to rust bootstrap, using fd-lock#98373bors merged 2 commits intorust-lang:masterfrom
Conversation
|
(rust-highfive has picked a reviewer for you, use r? to override) |
16ae306 to
be41698
Compare
jyn514
left a comment
There was a problem hiding this comment.
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.