Allocate RootValues more efficiently#546
Merged
Merged
Conversation
Previously every RootValue was created with std::allocate_shared<Value *>(traceable_allocator<Value *>()), which costs a GC_MALLOC_UNCOLLECTABLE() / GC_FREE() pair per root value. Uncollectable allocations always take the global GC allocation lock, making this a significant source of mutex contention during parallel evaluation: the eval cache allocates a root value per AttrCursor (i.e. per attribute visited by 'nix search'), and the parallel evaluator allocates one per queued work item. Instead, carve root slots out of large uncollectable slabs (which are permanently part of the GC root set) and recycle them through a free list protected by a plain mutex, whose critical section is a few instructions rather than the entire GC allocator. Slots are cleared on release so they don't keep values alive; liveness semantics are unchanged. The slabs are never freed, so pool memory is bounded by the peak number of simultaneously live root values (8 bytes per slot). At 16 eval cores (with GC disabled via GC_INITIAL_HEAP_SIZE=40G), this cuts kernel time from ~8-9s to ~7s, context switches from 1.46M to 0.99M, and elapsed time from ~4.9s to ~4.3-4.5s for 'nix search nixpkgs --no-eval-cache fizzbuzz'. Assisted-by: Claude Fable 5 <noreply@anthropic.com>
RootValue (std::shared_ptr<Value *>) adds avoidable overhead per GC
root: a malloc'd control block, atomic reference count updates on
copies (a measured source of cache line contention during parallel
evaluation), and 16 bytes per handle. Most users never copy their
roots, so introduce UniqueRootValue, a move-only RAII wrapper around
a pointer to a root slot that returns the slot to the pool on
destruction.
The pool internals move from allocRootValue() into
allocRootValueSlot() / freeRootValueSlot(), shared by both handle
types, and the whole root value machinery moves into new files
root-value.{hh,cc}. The non-Boehm fallback now also goes through the
slot functions (new/delete) instead of a make_shared special case.
Converted to UniqueRootValue: ValMap, fileEvalCache, internalPrimOps,
genericClosure's work/result lists, EvalCache::value,
AttrCursor::_value, InstallableAttrPath::v, and the JSON parser
state. RootValue remains (documented as such) for roots captured in
std::function-backed lambdas, which require copyability.
Assisted-by: Claude Fable 5 <noreply@anthropic.com>
Replace the Sync<std::vector<Value **>> free slot pool with an intrusive linked list: each slot is a union of Value * (in use) and a pointer to the next free slot, with a Sync<Slot *> head. Allocation pops the head; freeing pushes in O(1). This avoids the auxiliary vector memory (8 bytes per free slot), the 4096-iteration push_back loop when carving a new slab, and most importantly the possibility of a vector reallocation (a malloc) while holding the pool lock on the free path. GC safety is unchanged: slabs remain uncollectable and conservatively scanned. In-use slots contain a Value * and root the value; free slots contain a pointer into a slab (or null), which is scanned harmlessly since slabs are never freed anyway. Writing the next pointer on free overwrites the Value *, which doubles as the "stop rooting the value" step. Assisted-by: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaces the shared_ptr-based ChangesUniqueRootValue migration
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/libexpr/eval.cc`:
- Around line 1165-1171: The cache-hit fast path in eval.cc bypasses the
direct-attrset validation and can return a value cached under a looser
mustBeTrivial setting, so update the file-eval cache handling in the
resolvedPath lookup path to preserve the mustBeTrivial contract. Either
incorporate mustBeTrivial into the fileEvalCache key used by the cvisit/lookup
logic, or re-run the same triviality check before accepting a cached Value in
the eval path that assigns v from v2.
In `@src/libexpr/include/nix/expr/get-drvs.hh`:
- Around line 35-36: PackageInfo is storing raw Bindings pointers for
attrs/meta, but the backing Value needs to be rooted instead to keep the attrset
alive. Update the PackageInfo storage and related accessors to hold a rooted
Value or equivalent bindings-aware handle, and derive attrs/meta from that
rooted value rather than trying to apply UniqueRootValue directly to the pointer
fields. Use PackageInfo and its attrs/meta members as the main symbols when
making the change.
In `@src/libexpr/include/nix/expr/root-value.hh`:
- Around line 50-55: Guard UniqueRootValue::operator=(UniqueRootValue&&) against
self-move assignment by checking whether this and other are the same object
before freeing the current slot. If they are identical, return immediately;
otherwise release the existing slot, then transfer other.slot with std::exchange
as before so x = std::move(x) cannot return the same slot to the pool and leave
the object holding a freed slot.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 80257c66-152f-4579-9f5e-d265eae8f244
📒 Files selected for processing (16)
src/libcmd/include/nix/cmd/installable-attr-path.hhsrc/libcmd/installable-attr-path.ccsrc/libcmd/repl.ccsrc/libexpr/eval-cache.ccsrc/libexpr/eval.ccsrc/libexpr/include/nix/expr/eval-cache.hhsrc/libexpr/include/nix/expr/eval.hhsrc/libexpr/include/nix/expr/get-drvs.hhsrc/libexpr/include/nix/expr/meson.buildsrc/libexpr/include/nix/expr/root-value.hhsrc/libexpr/include/nix/expr/value.hhsrc/libexpr/json-to-value.ccsrc/libexpr/meson.buildsrc/libexpr/primops.ccsrc/libexpr/primops/fetchTree.ccsrc/libexpr/root-value.cc
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
cole-h
approved these changes
Jul 8, 2026
This was referenced Jul 8, 2026
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
2 participants
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
Taken from #534.
RootValues were expensive for parallel eval, since eachRootValuecaused a Boehm GC call to allocate traceable-but-uncollectable memory while holding the global GC root. We now allocate slab of uncollectable memory from which theRootValues are allocated.Also, there is a new
UniqueRootValuetype which is likeRootValueexcept that it isn't wrapped in anstd::shared_ptr, so it entirely avoids a heap allocation. Most instances oftraceable_allocator<Value *>are now aUniqueRootValue.Speedup:
Context
Summary by CodeRabbit
Bug Fixes
Chores