Conversation
Note: this only prevents the paths from being GCed during the copy. You still need to use `nix copy --out-link ...` to prevent GC afterwards. Less ambitious fix than #15719.
Introduce LocalStore::addTempRoots, which adds all temproots in a batch for LocalStore. Make this available from the base class Store by making a virtual method Store::addTempRoots, overriding it in LocalStore and RemoteStore (it is just a loop for each path because the benefit of having this be batched is unclear), and making the single-path Store::addTempRoot a wrapper around that.
Since 734a205, copyPaths() calls addTempRoot() on the remote store for every path in the closure. However, this is extremely show for large closures, especially over high-latency SSH connections. So now, we use a new daemon operation AddTempRoots to do all paths in a single call. Fixes DeterminateSystems#533. Assisted-by: Claude Fable 5 <noreply@anthropic.com>
Use the new batched addTempRoots for both convenience and better performance for LocalStore.
Use the new batched addTempRoots for both convenience and better performance for LocalStore.
github-actions
Bot
added
new-cli
| AddBuildLog = 45, | ||
| BuildPathsWithResults = 46, | ||
| AddPermRoot = 47, | ||
| AddTempRoots = 49, |
There was a problem hiding this comment.
Note: 48 is in use by the nix ps PR.
| /* Fallback for daemons that don't support the batched | ||
| operation. Note that this is very slow for large sets of | ||
| paths on high-latency links, due to a network round-trip per | ||
| path. */ | ||
| for (auto & path : paths) | ||
| conn->addTempRoot(*this, &conn.daemonException, path); |
There was a problem hiding this comment.
Does this mean that older remotes (like nixbuild.net and such) will be unbearably slow before this operation is implemented?
There was a problem hiding this comment.
If you have thousands of paths, yes.
There was a problem hiding this comment.
I guess this is unavoidable for correctness with actual nix remotes, so there isn't much we can do about it. There might be a cheaper way of (temp)rooting a lot of paths though (hacky but might be worth considering) - write a random content addressed store path with a lot of references - it will end up being temprooted because it's a new path and will have references. GC will collect it anyway, because we woudn't make it a permanent root.
4 participants
Motivation
Based on #15719 and DeterminateSystems#482 and DeterminateSystems#538.
copyPaths()now registers the paths being copied as temporary roots on the destination store. However, callingaddTempRoot()sequentially leads to a huge performance regression when copying to a high-latency remote store (i.e.ssh-ng), so we need a batchaddTempRoots()operation in the daemon protocol.Context
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.