-
Notifications
You must be signed in to change notification settings - Fork 2.7k
(experiment) Fine grain locking #16089
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
base: master
Are you sure you want to change the base?
(experiment) Fine grain locking #16089
Conversation
How do you plan to handle deadlocks? EDIT: Though thinking more... Probably not an issue. I was thinking of cycles, but maybe dev-dep cycles will have a different hash? |
Yeah, my assumption is that there would be no cycles in the unit graph, so if unit is scheduled to run all of it's dependencies have already been built and their locks had been released. |
This comment has been minimized.
This comment has been minimized.
64e5bf3
to
f221f5e
Compare
f221f5e
to
3fc0143
Compare
I reverted the multiple locks per build unit approach for now. Plan to discuss more about the path forward in the next Cargo team meeting. |
What does this PR try to resolve?
This is an experiment at adding fine grain locking (at a build unit level) during compilation.
With #15947 merged, this unblocks us to start experimenting with more granular locking tracked in #4282
The primary goal of this PR is to evaluate locking schemes and review their trades offs (i.e. performance, complexity, etc)
Implementation approach / details
The approach is to add a
lock
file to each build unit dir (build-dir/<profile>/build/<pkg>/<hash>/lock
) and acquire an exclusive lock during the compilation of that unit as well as a shared lock of all of its dependencies. These locks are taken usingstd::fs::File::{lock, lock_shared}
.For this experiment, I found it easier to create the locking from scratch rather than re-using the using locking systems in
Filesystem
andCacheLocker
as their interfaces requiregctx
which is out of scope during the actual compilation phase passed toWork::new()
. (and plumbinggctx
into it, while possible was a bit annoying due to lifetime issues)I encapsulated all of the locking logic into
CompilationLock
inlocking.rs
.Note: For now I simply reused the
-Zbuild-dir-new-layout
flag to enable fine grain locking, though we may want a stand alone flag for this in the future.Benchmarking and experimenting
After verifying that the compilation functionality is working, I did some basic benchmarks with hyperfine on a test crate with about ~200 total dependencies to represent a basic small to medium sized crate. Bench marks were run on a Fedora linux x86 machine with a 20 core CPU.
Cargo.toml
(I didn't a lot of thought into the specific dependencies. I simply grabbed some crates a new that had a good amount of transitive dependencies so I did not need at a lot of dependencies manually.)
Results:
From the results above we can see we are taking nearly a ~10% performance hit due to the locking overhead. Which is quiet bad IMO...
Out of curiosity, I also tried taking the shared locks in parallel using
rayon
's.par_iter()
to see if that would improve the situation.Code Change
However we can see this did really improve it by much if at all.
Another idea I had was to see if taking a lock on the build unit directory (
build-dir/<profile>/build/<pkg>/<hash>
) directly instead of writing a dedicated lock file would have any effect. However, this also had minimal if any improvement compared to using a standalone file.I also benchmarked with a larger project with about ~750 dependencies to see how the changes scale with large projects.
Note: This is without rayon and using the
lock
file setup from the first benchmark above.Cargo.toml
Other observations
I also ran a baseline to make sure the performance loss was not coming from layout restructuring (as opposed to adding locking) by running the same bench with out the locking changes. (built from commit 81c3f77)