Reduce false sharing between pathInfoCache and Store

`perf c2c` shows a lot of cacheline conflicts between purely read-only
Store methods (like `parseStorePath()`) and the Sync classes. So
allocate pathInfoCache separately to avoid that.

Author: edolstra

src/libstore/binary-cache-store.cc

@@ -125,11 +125,8 @@ void BinaryCacheStore::writeNarInfo(ref<NarInfo> narInfo)
 
     upsertFile(narInfoFile, narInfo->to_string(*this), "text/x-nix-narinfo");
 
-    {
-        auto state_(state.lock());
-        state_->pathInfoCache.upsert(
-            std::string(narInfo->path.to_string()), PathInfoCacheValue{.value = std::shared_ptr<NarInfo>(narInfo)});
-    }
+    pathInfoCache->lock()->upsert(
+        std::string(narInfo->path.to_string()), PathInfoCacheValue{.value = std::shared_ptr<NarInfo>(narInfo)});
 
     if (diskCache)
         diskCache->upsertNarInfo(

src/libstore/include/nix/store/store-api.hh

@@ -310,14 +310,11 @@ protected:
         }
     };
 
-    struct State
-    {
-        LRUCache<std::string, PathInfoCacheValue> pathInfoCache;
-    };
-
     void invalidatePathInfoCacheFor(const StorePath & path);
 
-    SharedSync<State> state;
+    // Note: this is a `ref` to avoid false sharing with immutable
+    // bits of `Store`.
+    ref<SharedSync<LRUCache<std::string, PathInfoCacheValue>>> pathInfoCache;
 
     std::shared_ptr<NarInfoDiskCache> diskCache;
 
@@ -860,7 +857,7 @@ public:
      */
     void clearPathInfoCache()
     {
-        state.lock()->pathInfoCache.clear();
+        pathInfoCache->lock()->clear();
     }
 
     /**

src/libstore/local-store.cc

@@ -716,12 +716,8 @@ uint64_t LocalStore::addValidPath(State & state, const ValidPathInfo & info, boo
         }
     }
 
-    {
-        auto state_(Store::state.lock());
-        state_->pathInfoCache.upsert(
-            std::string(info.path.to_string()),
-            PathInfoCacheValue{.value = std::make_shared<const ValidPathInfo>(info)});
-    }
+    pathInfoCache->lock()->upsert(
+        std::string(info.path.to_string()), PathInfoCacheValue{.value = std::make_shared<const ValidPathInfo>(info)});
 
     return id;
 }
@@ -1023,10 +1019,7 @@ void LocalStore::invalidatePath(State & state, const StorePath & path)
     /* Note that the foreign key constraints on the Refs table take
        care of deleting the references entries for `path'. */
 
-    {
-        auto state_(Store::state.lock());
-        state_->pathInfoCache.erase(std::string(path.to_string()));
-    }
+    pathInfoCache->lock()->erase(std::string(path.to_string()));
 }
 
 const PublicKeys & LocalStore::getPublicKeys()

src/libstore/remote-store.cc

@@ -764,10 +764,7 @@ void RemoteStore::collectGarbage(const GCOptions & options, GCResults & results)
     results.bytesFreed = readLongLong(conn->from);
     readLongLong(conn->from); // obsolete
 
-    {
-        auto state_(Store::state.lock());
-        state_->pathInfoCache.clear();
-    }
+    pathInfoCache->lock()->clear();
 }
 
 void RemoteStore::optimiseStore()

src/libstore/store-api.cc

@@ -306,7 +306,7 @@ StringSet Store::Config::getDefaultSystemFeatures()
 Store::Store(const Store::Config & config)
     : StoreDirConfig{config}
     , config{config}
-    , state({(size_t) config.pathInfoCacheSize})
+    , pathInfoCache(make_ref<decltype(pathInfoCache)::element_type>((size_t) config.pathInfoCacheSize))
 {
     assertLibStoreInitialized();
 }
@@ -326,7 +326,7 @@ bool Store::PathInfoCacheValue::isKnownNow()
 
 void Store::invalidatePathInfoCacheFor(const StorePath & path)
 {
-    state.lock()->pathInfoCache.erase(path.to_string());
+    pathInfoCache->lock()->erase(path.to_string());
 }
 
 std::map<std::string, std::optional<StorePath>> Store::queryStaticPartialDerivationOutputMap(const StorePath & path)
@@ -448,22 +448,18 @@ void Store::querySubstitutablePathInfos(const StorePathCAMap & paths, Substituta
 
 bool Store::isValidPath(const StorePath & storePath)
 {
-    {
-        auto state_(state.lock());
-        auto res = state_->pathInfoCache.get(storePath.to_string());
-        if (res && res->isKnownNow()) {
-            stats.narInfoReadAverted++;
-            return res->didExist();
-        }
+    auto res = pathInfoCache->lock()->get(storePath.to_string());
+    if (res && res->isKnownNow()) {
+        stats.narInfoReadAverted++;
+        return res->didExist();
     }
 
     if (diskCache) {
         auto res = diskCache->lookupNarInfo(
             config.getReference().render(/*FIXME withParams=*/false), std::string(storePath.hashPart()));
         if (res.first != NarInfoDiskCache::oUnknown) {
             stats.narInfoReadAverted++;
-            auto state_(state.lock());
-            state_->pathInfoCache.upsert(
+            pathInfoCache->lock()->upsert(
                 storePath.to_string(),
                 res.first == NarInfoDiskCache::oInvalid ? PathInfoCacheValue{}
                                                         : PathInfoCacheValue{.value = res.second});
@@ -518,30 +514,25 @@ std::optional<std::shared_ptr<const ValidPathInfo>> Store::queryPathInfoFromClie
 {
     auto hashPart = std::string(storePath.hashPart());
 
-    {
-        auto res = state.lock()->pathInfoCache.get(storePath.to_string());
-        if (res && res->isKnownNow()) {
-            stats.narInfoReadAverted++;
-            if (res->didExist())
-                return std::make_optional(res->value);
-            else
-                return std::make_optional(nullptr);
-        }
+    auto res = pathInfoCache->lock()->get(storePath.to_string());
+    if (res && res->isKnownNow()) {
+        stats.narInfoReadAverted++;
+        if (res->didExist())
+            return std::make_optional(res->value);
+        else
+            return std::make_optional(nullptr);
     }
 
     if (diskCache) {
         auto res = diskCache->lookupNarInfo(config.getReference().render(/*FIXME withParams=*/false), hashPart);
         if (res.first != NarInfoDiskCache::oUnknown) {
             stats.narInfoReadAverted++;
-            {
-                auto state_(state.lock());
-                state_->pathInfoCache.upsert(
-                    storePath.to_string(),
-                    res.first == NarInfoDiskCache::oInvalid ? PathInfoCacheValue{}
-                                                            : PathInfoCacheValue{.value = res.second});
-                if (res.first == NarInfoDiskCache::oInvalid || !goodStorePath(storePath, res.second->path))
-                    return std::make_optional(nullptr);
-            }
+            pathInfoCache->lock()->upsert(
+                storePath.to_string(),
+                res.first == NarInfoDiskCache::oInvalid ? PathInfoCacheValue{}
+                                                        : PathInfoCacheValue{.value = res.second});
+            if (res.first == NarInfoDiskCache::oInvalid || !goodStorePath(storePath, res.second->path))
+                return std::make_optional(nullptr);
             assert(res.second);
             return std::make_optional(res.second);
         }
@@ -577,10 +568,7 @@ void Store::queryPathInfo(const StorePath & storePath, Callback<ref<const ValidP
                 if (diskCache)
                     diskCache->upsertNarInfo(config.getReference().render(/*FIXME withParams=*/false), hashPart, info);
 
-                {
-                    auto state_(state.lock());
-                    state_->pathInfoCache.upsert(storePath.to_string(), PathInfoCacheValue{.value = info});
-                }
+                pathInfoCache->lock()->upsert(storePath.to_string(), PathInfoCacheValue{.value = info});
 
                 if (!info || !goodStorePath(storePath, info->path)) {
                     stats.narInfoMissing++;
@@ -803,10 +791,7 @@ StorePathSet Store::exportReferences(const StorePathSet & storePaths, const Stor
 
 const Store::Stats & Store::getStats()
 {
-    {
-        auto state_(state.readLock());
-        stats.pathInfoCacheSize = state_->pathInfoCache.size();
-    }
+    stats.pathInfoCacheSize = pathInfoCache->readLock()->size();
     return stats;
 }
 

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

@@ -18,6 +18,9 @@ private:
     std::shared_ptr<T> p;
 
 public:
+
+    using element_type = T;
+
     explicit ref(const std::shared_ptr<T> & p)
         : p(p)
     {

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

@@ -36,6 +36,8 @@ private:
 
 public:
 
+    using element_type = T;
+
     SyncBase() {}
 
     SyncBase(const T & data)