-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Implement fine grain locking for build-dir
#16155
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
src/cargo/core/compiler/locking.rs
Outdated
| let primary_lock = open_file(&self.primary)?; | ||
| primary_lock.lock()?; | ||
|
|
||
| let secondary_lock = open_file(&self.secondary)?; | ||
| secondary_lock.lock()?; | ||
|
|
||
| self.guard = Some(UnitLockGuard { | ||
| primary: primary_lock, | ||
| _secondary: Some(secondary_lock), | ||
| }); | ||
| Ok(()) |
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.
Have we double checked if we run into problems like #15698?
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.
I took a closer look and it appears that there is a possibility that a similar issue could happen.
I think in practice it would not dead lock since failing to take a lock would result in the build failing, so the lock would be released when the process exits.
But regardless, I went ahead and added logic to unlock the partial lock if we fail to take the full lock just incase.
b234db5 to
1deca86
Compare
4ace282 to
1a81371
Compare
39d1cee to
c75a617
Compare
| Ok(key) | ||
| } | ||
|
|
||
| pub fn lock(&self, key: &LockKey) -> CargoResult<()> { |
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.
Would it work to also have a Blocking message for this?
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.
I looked into this and I think it should be possible. We don't have direct access to GlobalContext inside of JobState.
However, we do have indirect access through DiagDedupe.
I think we can modify the Filesystem functions to accept a trait like ReportBlocking and implement that trait for Shell and DiagDedupe.
Though perhaps we defer that to a follow up PR?
c75a617 to
eee8ef7
Compare
eee8ef7 to
4235aa2
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
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.
Thanks!
|
The PR description is up-to-date, right? |
|
As much as I'm aware, yes. I did a pass yesterday and moved the historical section down further to not draw as much attention to it. |
Update cargo submodule 9 commits in 94c368ad2b9db0f0da5bdd8421cea13786ce4412..b54051b1505281ec7a45a250140a0ff25d33f319 2025-12-26 19:39:15 +0000 to 2025-12-30 20:35:52 +0000 - fix(log): add `dependencies` field to `UnitRegistered` (rust-lang/cargo#16448) - Implement fine grain locking for `build-dir` (rust-lang/cargo#16155) - feat(resolver): List features when no close match (rust-lang/cargo#16445) - feat(report): new command `cargo report sessions` (rust-lang/cargo#16428) - feat (patch): Display where the patch was defined in patch-related error messages (rust-lang/cargo#16407) - test(build-rs): Reduce from 'build' to 'check' where possible (rust-lang/cargo#16444) - feat(toml): TOML 1.1 parse support (rust-lang/cargo#16415) - feat(report): support --manifest-path in `cargo report timings` (rust-lang/cargo#16441) - fix(vendor): recursively filter git files in subdirectories (rust-lang/cargo#16439) r? ghost
Update cargo submodule 19 commits in 94c368ad2b9db0f0da5bdd8421cea13786ce4412..623444427f04292f3a3c7eac560cfa65ba0bb88e 2025-12-26 19:39:15 +0000 to 2026-01-06 21:05:05 +0000 - docs(unstable): expand docs for `-Zbuild-analysis` (rust-lang/cargo#16476) - test: add `-Zunstable-options` with custom targets (rust-lang/cargo#16467) - feat(report): add cargo report rebuilds (rust-lang/cargo#16456) - feat(test-support): Use test name for dir when running tests (rust-lang/cargo#16121) - refactor: Migrate some cases to expect/reason (rust-lang/cargo#16461) - docs(build-script): clarify OUT_DIR is not cleaned between builds (rust-lang/cargo#16437) - chore: Update dependencies (rust-lang/cargo#16460) - Update handlebars to 6.4.0 (rust-lang/cargo#16457) - chore(deps): update alpine docker tag to v3.23 (rust-lang/cargo#16454) - Any build scripts can now use cargo::metadata=KEY=VALUE (rust-lang/cargo#16436) - fix(log): add `dependencies` field to `UnitRegistered` (rust-lang/cargo#16448) - Implement fine grain locking for `build-dir` (rust-lang/cargo#16155) - feat(resolver): List features when no close match (rust-lang/cargo#16445) - feat(report): new command `cargo report sessions` (rust-lang/cargo#16428) - feat (patch): Display where the patch was defined in patch-related error messages (rust-lang/cargo#16407) - test(build-rs): Reduce from 'build' to 'check' where possible (rust-lang/cargo#16444) - feat(toml): TOML 1.1 parse support (rust-lang/cargo#16415) - feat(report): support --manifest-path in `cargo report timings` (rust-lang/cargo#16441) - fix(vendor): recursively filter git files in subdirectories (rust-lang/cargo#16439) r? ghost
This PR adds fine grain locking for the build cache using build unit level locking.
I'd recommend reading the design details in this description and then reviewing commit by commit.
Part of #4282
Previous attempt: #16089
Design decisions / rational
build-dir/<profile>/.cargo-lockcargo clean(exclusive)build-dir#16155 (comment)For the rational for this design see the discussion #t-cargo > Build cache and locking design @ 💬
Open Questions
build-dir#16155 (comment)build-dir#16155 (comment))cargo doccargo checkwould not take a lock artifact-dir butcargo buildwould). This would mean that 2cargo buildinvocations would not run in parallel because one of them would hold the lock artifact-dir (blocking the other). This might actually be ideal to avoid 2 instances fighting over the CPU while recompiling the same crates.-Zbuild-dir-new-layout. With the current implementation I am not seeing any perf regression on linux but I have yet to test on windows/macos.Original Design
Implementation details
primary.lockandsecondary.lock(seelocking.rsmodule docs for more details on the states)determine_locking_mode()).cargo-locklock as RO shared to continue working with older cargo versions while allowing multiple newer cargo instances to run in parallel.cargo cleancontinues to use coarse grain locking for simplicity.UnitGraph.number of build units * 10) we automatically fallback to coarse grain locking and display a warning to the user.RcFileLockI was seeing peaks of ~12,000 open fds which I felt was quiet high even for a large project like Zed.FileLockInternerthat holds on to the file descriptors (RcFileLock) until the end of the process. (We could potentially add it toJobStateif preferred, it would just be a bit more plumbing)See #16155 (comment) for proposal to transition away from this to the current scheme