From 7f9b5226af81607fac499807140632b4a59e598e Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Sun, 7 Sep 2025 16:54:39 +0200 Subject: [PATCH 1/6] Add getConcurrent helper function --- src/libutil/include/nix/util/util.hh | 11 +++++++++++ src/libutil/posix-source-accessor.cc | 4 +--- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/libutil/include/nix/util/util.hh b/src/libutil/include/nix/util/util.hh index 561550c41..a20305a6f 100644 --- a/src/libutil/include/nix/util/util.hh +++ b/src/libutil/include/nix/util/util.hh @@ -218,6 +218,17 @@ typename T::mapped_type * get(T & map, K & key) template typename T::mapped_type * get(T && map, const typename T::key_type & key) = delete; +/** + * Look up a value in a `boost::concurrent_flat_map`. + */ +template +std::optional getConcurrent(const T & map, const typename T::key_type & key) +{ + std::optional res; + map.cvisit(key, [&](auto & x) { res = x.second; }); + return res; +} + /** * Get a value for the specified key from an associate container, or a default value if the key isn't present. */ diff --git a/src/libutil/posix-source-accessor.cc b/src/libutil/posix-source-accessor.cc index 877c63331..c524f3e4f 100644 --- a/src/libutil/posix-source-accessor.cc +++ b/src/libutil/posix-source-accessor.cc @@ -95,9 +95,7 @@ std::optional PosixSourceAccessor::cachedLstat(const CanonPath & pa // former is not hashable on libc++. Path absPath = makeAbsPath(path).string(); - std::optional res; - cache.cvisit(absPath, [&](auto & x) { res.emplace(x.second); }); - if (res) + if (auto res = getConcurrent(cache, absPath)) return *res; auto st = nix::maybeLstat(absPath.c_str()); From 8fbf4b94279765d5b5cf835c35bd0308de3d1cc6 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Mon, 8 Sep 2025 08:07:34 +0200 Subject: [PATCH 2/6] CanonPath: Implement boost::hash --- src/libutil/include/nix/util/canon-path.hh | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/libutil/include/nix/util/canon-path.hh b/src/libutil/include/nix/util/canon-path.hh index 334c9e332..dd07929b4 100644 --- a/src/libutil/include/nix/util/canon-path.hh +++ b/src/libutil/include/nix/util/canon-path.hh @@ -8,6 +8,8 @@ #include #include +#include + namespace nix { /** @@ -258,11 +260,17 @@ public: */ std::string makeRelative(const CanonPath & path) const; - friend struct std::hash; + friend std::size_t hash_value(const CanonPath &); }; std::ostream & operator<<(std::ostream & stream, const CanonPath & path); +inline std::size_t hash_value(const CanonPath & path) +{ + boost::hash hasher; + return hasher(path.path); +} + } // namespace nix template<> @@ -270,8 +278,8 @@ struct std::hash { using is_avalanching = std::true_type; - std::size_t operator()(const nix::CanonPath & s) const noexcept + std::size_t operator()(const nix::CanonPath & path) const noexcept { - return std::hash{}(s.path); + return nix::hash_value(path); } }; From 47c16fc4bd13c52cebdb3c61597a31bc0df14216 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Mon, 8 Sep 2025 08:07:45 +0200 Subject: [PATCH 3/6] SourcePath: Implement boost::hash --- src/libutil/include/nix/util/source-path.hh | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/libutil/include/nix/util/source-path.hh b/src/libutil/include/nix/util/source-path.hh index f7cfc8ef7..08f9fe580 100644 --- a/src/libutil/include/nix/util/source-path.hh +++ b/src/libutil/include/nix/util/source-path.hh @@ -119,15 +119,23 @@ struct SourcePath std::ostream & operator<<(std::ostream & str, const SourcePath & path); +inline std::size_t hash_value(const SourcePath & path) +{ + std::size_t hash = 0; + boost::hash_combine(hash, path.accessor->number); + boost::hash_combine(hash, path.path); + return hash; +} + } // namespace nix template<> struct std::hash { + using is_avalanching = std::true_type; + std::size_t operator()(const nix::SourcePath & s) const noexcept { - std::size_t hash = 0; - hash_combine(hash, s.accessor->number, s.path); - return hash; + return nix::hash_value(s); } }; From ad6eb22368c27562152d5bf0e7c2fe5b6fac2d47 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Mon, 8 Sep 2025 08:24:40 +0200 Subject: [PATCH 4/6] Ensure that files are parsed/evaluated only once When doing multithreaded evaluation, we want to ensure that any Nix file is parsed and evaluated only once. The easiest way to do this is to rely on thunks, since those ensure locking in the multithreaded evaluator. `fileEvalCache` is now a mapping from `SourcePath` to a `Value *`. The value is initially a thunk (pointing to a `ExprParseFile` helper object) that can be forced to parse and evaluate the file. So a subsequent thread requesting the same file will see a thunk that is possibly locked and wait for it. The parser cache is gone since it's no longer needed. However, there is a new `importResolutionCache` that maps `SourcePath`s to `SourcePath`s (e.g. `/foo` to `/foo/default.nix`). Previously we put multiple entries in `fileEvalCache`, which was ugly and could result in work duplication. --- src/libexpr/eval.cc | 159 ++++++++++++++++++--------- src/libexpr/include/nix/expr/eval.hh | 29 ++--- 2 files changed, 113 insertions(+), 75 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 4fe9e9e3a..5629865f0 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -38,6 +38,7 @@ #include #include +#include #include "nix/util/strings-inline.hh" @@ -192,6 +193,27 @@ static Symbol getName(const AttrName & name, EvalState & state, Env & env) static constexpr size_t BASE_ENV_SIZE = 128; +struct EvalState::SrcToStore +{ + boost::concurrent_flat_map inner; +}; + +struct EvalState::ImportResolutionCache +{ + boost::concurrent_flat_map inner; +}; + +struct EvalState::FileEvalCache +{ + boost::concurrent_flat_map< + SourcePath, + Value *, + std::hash, + std::equal_to, + traceable_allocator>> + inner; +}; + EvalState::EvalState( const LookupPath & lookupPathFromArguments, ref store, @@ -264,6 +286,9 @@ EvalState::EvalState( , debugRepl(nullptr) , debugStop(false) , trylevel(0) + , srcToStore(make_ref()) + , importResolutionCache(make_ref()) + , fileEvalCache(make_ref()) , regexCache(makeRegexCache()) #if NIX_USE_BOEHMGC , valueAllocCache(std::allocate_shared(traceable_allocator(), nullptr)) @@ -1031,63 +1056,85 @@ Value * ExprPath::maybeThunk(EvalState & state, Env & env) return &v; } +/** + * A helper `Expr` class to lets us parse and evaluate Nix expressions + * from a thunk, ensuring that every file is parsed/evaluated only + * once (via the thunk stored in `EvalState::fileEvalCache`). + */ +struct ExprParseFile : Expr +{ + SourcePath & path; + bool mustBeTrivial; + + ExprParseFile(SourcePath & path, bool mustBeTrivial) + : path(path) + , mustBeTrivial(mustBeTrivial) + { + } + + void eval(EvalState & state, Env & env, Value & v) override + { + printTalkative("evaluating file '%s'", path); + + auto e = state.parseExprFromFile(path); + + try { + auto dts = + state.debugRepl + ? makeDebugTraceStacker( + state, *e, state.baseEnv, e->getPos(), "while evaluating the file '%s':", path.to_string()) + : nullptr; + + // Enforce that 'flake.nix' is a direct attrset, not a + // computation. + if (mustBeTrivial && !(dynamic_cast(e))) + state.error("file '%s' must be an attribute set", path).debugThrow(); + + state.eval(e, v); + } catch (Error & e) { + state.addErrorTrace(e, "while evaluating the file '%s':", path.to_string()); + throw; + } + } +}; + void EvalState::evalFile(const SourcePath & path, Value & v, bool mustBeTrivial) { - FileEvalCache::iterator i; - if ((i = fileEvalCache.find(path)) != fileEvalCache.end()) { - v = i->second; + auto resolvedPath = getConcurrent(importResolutionCache->inner, path); + + if (!resolvedPath) { + resolvedPath = resolveExprPath(path); + importResolutionCache->inner.emplace(path, *resolvedPath); + } + + if (auto v2 = getConcurrent(fileEvalCache->inner, *resolvedPath)) { + forceValue(**v2, noPos); + v = **v2; return; } - auto resolvedPath = resolveExprPath(path); - if ((i = fileEvalCache.find(resolvedPath)) != fileEvalCache.end()) { - v = i->second; - return; - } + Value * vExpr; + ExprParseFile expr{*resolvedPath, mustBeTrivial}; - printTalkative("evaluating file '%1%'", resolvedPath); - Expr * e = nullptr; + fileEvalCache->inner.try_emplace_and_cvisit( + *resolvedPath, + nullptr, + [&](auto & i) { + vExpr = allocValue(); + vExpr->mkThunk(&baseEnv, &expr); + i.second = vExpr; + }, + [&](auto & i) { vExpr = i.second; }); - auto j = fileParseCache.find(resolvedPath); - if (j != fileParseCache.end()) - e = j->second; + forceValue(*vExpr, noPos); - if (!e) - e = parseExprFromFile(resolvedPath); - - fileParseCache.emplace(resolvedPath, e); - - try { - auto dts = debugRepl ? makeDebugTraceStacker( - *this, - *e, - this->baseEnv, - e->getPos(), - "while evaluating the file '%1%':", - resolvedPath.to_string()) - : nullptr; - - // Enforce that 'flake.nix' is a direct attrset, not a - // computation. - if (mustBeTrivial && !(dynamic_cast(e))) - error("file '%s' must be an attribute set", path).debugThrow(); - eval(e, v); - } catch (Error & e) { - addErrorTrace(e, "while evaluating the file '%1%':", resolvedPath.to_string()); - throw; - } - - fileEvalCache.emplace(resolvedPath, v); - if (path != resolvedPath) - fileEvalCache.emplace(path, v); + v = *vExpr; } void EvalState::resetFileCache() { - fileEvalCache.clear(); - fileEvalCache.rehash(0); - fileParseCache.clear(); - fileParseCache.rehash(0); + fileEvalCache->inner.clear(); + fileEvalCache->inner.rehash(0); inputCache->clear(); } @@ -2372,9 +2419,10 @@ StorePath EvalState::copyPathToStore(NixStringContext & context, const SourcePat if (nix::isDerivation(path.path.abs())) error("file names are not allowed to end in '%1%'", drvExtension).debugThrow(); - std::optional dstPath; - if (!srcToStore.cvisit(path, [&dstPath](const auto & kv) { dstPath.emplace(kv.second); })) { - dstPath.emplace(fetchToStore( + auto dstPathCached = getConcurrent(srcToStore->inner, path); + + auto dstPath = dstPathCached ? *dstPathCached : [&]() { + auto dstPath = fetchToStore( fetchSettings, *store, path.resolveSymlinks(SymlinkResolution::Ancestors), @@ -2382,14 +2430,15 @@ StorePath EvalState::copyPathToStore(NixStringContext & context, const SourcePat path.baseName(), ContentAddressMethod::Raw::NixArchive, nullptr, - repair)); - allowPath(*dstPath); - srcToStore.try_emplace(path, *dstPath); - printMsg(lvlChatty, "copied source '%1%' -> '%2%'", path, store->printStorePath(*dstPath)); - } + repair); + allowPath(dstPath); + srcToStore->inner.try_emplace(path, dstPath); + printMsg(lvlChatty, "copied source '%1%' -> '%2%'", path, store->printStorePath(dstPath)); + return dstPath; + }(); - context.insert(NixStringContextElem::Opaque{.path = *dstPath}); - return *dstPath; + context.insert(NixStringContextElem::Opaque{.path = dstPath}); + return dstPath; } SourcePath EvalState::coerceToPath(const PosIdx pos, Value & v, NixStringContext & context, std::string_view errorCtx) diff --git a/src/libexpr/include/nix/expr/eval.hh b/src/libexpr/include/nix/expr/eval.hh index 9e0638de8..4b294ad9a 100644 --- a/src/libexpr/include/nix/expr/eval.hh +++ b/src/libexpr/include/nix/expr/eval.hh @@ -20,7 +20,6 @@ // For `NIX_USE_BOEHMGC`, and if that's set, `GC_THREADS` #include "nix/expr/config.hh" -#include #include #include #include @@ -412,31 +411,21 @@ private: /* Cache for calls to addToStore(); maps source paths to the store paths. */ - boost::concurrent_flat_map> srcToStore; + struct SrcToStore; + ref srcToStore; /** - * A cache from path names to parse trees. + * A cache that maps paths to "resolved" paths for importing Nix + * expressions, i.e. `/foo` to `/foo/default.nix`. */ - typedef boost::unordered_flat_map< - SourcePath, - Expr *, - std::hash, - std::equal_to, - traceable_allocator>> - FileParseCache; - FileParseCache fileParseCache; + struct ImportResolutionCache; + ref importResolutionCache; /** - * A cache from path names to values. + * A cache from resolved paths to values. */ - typedef boost::unordered_flat_map< - SourcePath, - Value, - std::hash, - std::equal_to, - traceable_allocator>> - FileEvalCache; - FileEvalCache fileEvalCache; + struct FileEvalCache; + ref fileEvalCache; /** * Associate source positions of certain AST nodes with their preceding doc comment, if they have one. From 4b63ff63a416f364e40c0183d53f56b77595f44f Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 12 Sep 2025 17:26:29 +0200 Subject: [PATCH 5/6] Remove some unnecessary hash template arguments --- src/libexpr/include/nix/expr/eval.hh | 2 +- src/libfetchers/filtering-source-accessor.cc | 6 +++--- src/libfetchers/git-utils.cc | 4 ++-- .../include/nix/fetchers/filtering-source-accessor.hh | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/libexpr/include/nix/expr/eval.hh b/src/libexpr/include/nix/expr/eval.hh index 4b294ad9a..57f0f3f9d 100644 --- a/src/libexpr/include/nix/expr/eval.hh +++ b/src/libexpr/include/nix/expr/eval.hh @@ -431,7 +431,7 @@ private: * Associate source positions of certain AST nodes with their preceding doc comment, if they have one. * Grouped by file. */ - boost::unordered_flat_map> positionToDocComment; + boost::unordered_flat_map positionToDocComment; LookupPath lookupPath; diff --git a/src/libfetchers/filtering-source-accessor.cc b/src/libfetchers/filtering-source-accessor.cc index d0991ae23..a99ecacef 100644 --- a/src/libfetchers/filtering-source-accessor.cc +++ b/src/libfetchers/filtering-source-accessor.cc @@ -59,12 +59,12 @@ void FilteringSourceAccessor::checkAccess(const CanonPath & path) struct AllowListSourceAccessorImpl : AllowListSourceAccessor { std::set allowedPrefixes; - boost::unordered_flat_set> allowedPaths; + boost::unordered_flat_set allowedPaths; AllowListSourceAccessorImpl( ref next, std::set && allowedPrefixes, - boost::unordered_flat_set> && allowedPaths, + boost::unordered_flat_set && allowedPaths, MakeNotAllowedError && makeNotAllowedError) : AllowListSourceAccessor(SourcePath(next), std::move(makeNotAllowedError)) , allowedPrefixes(std::move(allowedPrefixes)) @@ -86,7 +86,7 @@ struct AllowListSourceAccessorImpl : AllowListSourceAccessor ref AllowListSourceAccessor::create( ref next, std::set && allowedPrefixes, - boost::unordered_flat_set> && allowedPaths, + boost::unordered_flat_set && allowedPaths, MakeNotAllowedError && makeNotAllowedError) { return make_ref( diff --git a/src/libfetchers/git-utils.cc b/src/libfetchers/git-utils.cc index 4ed94a4ed..a3652e522 100644 --- a/src/libfetchers/git-utils.cc +++ b/src/libfetchers/git-utils.cc @@ -817,7 +817,7 @@ struct GitSourceAccessor : SourceAccessor return toHash(*git_tree_entry_id(entry)); } - boost::unordered_flat_map> lookupCache; + boost::unordered_flat_map lookupCache; /* Recursively look up 'path' relative to the root. */ git_tree_entry * lookup(State & state, const CanonPath & path) @@ -1254,7 +1254,7 @@ GitRepoImpl::getAccessor(const WorkdirInfo & wd, bool exportIgnore, MakeNotAllow makeFSSourceAccessor(path), std::set{wd.files}, // Always allow access to the root, but not its children. - boost::unordered_flat_set>{CanonPath::root}, + boost::unordered_flat_set{CanonPath::root}, std::move(makeNotAllowedError)) .cast(); if (exportIgnore) diff --git a/src/libfetchers/include/nix/fetchers/filtering-source-accessor.hh b/src/libfetchers/include/nix/fetchers/filtering-source-accessor.hh index 1d4028be5..f8a57bfb3 100644 --- a/src/libfetchers/include/nix/fetchers/filtering-source-accessor.hh +++ b/src/libfetchers/include/nix/fetchers/filtering-source-accessor.hh @@ -72,7 +72,7 @@ struct AllowListSourceAccessor : public FilteringSourceAccessor static ref create( ref next, std::set && allowedPrefixes, - boost::unordered_flat_set> && allowedPaths, + boost::unordered_flat_set && allowedPaths, MakeNotAllowedError && makeNotAllowedError); using FilteringSourceAccessor::FilteringSourceAccessor; From 8d8f49cb5a7c9889f9df95db3dd471b5549307bf Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 12 Sep 2025 17:49:15 +0200 Subject: [PATCH 6/6] Use concurrent_flat_map_fwd.hpp --- src/libexpr/eval.cc | 43 +++++++--------------------- src/libexpr/include/nix/expr/eval.hh | 17 +++++++---- 2 files changed, 22 insertions(+), 38 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 5629865f0..93916d465 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -193,27 +193,6 @@ static Symbol getName(const AttrName & name, EvalState & state, Env & env) static constexpr size_t BASE_ENV_SIZE = 128; -struct EvalState::SrcToStore -{ - boost::concurrent_flat_map inner; -}; - -struct EvalState::ImportResolutionCache -{ - boost::concurrent_flat_map inner; -}; - -struct EvalState::FileEvalCache -{ - boost::concurrent_flat_map< - SourcePath, - Value *, - std::hash, - std::equal_to, - traceable_allocator>> - inner; -}; - EvalState::EvalState( const LookupPath & lookupPathFromArguments, ref store, @@ -286,9 +265,9 @@ EvalState::EvalState( , debugRepl(nullptr) , debugStop(false) , trylevel(0) - , srcToStore(make_ref()) - , importResolutionCache(make_ref()) - , fileEvalCache(make_ref()) + , srcToStore(make_ref()) + , importResolutionCache(make_ref()) + , fileEvalCache(make_ref()) , regexCache(makeRegexCache()) #if NIX_USE_BOEHMGC , valueAllocCache(std::allocate_shared(traceable_allocator(), nullptr)) @@ -1100,14 +1079,14 @@ struct ExprParseFile : Expr void EvalState::evalFile(const SourcePath & path, Value & v, bool mustBeTrivial) { - auto resolvedPath = getConcurrent(importResolutionCache->inner, path); + auto resolvedPath = getConcurrent(*importResolutionCache, path); if (!resolvedPath) { resolvedPath = resolveExprPath(path); - importResolutionCache->inner.emplace(path, *resolvedPath); + importResolutionCache->emplace(path, *resolvedPath); } - if (auto v2 = getConcurrent(fileEvalCache->inner, *resolvedPath)) { + if (auto v2 = getConcurrent(*fileEvalCache, *resolvedPath)) { forceValue(**v2, noPos); v = **v2; return; @@ -1116,7 +1095,7 @@ void EvalState::evalFile(const SourcePath & path, Value & v, bool mustBeTrivial) Value * vExpr; ExprParseFile expr{*resolvedPath, mustBeTrivial}; - fileEvalCache->inner.try_emplace_and_cvisit( + fileEvalCache->try_emplace_and_cvisit( *resolvedPath, nullptr, [&](auto & i) { @@ -1133,8 +1112,8 @@ void EvalState::evalFile(const SourcePath & path, Value & v, bool mustBeTrivial) void EvalState::resetFileCache() { - fileEvalCache->inner.clear(); - fileEvalCache->inner.rehash(0); + importResolutionCache->clear(); + fileEvalCache->clear(); inputCache->clear(); } @@ -2419,7 +2398,7 @@ StorePath EvalState::copyPathToStore(NixStringContext & context, const SourcePat if (nix::isDerivation(path.path.abs())) error("file names are not allowed to end in '%1%'", drvExtension).debugThrow(); - auto dstPathCached = getConcurrent(srcToStore->inner, path); + auto dstPathCached = getConcurrent(*srcToStore, path); auto dstPath = dstPathCached ? *dstPathCached : [&]() { auto dstPath = fetchToStore( @@ -2432,7 +2411,7 @@ StorePath EvalState::copyPathToStore(NixStringContext & context, const SourcePat nullptr, repair); allowPath(dstPath); - srcToStore->inner.try_emplace(path, dstPath); + srcToStore->try_emplace(path, dstPath); printMsg(lvlChatty, "copied source '%1%' -> '%2%'", path, store->printStorePath(dstPath)); return dstPath; }(); diff --git a/src/libexpr/include/nix/expr/eval.hh b/src/libexpr/include/nix/expr/eval.hh index 57f0f3f9d..a5e3a2bc7 100644 --- a/src/libexpr/include/nix/expr/eval.hh +++ b/src/libexpr/include/nix/expr/eval.hh @@ -21,6 +21,8 @@ #include "nix/expr/config.hh" #include +#include + #include #include #include @@ -411,21 +413,24 @@ private: /* Cache for calls to addToStore(); maps source paths to the store paths. */ - struct SrcToStore; - ref srcToStore; + ref> srcToStore; /** * A cache that maps paths to "resolved" paths for importing Nix * expressions, i.e. `/foo` to `/foo/default.nix`. */ - struct ImportResolutionCache; - ref importResolutionCache; + ref> importResolutionCache; /** * A cache from resolved paths to values. */ - struct FileEvalCache; - ref fileEvalCache; + ref, + std::equal_to, + traceable_allocator>>> + fileEvalCache; /** * Associate source positions of certain AST nodes with their preceding doc comment, if they have one.