From 90d70aa4c955570b24ef12155272ce2777a3ff56 Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Wed, 14 May 2025 21:23:13 +0000 Subject: [PATCH 1/4] libutil: Format lru-cache.hh Rip off the band-aid for further refactors. The diff is very small, so it makes to get it out of the way first. --- maintainers/flake-module.nix | 1 - src/libutil/include/nix/util/lru-cache.hh | 19 ++++++++++++++----- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/maintainers/flake-module.nix b/maintainers/flake-module.nix index 2dbeaf889..6497b17c1 100644 --- a/maintainers/flake-module.nix +++ b/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$'' diff --git a/src/libutil/include/nix/util/lru-cache.hh b/src/libutil/include/nix/util/lru-cache.hh index 6e14cac35..5fa3b34a6 100644 --- a/src/libutil/include/nix/util/lru-cache.hh +++ b/src/libutil/include/nix/util/lru-cache.hh @@ -25,21 +25,28 @@ private: using Data = std::map>; using LRU = std::list; - 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) { - if (capacity == 0) return; + if (capacity == 0) + return; erase(key); @@ -64,7 +71,8 @@ public: bool erase(const Key & 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; @@ -77,7 +85,8 @@ public: std::optional get(const Key & 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. From cd61e922ff312eefe0be55b7b468b83117f94181 Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Wed, 14 May 2025 21:42:35 +0000 Subject: [PATCH 2/4] libutil: Use heterogeneous lookup for LRUCache This gets rid of some ugly std::string_view -> std::string conversions, which are an eye-sore and lead to extra copying. --- src/libstore/store-api.cc | 10 +++++----- src/libutil/include/nix/util/lru-cache.hh | 13 ++++++++----- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 08c542041..f20d2d329 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -570,7 +570,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(); @@ -582,7 +582,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; } @@ -641,7 +641,7 @@ std::optional> 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()) @@ -657,7 +657,7 @@ std::optional> 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)) @@ -701,7 +701,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)) { diff --git a/src/libutil/include/nix/util/lru-cache.hh b/src/libutil/include/nix/util/lru-cache.hh index 5fa3b34a6..20891a086 100644 --- a/src/libutil/include/nix/util/lru-cache.hh +++ b/src/libutil/include/nix/util/lru-cache.hh @@ -11,7 +11,7 @@ namespace nix { /** * A simple least-recently used cache. Not thread-safe. */ -template +template> class LRUCache { private: @@ -22,7 +22,7 @@ private: // and LRU. struct LRUIterator; - using Data = std::map>; + using Data = std::map, Compare>; using LRU = std::list; struct LRUIterator @@ -43,7 +43,8 @@ public: /** * Insert or upsert an item in the cache. */ - void upsert(const Key & key, const Value & value) + template + void upsert(const K & key, const Value & value) { if (capacity == 0) return; @@ -68,7 +69,8 @@ public: i->second.first.it = j; } - bool erase(const Key & key) + template + bool erase(const K & key) { auto i = data.find(key); if (i == data.end()) @@ -82,7 +84,8 @@ public: * Look up an item in the cache. If it exists, it becomes the most * recently used item. * */ - std::optional get(const Key & key) + template + std::optional get(const K & key) { auto i = data.find(key); if (i == data.end()) From 2f2e04142e80e6762761cb90522b8276a13bdeab Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Wed, 14 May 2025 22:05:53 +0000 Subject: [PATCH 3/4] libutil: Simplify LRUCache::get by using list splice Splicing the list element to the back can be done in a much simpler and concise way without the need for erasing and re-inserting the element. Doing it this way is equivalent to just moving node pointers around, whereas inserting/erasing allocates/deallocates new nodes. --- src/libutil/include/nix/util/lru-cache.hh | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/libutil/include/nix/util/lru-cache.hh b/src/libutil/include/nix/util/lru-cache.hh index 20891a086..686b73149 100644 --- a/src/libutil/include/nix/util/lru-cache.hh +++ b/src/libutil/include/nix/util/lru-cache.hh @@ -93,12 +93,16 @@ public: /** * 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 From d955b401a73946bc084df36784286cd72c66ec41 Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Wed, 14 May 2025 22:14:46 +0000 Subject: [PATCH 4/4] libutil: Sprinkle some noexcept on LRUCache::{size,clear} The underlying containers are already noexcept to destroy and dtors are noexcept in general. --- src/libutil/include/nix/util/lru-cache.hh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libutil/include/nix/util/lru-cache.hh b/src/libutil/include/nix/util/lru-cache.hh index 686b73149..c9bcd7ee0 100644 --- a/src/libutil/include/nix/util/lru-cache.hh +++ b/src/libutil/include/nix/util/lru-cache.hh @@ -105,12 +105,12 @@ public: return value; } - size_t size() const + size_t size() const noexcept { return data.size(); } - void clear() + void clear() noexcept { data.clear(); lru.clear();