libstore: reuse parsed derivations in registerValidPaths#14828
libstore: reuse parsed derivations in registerValidPaths#14828Mic92 merged 4 commits intoNixOS:masterfrom
Conversation
github-actions
bot
added
the
store
|
@Zaczero just so you know, stepping back a bit, a few of us (@Mic92 and I at least) have considered whether we ought to put derivations in SQLite rather than serialize them to If you are interested in doing performance work around derivations, perhaps you would want take that on? |
|
@Ericson2314 I don't know if I want to make bigger changes to Nix yet. I am getting familiarized with its source, and I wanted to focus on doable existing FIXMEs/TODOs. |
src/libstore/local-store.cc
Outdated
| not be valid yet. */ | ||
| for (auto & [_, i] : infos) | ||
| if (i.path.isDerivation()) { | ||
| // FIXME: inefficient; we already loaded the derivation in addValidPath(). | ||
| readInvalidDerivation(i.path).checkInvariants(*this, i.path); | ||
| if (auto drv = derivations.find(i.path); drv != derivations.end()) { | ||
| drv->second.checkInvariants(*this, i.path); | ||
| } else { | ||
| readInvalidDerivation(i.path).checkInvariants(*this, i.path); | ||
| } | ||
| } |
There was a problem hiding this comment.
Do we even need this code at all? isn't addValidPath already calling checkInvariants? The code (before you did anything, to be clear) seems redundant to me.
There was a problem hiding this comment.
It appears to be the other way around. addValidPath is never called with checkOutputs=true so it never checkInvariants.
There was a problem hiding this comment.
I think you are right. The comment was out-dated.
There was a problem hiding this comment.
There was a problem hiding this comment.
Only if this comment no longer applies:
/* Check that the derivation outputs are correct. We can't do
this in addValidPath() above, because the references might
not be valid yet. */
There was a problem hiding this comment.
@Zaczero check the commit message, for why I think this is not longer the case.
There was a problem hiding this comment.
Looks good, I didn't notice the big commit message 🤓
- Add a focused nix-store-benchmarks benchmark that registers many derivation paths into a temporary local store root
- LocalStore::registerValidPaths() parsed derivations twice: once in addValidPath() and again when calling checkInvariants(), despite already having loaded the derivation. - Plumb the parsed Derivation out of addValidPath() and reuse it for the invariant check pass, falling back to re-parsing only when a derivation wasn’t newly registered in this call. - BM_RegisterValidPathsDerivations/200_mean runs 32% faster
Zaczero
force-pushed
the
zaczero/libstore-registerValidPaths
branch
from
97c1b49 to
cccfa38
Compare
Mic92
force-pushed
the
zaczero/libstore-registerValidPaths
branch
2 times, most recently
from
731c718 to
cccfa38
Compare
|
Sorry. Didn't meant to force push. Don't know how that happend. I reverted back to your change. |
…riants loop The separate checkInvariants loop after addValidPath was added in 2014 (d210cdc) to work around an assertion failure: nix-store: derivations.cc:242: Assertion 'store.isValidPath(i->first)' failed. At that time, hashDerivationModulo() contained assert(store.isValidPath(...)) which required input derivations to be registered as valid in the database before computing their hash. The workaround was to: 1. Call addValidPath with checkOutputs=false 2. Add all references to the database 3. Run checkInvariants in a separate loop after paths were valid In 2020 (bccff82), the isValidPath assertion was removed to fix a deadlock in IFD through the daemon (issue NixOS#4235). The fix changed hashDerivationModulo to use readInvalidDerivation, which reads directly from the filesystem without requiring database validity. This made the separate checkInvariants loop unnecessary, but nobody noticed the code could be simplified. The comment "We can't do this in addValidPath() above, because the references might not be valid yet" became stale. Now we simply call addValidPath() with the default checkOutputs=true, which runs checkInvariants internally using the already-parsed derivation. This commit eliminates the separate loop over derivations.
Testing with 10 derivations is sufficient to verify performance characteristics. The larger test cases (50, 200) don't provide additional insight and slow down the benchmark unnecessarily.
3 participants
Motivation
Context
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.