Repository navigation
gc: don't hold the roots lock while marking a closure alive - #769
Open
czawistowski-nvidia wants to merge 1 commit into
Open
czawistowski-nvidia wants to merge 1 commit into
czawistowski-nvidia wants to merge 1 commit into
Conversation
When the collector finds a temporary root, it computes the root's closure (`markAlive()`) while still holding the lock on its shared state. The thread that receives new temporary roots from other processes takes that same lock, so every `addTempRoot()` in the system (evaluations, builds, `nix copy`) waits until the closure has been computed, which takes longer the larger the closure is. Release the lock before calling `markAlive()`, both in the referrers walk and when a path became a temporary root before its deletion. The lock is only needed to look up the root and to set `pending`. Add a synchronisation point and a test that registers a temporary root while the collector is inside `markAlive()`.
|
Warning Review limit reached
This review includes 3 billable files and costs up to $0.75. Or wait 59 minutes for your next included review. View limit details
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
While the garbage collector computes the closure of a temporary root, every other process that registers a temporary root waits for it.
When the referrers walk in
collectGarbage()reaches a temporary root, it callsmarkAlive(), which computes the root's whole closure, while still holding the lock on the collector's shared state. The thread that receives new temporary roots from other processes takes the same lock before it acknowledges a root. SoaddTempRoot()in every evaluation, build ornix copyon the machine blocks until that closure has been computed, and the larger the closure, the longer they wait. The same happens in the deletion loop when a path became a temporary root after the walk.markAlive()computes the closure from scratch with one database query per path (see NixOS#15712), so the wait grows with the size of the root's closure, e.g. an evaluation's.drvclosure or a system closure that is only held by a temporary root.Change
Look up the temporary root and set
pendingunder the lock as before, then release it before callingmarkAlive().markAlive()only touches state that the collector thread owns (alive, the database), so it doesn't need the lock.For the test, this adds a
_NIX_TEST_GC_SYNC_3synchronisation point at the firstmarkAlive()call, next to the existing_NIX_TEST_GC_SYNC_1/2.Testing
New functional test
gc-mark-alive-unlocked.sh: it starts a build so that its.drvis held only by a temporary root, runsnix-store --gcuntil it stops inmarkAlive()for that.drv, and then runsnix-store --addunder a 20 s timeout.Without the change (with only the synchronisation point added) the test fails:
nix-store --addblocks until the timeout.With it:
gc-mark-alive-unlocked(10 of 10 sequential runs),gc,gc-auto,gc-closure,gc-concurrent,gc-non-blocking,gc-runtime,nix-collect-garbage-d,export-graph,check-reqsandnix-store-testspass.How long a client waits, measured on current
mainwith and without this change (onlylibnixstoreswapped). Setup: a local store on tmpfs with a synthetic closure, where each path refers to the previous one and up to 7 random earlier ones, held only by a temporary root (builtins.storePathin a waiting evaluation).nix-store --gcruns while another process sends temporary roots over the GC socket back to back and times each acknowledgement. Reported: the longest wait per GC run, n = 10 runs per variant, interleaved, on a shared and loaded VM.Before the change, the wait is about as long as
markAlive()takes; at 20,000 paths that is most of the 1.5 s GC run. A store whose database is on disk, rather than tmpfs, would be slower (not measured). The GC's own run time is unchanged (p50 1.5 s both ways at 20,000 paths).