Merge pull request #197 from DeterminateSystems/more-concurrent-boost

More use of boost::concurrent_flat_map and a correctness fix

Author: cole-h

src/libexpr/eval.cc

@@ -40,6 +40,7 @@
 
 #include <nlohmann/json.hpp>
 #include <boost/container/small_vector.hpp>
+#include <boost/unordered/concurrent_flat_map.hpp>
 
 #ifndef _WIN32 // TODO use portable implementation
 #  include <sys/resource.h>
@@ -213,6 +214,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,
@@ -344,6 +366,9 @@ EvalState::EvalState(
     , debugStop(false)
     , trylevel(0)
     , asyncPathWriter(AsyncPathWriter::make(store))
+    , srcToStore(make_ref<SrcToStore>())
+    , importResolutionCache(make_ref<ImportResolutionCache>())
+    , fileEvalCache(make_ref<FileEvalCache>())
     , regexCache(makeRegexCache())
 #if NIX_USE_BOEHMGC
     , baseEnvP(std::allocate_shared<Env *>(traceable_allocator<Env *>(), &allocEnv(BASE_ENV_SIZE)))
@@ -1168,29 +1193,31 @@ struct ExprParseFile : Expr
 
 void EvalState::evalFile(const SourcePath & path, Value & v, bool mustBeTrivial)
 {
-    auto resolvedPath = getOptional(*importResolutionCache.readLock(), path);
+    auto resolvedPath = getConcurrent(importResolutionCache->inner, path);
 
     if (!resolvedPath) {
         resolvedPath = resolveExprPath(path);
-        importResolutionCache.lock()->emplace(path, *resolvedPath);
+        importResolutionCache->inner.emplace(path, *resolvedPath);
     }
 
-    if (auto v2 = get(*fileEvalCache.readLock(), *resolvedPath)) {
-        forceValue(*const_cast<Value *>(v2), noPos);
-        v = *v2;
+    if (auto v2 = getConcurrent(fileEvalCache->inner, *resolvedPath)) {
+        forceValue(**v2, noPos);
+        v = **v2;
         return;
     }
 
     Value * vExpr;
     ExprParseFile expr{*resolvedPath, mustBeTrivial};
 
-    {
-        auto cache(fileEvalCache.lock());
-        auto [i, inserted] = cache->try_emplace(*resolvedPath);
-        if (inserted)
-            i->second.mkThunk(&baseEnv, &expr);
-        vExpr = &i->second;
-    }
+    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);
 
@@ -1199,7 +1226,7 @@ void EvalState::evalFile(const SourcePath & path, Value & v, bool mustBeTrivial)
 
 void EvalState::resetFileCache()
 {
-    fileEvalCache.lock()->clear();
+    fileEvalCache->inner.clear();
     inputCache->clear();
 }
 
@@ -2499,7 +2526,7 @@ 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();
 
-    auto dstPathCached = get(*srcToStore.lock(), path);
+    auto dstPathCached = getConcurrent(srcToStore->inner, path);
 
     auto dstPath = dstPathCached ? *dstPathCached : [&]() {
         auto dstPath = fetchToStore(
@@ -2512,7 +2539,7 @@ StorePath EvalState::copyPathToStore(NixStringContext & context, const SourcePat
             nullptr,
             repair);
         allowPath(dstPath);
-        srcToStore.lock()->try_emplace(path, dstPath);
+        srcToStore->inner.try_emplace(path, dstPath);
         printMsg(lvlChatty, "copied source '%1%' -> '%2%'", path, store->printStorePath(dstPath));
         return dstPath;
     }();

src/libexpr/include/nix/expr/eval.hh

@@ -371,25 +371,21 @@ private:
 
     /* Cache for calls to addToStore(); maps source paths to the store
        paths. */
-    Sync<std::unordered_map<SourcePath, StorePath>> srcToStore;
+    struct SrcToStore;
+    ref<SrcToStore> srcToStore;
 
     /**
      * A cache that maps paths to "resolved" paths for importing Nix
      * expressions, i.e. `/foo` to `/foo/default.nix`.
      */
-    SharedSync<std::unordered_map<SourcePath, SourcePath>> importResolutionCache;
+    struct ImportResolutionCache;
+    ref<ImportResolutionCache> importResolutionCache;
 
     /**
      * A cache from resolved paths to values.
      */
-    typedef std::unordered_map<
-        SourcePath,
-        Value,
-        std::hash<SourcePath>,
-        std::equal_to<SourcePath>,
-        traceable_allocator<std::pair<const SourcePath, Value>>>
-        FileEvalCache;
-    SharedSync<FileEvalCache> fileEvalCache;
+    struct FileEvalCache;
+    ref<FileEvalCache> fileEvalCache;
 
     /**
      * Associate source positions of certain AST nodes with their preceding doc comment, if they have one.

src/libutil/include/nix/util/source-path.hh

@@ -119,15 +119,21 @@ 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<nix::SourcePath>
 {
     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);
     }
 };

src/libutil/include/nix/util/util.hh

@@ -227,6 +227,14 @@ std::optional<typename T::mapped_type> getOptional(const T & map, const typename
     return {i->second};
 }
 
+template<class T>
+std::optional<typename T::mapped_type> getConcurrent(const T & map, const typename T::key_type & key)
+{
+    std::optional<typename T::mapped_type> 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.
  */

src/libutil/mounted-source-accessor.cc

@@ -86,9 +86,7 @@ struct MountedSourceAccessorImpl : MountedSourceAccessor
 
     std::shared_ptr<SourceAccessor> getMount(CanonPath mountPoint) override
     {
-        std::optional<ref<SourceAccessor>> res;
-        mounts.cvisit(mountPoint, [&](auto & x) { res = x.second; });
-        if (res)
+        if (auto res = getConcurrent(mounts, mountPoint))
             return *res;
         else
             return nullptr;

src/libutil/posix-source-accessor.cc

@@ -95,9 +95,7 @@ std::optional<struct stat> PosixSourceAccessor::cachedLstat(const CanonPath & pa
     // former is not hashable on libc++.
     Path absPath = makeAbsPath(path).string();
 
-    std::optional<Cache::mapped_type> 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());