Conversation
Adding paths to the store can be slow due to I/O overhead, but especially when going through the daemon because of the round-trip latency of every wopAddToStore call. So we now do the addToStore() calls asynchronously from a separate thread from the evaluator. This slightly speeds up the local store, and makes going through the daemon almost as fast as a local store.
github-actions
bot
added
new-cli
|
Shouldn't we just send the daemon a file path or file descriptor and then it can add the data itself? I would think serializing local files over NARs to be unpacked by the daemon (on the same system) is bad performance whether synchronous or asynchronous, compared to letting the OS move the files around. |
|
@edef1c reminds me of https://en.wikipedia.org/wiki/Confused_deputy_problem, so yes it should be a file descriptor, not path, to cleanly avoid all these problems. (The way this PR does it is still good for the ssh/non-local case, to be clear.) |
Are there not also remote daemons where this doesn't work? There this eval store option. |
|
cc @NaN-git |
xokdvium
left a comment
xokdvium
left a comment
There was a problem hiding this comment.
At this point we could just look into porting the kj-async based refactoring to the store layer that Lix has done. Coroutines seem very fitting for this sort of thing. It would require more up-front effort, but it seems worthwhile in the long run. Doing ad-hoc async is going to become more painful as we have more of it.
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2025-08-20-nix-team-meeting-minutes-242-241/68245/1 |
| // FIXME: use addMultipeToStore() once it doesn't require a | ||
| // NAR hash from the client for CA objects. | ||
|
|
||
| for (auto & item : items) { | ||
| StringSource source(item.contents); | ||
| auto storePath = store->addToStoreFromDump( | ||
| source, | ||
| item.storePath.name(), | ||
| FileSerialisationMethod::Flat, | ||
| ContentAddressMethod::Raw::Text, | ||
| HashAlgorithm::SHA256, | ||
| item.references, | ||
| item.repair); | ||
| assert(storePath == item.storePath); | ||
| } |
There was a problem hiding this comment.
As we discussed briefly in the meeting, we could instead implement pipelining, which does not require an update to the worker protocol.
Another benefit of pipelining that was not brought up is that it also allows the first response to come in before any of the other requests, reducing the latency of individual add operations.
In principle yes, but converting the whole evaluator to coroutines might not be the right thing to do, or at least not the first thing to do. I do agree that an async refactor would benefit the code a lot, just by virtue of getting to use better control flow primitives/abstractions than what |
Something we should discuss in a team meeting I think, so that we are all aligned on it. |
6 participants
Motivation
Adding paths to the store can be slow due to I/O overhead, but especially when going through the daemon because of the round-trip latency of every
wopAddToStorecall.So we now do the
addToStore()calls asynchronously from a separate thread from the evaluator. This slightly speeds up the local store, and makes going through the daemon almost as fast as a local store.Timings doing
nix eval github:NixOS/hydra/b812bb5017cac055fa56ffeac5440b6365830d67#nixosConfigurations.container.config.system.build.toplevel:The async path writer is currently only used by
writeDerivation()(i.e. for adding.drvfiles) but can be extended in the future to handle other source files.Includes DeterminateSystems#162 and DeterminateSystems#176.
Context
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.