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.

Author: edolstra

src/libexpr/eval.cc

@@ -38,6 +38,7 @@
 
 #include <nlohmann/json.hpp>
 #include <boost/container/small_vector.hpp>
+#include <boost/unordered/concurrent_flat_map.hpp>
 
 #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<SourcePath, StorePath> inner;
+};
+
+struct EvalState::ImportResolutionCache
+{
+    boost::concurrent_flat_map<SourcePath, SourcePath> inner;
+};
+
+struct EvalState::FileEvalCache
+{
+    boost::concurrent_flat_map<
+        SourcePath,
+        Value *,
+        std::hash<SourcePath>,
+        std::equal_to<SourcePath>,
+        traceable_allocator<std::pair<const SourcePath, Value *>>>
+        inner;
+};
+
 EvalState::EvalState(
     const LookupPath & lookupPathFromArguments,
     ref<Store> store,
@@ -264,6 +286,9 @@ EvalState::EvalState(
     , debugRepl(nullptr)
     , debugStop(false)
     , trylevel(0)
+    , srcToStore(make_ref<SrcToStore>())
+    , importResolutionCache(make_ref<ImportResolutionCache>())
+    , fileEvalCache(make_ref<FileEvalCache>())
     , regexCache(makeRegexCache())
 #if NIX_USE_BOEHMGC
     , valueAllocCache(std::allocate_shared<void *>(traceable_allocator<void *>(), nullptr))
@@ -1031,63 +1056,85 @@ Value * ExprPath::maybeThunk(EvalState & state, Env & env)
     return &v;
 }
 
-void EvalState::evalFile(const SourcePath & path, Value & v, bool mustBeTrivial)
+/**
+ * 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
 {
-    FileEvalCache::iterator i;
-    if ((i = fileEvalCache.find(path)) != fileEvalCache.end()) {
-        v = i->second;
-        return;
-    }
+    SourcePath & path;
+    bool mustBeTrivial;
 
-    auto resolvedPath = resolveExprPath(path);
-    if ((i = fileEvalCache.find(resolvedPath)) != fileEvalCache.end()) {
-        v = i->second;
-        return;
+    ExprParseFile(SourcePath & path, bool mustBeTrivial)
+        : path(path)
+        , mustBeTrivial(mustBeTrivial)
+    {
     }
 
-    printTalkative("evaluating file '%1%'", resolvedPath);
-    Expr * e = nullptr;
+    void eval(EvalState & state, Env & env, Value & v) override
+    {
+        printTalkative("evaluating file '%s'", path);
+
+        auto e = state.parseExprFromFile(path);
 
-    auto j = fileParseCache.find(resolvedPath);
-    if (j != fileParseCache.end())
-        e = j->second;
+        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<ExprAttrs *>(e)))
+                state.error<EvalError>("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;
+        }
+    }
+};
 
-    if (!e)
-        e = parseExprFromFile(resolvedPath);
+void EvalState::evalFile(const SourcePath & path, Value & v, bool mustBeTrivial)
+{
+    auto resolvedPath = getConcurrent(importResolutionCache->inner, path);
 
-    fileParseCache.emplace(resolvedPath, e);
+    if (!resolvedPath) {
+        resolvedPath = resolveExprPath(path);
+        importResolutionCache->inner.emplace(path, *resolvedPath);
+    }
 
-    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<ExprAttrs *>(e)))
-            error<EvalError>("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;
+    if (auto v2 = getConcurrent(fileEvalCache->inner, *resolvedPath)) {
+        forceValue(**v2, noPos);
+        v = **v2;
+        return;
     }
 
-    fileEvalCache.emplace(resolvedPath, v);
-    if (path != resolvedPath)
-        fileEvalCache.emplace(path, v);
+    Value * vExpr;
+    ExprParseFile expr{*resolvedPath, mustBeTrivial};
+
+    fileEvalCache->inner.try_emplace_and_cvisit(
+        *resolvedPath,
+        nullptr,
+        [&](auto & i) {
+            vExpr = allocValue();
+            vExpr->mkThunk(&baseEnv, &expr);
+            i.second = vExpr;
+        },
+        [&](auto & i) { vExpr = i.second; });
+
+    forceValue(*vExpr, noPos);
+
+    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,24 +2419,26 @@ StorePath EvalState::copyPathToStore(NixStringContext & context, const SourcePat
     if (nix::isDerivation(path.path.abs()))
         error<EvalError>("file names are not allowed to end in '%1%'", drvExtension).debugThrow();
 
-    std::optional<StorePath> 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),
             settings.readOnlyMode ? FetchMode::DryRun : FetchMode::Copy,
             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)

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 <boost/unordered/concurrent_flat_map.hpp>
 #include <boost/unordered/unordered_flat_map.hpp>
 #include <map>
 #include <optional>
@@ -412,31 +411,21 @@ private:
 
     /* Cache for calls to addToStore(); maps source paths to the store
        paths. */
-    boost::concurrent_flat_map<SourcePath, StorePath, std::hash<SourcePath>> srcToStore;
+    struct SrcToStore;
+    ref<SrcToStore> 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<SourcePath>,
-        std::equal_to<SourcePath>,
-        traceable_allocator<std::pair<const SourcePath, Expr *>>>
-        FileParseCache;
-    FileParseCache fileParseCache;
+    struct ImportResolutionCache;
+    ref<ImportResolutionCache> importResolutionCache;
 
     /**
-     * A cache from path names to values.
+     * A cache from resolved paths to values.
      */
-    typedef boost::unordered_flat_map<
-        SourcePath,
-        Value,
-        std::hash<SourcePath>,
-        std::equal_to<SourcePath>,
-        traceable_allocator<std::pair<const SourcePath, Value>>>
-        FileEvalCache;
-    FileEvalCache fileEvalCache;
+    struct FileEvalCache;
+    ref<FileEvalCache> fileEvalCache;
 
     /**
      * Associate source positions of certain AST nodes with their preceding doc comment, if they have one.