Conversation
(cherry picked from commit ff022d8)
The lookupPathResolved cache was a plain unordered_flat_map with unsynchronized find()/emplace() calls in resolveLookupPathPath(). Multiple threads resolving lookup paths concurrently would race on this cache. Replace it with a concurrent_flat_map following the same pattern used for srcToStore, importResolutionCache, and fileEvalCache. (cherry picked from commit 04516ac)
📝 WalkthroughWalkthroughThis PR converts two internal caches in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/libexpr/eval.cc`:
- Around line 3223-3231: The current concurrent-resolve path uses
getConcurrent(*lookupPathResolved, value) and then
lookupPathResolved->emplace(std::string(value), res) inside finish, which allows
a race where an earlier finisher can insert std::nullopt and later successful
resolution is lost; change the insertion to use the concurrent-aware helper
try_emplace_or_visit() (mirroring fileEvalCache's try_emplace_and_cvisit
pattern) so that if a thread wins the slot with std::nullopt a later successful
resolution can replace or visit that entry appropriately; update the finish
lambda to call try_emplace_or_visit with the std::string(value) key and handle
the visitor callback to replace the stored std::optional<SourcePath> on success.
🪄 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: 5db161b5-39c4-4a0b-bc03-b075f8a30305
📒 Files selected for processing (2)
src/libexpr/eval.ccsrc/libexpr/include/nix/expr/eval.hh
| if (auto cached = getConcurrent(*lookupPathResolved, value)) | ||
| return *cached; | ||
|
|
||
| auto finish = [&](std::optional<SourcePath> res) { | ||
| if (res) | ||
| debug("resolved search path element '%s' to '%s'", value, *res); | ||
| else | ||
| debug("failed to resolve search path element '%s'", value); | ||
| lookupPathResolved.emplace(value, res); | ||
| lookupPathResolved->emplace(std::string(value), res); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /root && find . -name "eval.cc" -path "*/libexpr/*" | head -5Repository: DeterminateSystems/nix-src
Length of output: 125
🏁 Script executed:
# Get context around lines 3223-3231
sed -n '3220,3235p' src/libexpr/eval.ccRepository: DeterminateSystems/nix-src
Length of output: 674
🏁 Script executed:
# Check lookupPathResolved declaration
rg "lookupPathResolved" src/libexpr/eval.cc -B 2 -A 2 | head -40Repository: DeterminateSystems/nix-src
Length of output: 662
🏁 Script executed:
# Find fileEvalCache implementation to compare patterns
rg "fileEvalCache" src/libexpr/eval.cc -B 3 -A 3 | head -60Repository: DeterminateSystems/nix-src
Length of output: 1326
🏁 Script executed:
# Check the concurrent_flat_map type and available methods
rg "concurrent_flat_map" src/libexpr/ -B 1 -A 1Repository: DeterminateSystems/nix-src
Length of output: 2404
🏁 Script executed:
# Search for try_emplace_and_cvisit to understand the comparison
rg "try_emplace_and_cvisit" src/libexpr/eval.cc -B 2 -A 5Repository: DeterminateSystems/nix-src
Length of output: 310
🏁 Script executed:
rg "emplace_or_visit|visit_or_visit" src/libexpr/ -lRepository: DeterminateSystems/nix-src
Length of output: 89
🏁 Script executed:
# Search for Boost concurrent_flat_map method usage patterns in the codebase
rg "concurrent_flat_map" src/libexpr/ -A 5 | grep -E "->|try_emplace|emplace|visit" | head -30Repository: DeterminateSystems/nix-src
Length of output: 180
🏁 Script executed:
# Check what methods are called on concurrent maps in general
ast-grep --pattern 'lookupPathResolved->$_'Repository: DeterminateSystems/nix-src
Length of output: 156
🏁 Script executed:
# Check Boost documentation or comments about concurrent_flat_map methods
rg "concurrent_flat_map|try_emplace_and_cvisit" src/ --type cpp -B 3 -A 8 | head -80Repository: DeterminateSystems/nix-src
Length of output: 5141
🌐 Web query:
Boost concurrent_flat_map emplace_or_visit method API
💡 Result:
In Boost.Unordered boost::unordered::concurrent_flat_map (header #include <boost/unordered/concurrent_flat_map.hpp>), the “emplace_or_visit” operation is provided as try_emplace_or_visit (plus a const-visitation variant try_emplace_or_cvisit). [1]
API (overloads)
template<class... Args, class F>
bool try_emplace_or_visit(const key_type& k, Args&&... args, F&& f);
template<class... Args, class F>
bool try_emplace_or_cvisit(const key_type& k, Args&&... args, F&& f);
template<class... Args, class F>
bool try_emplace_or_visit(key_type&& k, Args&&... args, F&& f);
template<class... Args, class F>
bool try_emplace_or_cvisit(key_type&& k, Args&&... args, F&& f);
template<class K, class... Args, class F>
bool try_emplace_or_visit(K&& k, Args&&... args, F&& f);
template<class K, class... Args, class F>
bool try_emplace_or_cvisit(K&& k, Args&&... args, F&& f);[2]
Semantics
- If key
kis not present: inserts a new element constructed fromkandargs...(piecewise construction); no element is constructed if the key already exists. [2] - If key
kis present: callsf(element_ref)on the existing element instead of inserting. With*_cvisit, the reference passed tofis const. [2] - Returns
trueif an insert happened; otherwisefalse. [2] - Concurrency note: this operation is blocking only on internal rehashing. [2]
- Important restriction: a visitation function executed by a container must not call operations on that same container. [2][3]
Typical use (replacement for operator[]-style “insert or update”)
m.try_emplace_or_visit(key, /*mapped ctor args...*/ 1,
[](auto& kv){ ++kv.second; });This pattern is explicitly called out as a way to replace missing operator[]/at in concurrent_flat_map. [3]
Fix negative-cache poisoning from concurrent resolution race.
This splits the miss path into getConcurrent() and emplace(). If two threads resolve the same entry concurrently and the first finisher inserts std::nullopt on line 3231, a later successful resolution is dropped and lookupPathResolved stays negative-cached for the lifetime of this EvalState. Switch to try_emplace_or_visit() so a success can replace an earlier std::nullopt, matching the pattern already used in fileEvalCache with try_emplace_and_cvisit().
Fix using try_emplace_or_visit
auto finish = [&](std::optional<SourcePath> res) {
if (res)
debug("resolved search path element '%s' to '%s'", value, *res);
else
debug("failed to resolve search path element '%s'", value);
- lookupPathResolved->emplace(std::string(value), res);
+ lookupPathResolved->try_emplace_or_visit(std::string(value), res, [&](auto & kv) {
+ if (res && !kv.second)
+ kv.second = res;
+ res = kv.second;
+ });
return res;
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/libexpr/eval.cc` around lines 3223 - 3231, The current concurrent-resolve
path uses getConcurrent(*lookupPathResolved, value) and then
lookupPathResolved->emplace(std::string(value), res) inside finish, which allows
a race where an earlier finisher can insert std::nullopt and later successful
resolution is lost; change the insertion to use the concurrent-aware helper
try_emplace_or_visit() (mirroring fileEvalCache's try_emplace_and_cvisit
pattern) so that if a thread wins the slot with std::nullopt a later successful
resolution can replace or visit that entry appropriately; update the finish
lambda to call try_emplace_or_visit with the std::string(value) key and handle
the visitor callback to replace the stored std::optional<SourcePath> on success.
Motivation
Cherry-picks upstream NixOS#15488 to make
lookupPathResolvedthread-safe.Also cherry-picks upstream ff022d8. Our
positionToDocCommentwas already thread-safe but the upstream version is (probably) more performant.Fixes #412.
Context
Summary by CodeRabbit
Performance
Bug Fixes