Merge pull request #13193 from xokdvium/lru-cache

libutil: Less unnecessary copying in `LRUCache`

maintainers/flake-module.nix

@@ -360,7 +360,6 @@
               ''^src/libutil/linux/namespaces\.cc$''
               ''^src/libutil/logging\.cc$''
               ''^src/libutil/include/nix/util/logging\.hh$''
-              ''^src/libutil/include/nix/util/lru-cache\.hh$''
               ''^src/libutil/memory-source-accessor\.cc$''
               ''^src/libutil/include/nix/util/memory-source-accessor\.hh$''
               ''^src/libutil/include/nix/util/pool\.hh$''

src/libstore/store-api.cc

@@ -571,7 +571,7 @@ bool Store::isValidPath(const StorePath & storePath)
 {
     {
         auto state_(state.lock());
-        auto res = state_->pathInfoCache.get(std::string(storePath.to_string()));
+        auto res = state_->pathInfoCache.get(storePath.to_string());
         if (res && res->isKnownNow()) {
             stats.narInfoReadAverted++;
             return res->didExist();
@@ -583,7 +583,7 @@ bool Store::isValidPath(const StorePath & storePath)
         if (res.first != NarInfoDiskCache::oUnknown) {
             stats.narInfoReadAverted++;
             auto state_(state.lock());
-            state_->pathInfoCache.upsert(std::string(storePath.to_string()),
+            state_->pathInfoCache.upsert(storePath.to_string(),
                 res.first == NarInfoDiskCache::oInvalid ? PathInfoCacheValue{} : PathInfoCacheValue { .value = res.second });
             return res.first == NarInfoDiskCache::oValid;
         }
@@ -642,7 +642,7 @@ std::optional<std::shared_ptr<const ValidPathInfo>> Store::queryPathInfoFromClie
     auto hashPart = std::string(storePath.hashPart());
 
     {
-        auto res = state.lock()->pathInfoCache.get(std::string(storePath.to_string()));
+        auto res = state.lock()->pathInfoCache.get(storePath.to_string());
         if (res && res->isKnownNow()) {
             stats.narInfoReadAverted++;
             if (res->didExist())
@@ -658,7 +658,7 @@ std::optional<std::shared_ptr<const ValidPathInfo>> Store::queryPathInfoFromClie
             stats.narInfoReadAverted++;
             {
                 auto state_(state.lock());
-                state_->pathInfoCache.upsert(std::string(storePath.to_string()),
+                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))
@@ -702,7 +702,7 @@ void Store::queryPathInfo(const StorePath & storePath,
 
                 {
                     auto state_(state.lock());
-                    state_->pathInfoCache.upsert(std::string(storePath.to_string()), PathInfoCacheValue { .value = info });
+                    state_->pathInfoCache.upsert(storePath.to_string(), PathInfoCacheValue { .value = info });
                 }
 
                 if (!info || !goodStorePath(storePath, info->path)) {

src/libutil/include/nix/util/lru-cache.hh

@@ -11,7 +11,7 @@ namespace nix {
 /**
  * A simple least-recently used cache. Not thread-safe.
  */
-template<typename Key, typename Value>
+template<typename Key, typename Value, typename Compare = std::less<>>
 class LRUCache
 {
 private:
@@ -22,24 +22,32 @@ private:
     // and LRU.
     struct LRUIterator;
 
-    using Data = std::map<Key, std::pair<LRUIterator, Value>>;
+    using Data = std::map<Key, std::pair<LRUIterator, Value>, Compare>;
     using LRU = std::list<typename Data::iterator>;
 
-    struct LRUIterator { typename LRU::iterator it; };
+    struct LRUIterator
+    {
+        typename LRU::iterator it;
+    };
 
     Data data;
     LRU lru;
 
 public:
 
-    LRUCache(size_t capacity) : capacity(capacity) { }
+    LRUCache(size_t capacity)
+        : capacity(capacity)
+    {
+    }
 
     /**
      * Insert or upsert an item in the cache.
      */
-    void upsert(const Key & key, const Value & value)
+    template<typename K>
+    void upsert(const K & key, const Value & value)
     {
-        if (capacity == 0) return;
+        if (capacity == 0)
+            return;
 
         erase(key);
 
@@ -61,10 +69,12 @@ public:
         i->second.first.it = j;
     }
 
-    bool erase(const Key & key)
+    template<typename K>
+    bool erase(const K & key)
     {
         auto i = data.find(key);
-        if (i == data.end()) return false;
+        if (i == data.end())
+            return false;
         lru.erase(i->second.first.it);
         data.erase(i);
         return true;
@@ -74,27 +84,33 @@ public:
      * Look up an item in the cache. If it exists, it becomes the most
      * recently used item.
      * */
-    std::optional<Value> get(const Key & key)
+    template<typename K>
+    std::optional<Value> get(const K & key)
     {
         auto i = data.find(key);
-        if (i == data.end()) return {};
+        if (i == data.end())
+            return {};
 
         /**
          * Move this item to the back of the LRU list.
+         *
+         * Think of std::list iterators as stable pointers to the list node,
+         * which never get invalidated. Thus, we can reuse the same lru list
+         * element and just splice it to the back of the list without the need
+         * to update its value in the key -> list iterator map.
          */
-        lru.erase(i->second.first.it);
-        auto j = lru.insert(lru.end(), i);
-        i->second.first.it = j;
+        auto & [it, value] = i->second;
+        lru.splice(/*pos=*/lru.end(), /*other=*/lru, it.it);
 
-        return i->second.second;
+        return value;
     }
 
-    size_t size() const
+    size_t size() const noexcept
     {
         return data.size();
     }
 
-    void clear()
+    void clear() noexcept
     {
         data.clear();
         lru.clear();