From e251ffdd46dd4c213e63e116ca5ad944e4fc801d Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Mon, 24 Nov 2025 14:47:49 +0100 Subject: [PATCH 1/2] RemoteFSAccessor: Make the local NAR cache content-addressed Use double-indirection for better NAR accessor caching Co-authored-by: John Ericson --- .../include/nix/store/remote-fs-accessor.hh | 17 ++++- src/libstore/remote-fs-accessor.cc | 69 ++++++++++--------- 2 files changed, 52 insertions(+), 34 deletions(-) diff --git a/src/libstore/include/nix/store/remote-fs-accessor.hh b/src/libstore/include/nix/store/remote-fs-accessor.hh index 6e3aef335dd..781af5d366a 100644 --- a/src/libstore/include/nix/store/remote-fs-accessor.hh +++ b/src/libstore/include/nix/store/remote-fs-accessor.hh @@ -11,7 +11,17 @@ class RemoteFSAccessor : public SourceAccessor { ref store; - std::map> nars; + /** + * Map from store path hash part to NAR hash. Used to then look up + * in `nars`. The indirection allows avoiding opening multiple + * redundant NAR accessors for the same NAR. + */ + std::map> narHashes; + + /** + * Map from NAR hash to NAR accessor. + */ + std::map> nars; bool requireValidPath; @@ -21,9 +31,10 @@ class RemoteFSAccessor : public SourceAccessor friend struct BinaryCacheStore; - std::filesystem::path makeCacheFile(std::string_view hashPart, const std::string & ext); + std::filesystem::path makeCacheFile(const Hash & narHash, const std::string & ext); - ref addToCache(std::string_view hashPart, std::string && nar); + ref + addToCache(const std::filesystem::path & cacheFile, const std::filesystem::path & listingFile, std::string && nar); public: diff --git a/src/libstore/remote-fs-accessor.cc b/src/libstore/remote-fs-accessor.cc index 0fd0945da02..abc19bbc5c1 100644 --- a/src/libstore/remote-fs-accessor.cc +++ b/src/libstore/remote-fs-accessor.cc @@ -18,32 +18,33 @@ RemoteFSAccessor::RemoteFSAccessor( createDirs(*cacheDir); } -std::filesystem::path RemoteFSAccessor::makeCacheFile(std::string_view hashPart, const std::string & ext) +std::filesystem::path RemoteFSAccessor::makeCacheFile(const Hash & narHash, const std::string & ext) { assert(cacheDir); - auto res = (*cacheDir / hashPart); - res.concat(concatStrings(".", ext)); + auto res = *cacheDir / narHash.to_string(HashFormat::Nix32, false); + res += "."; + res += ext; return res; } -ref RemoteFSAccessor::addToCache(std::string_view hashPart, std::string && nar) +ref RemoteFSAccessor::addToCache( + const std::filesystem::path & cacheFile, const std::filesystem::path & listingFile, std::string && nar) { - if (cacheDir) { + if (!cacheFile.empty()) { try { /* FIXME: do this asynchronously. */ - writeFile(makeCacheFile(hashPart, "nar"), nar); + writeFile(cacheFile, nar); } catch (...) { ignoreExceptionExceptInterrupt(); } } auto narAccessor = makeNarAccessor(std::move(nar)); - nars.emplace(hashPart, narAccessor); - if (cacheDir) { + if (!listingFile.empty()) { try { nlohmann::json j = listNarDeep(*narAccessor, CanonPath::root); - writeFile(makeCacheFile(hashPart, "ls"), j.dump()); + writeFile(listingFile, j.dump()); } catch (...) { ignoreExceptionExceptInterrupt(); } @@ -62,37 +63,43 @@ std::pair, CanonPath> RemoteFSAccessor::fetch(const CanonPat std::shared_ptr RemoteFSAccessor::accessObject(const StorePath & storePath) { - auto i = nars.find(std::string(storePath.hashPart())); - if (i != nars.end()) - return i->second; - - std::string listing; - std::filesystem::path cacheFile; + if (auto * narHash = get(narHashes, storePath.hashPart())) { + if (auto * accessor = get(nars, *narHash)) + return *accessor; + } - if (cacheDir && nix::pathExists(cacheFile = makeCacheFile(storePath.hashPart(), "nar"))) { + auto info = store->queryPathInfo(storePath); - try { - listing = nix::readFile(makeCacheFile(storePath.hashPart(), "ls")); - auto listingJson = nlohmann::json::parse(listing); - auto narAccessor = makeLazyNarAccessor(listingJson, seekableGetNarBytes(cacheFile)); + auto cacheAccessor = [&](ref accessor) { + narHashes.emplace(storePath.hashPart(), info->narHash); + nars.emplace(info->narHash, accessor); + return accessor; + }; - nars.emplace(storePath.hashPart(), narAccessor); - return narAccessor; + std::filesystem::path cacheFile, listingFile; - } catch (SystemError &) { - } - - try { - auto narAccessor = makeNarAccessor(nix::readFile(cacheFile)); - nars.emplace(storePath.hashPart(), narAccessor); - return narAccessor; - } catch (SystemError &) { + if (cacheDir) { + cacheFile = makeCacheFile(info->narHash, "nar"); + listingFile = makeCacheFile(info->narHash, "ls"); + + if (nix::pathExists(cacheFile)) { + try { + auto listing = nix::readFile(listingFile); + auto listingJson = nlohmann::json::parse(listing); + return cacheAccessor(makeLazyNarAccessor(listingJson, seekableGetNarBytes(cacheFile))); + } catch (SystemError &) { + } + + try { + return cacheAccessor(makeNarAccessor(nix::readFile(cacheFile))); + } catch (SystemError &) { + } } } StringSink sink; store->narFromPath(storePath, sink); - return addToCache(storePath.hashPart(), std::move(sink.s)); + return cacheAccessor(addToCache(cacheFile, listingFile, std::move(sink.s))); } std::optional RemoteFSAccessor::maybeLstat(const CanonPath & path) From 2af5792cc2f8f46a3057252f0678d8f70c7abbb8 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 12 Jan 2026 23:09:45 -0500 Subject: [PATCH 2/2] Inline `RemoteFSAccessor::addToCache` It was not pulling its weight. (Only used once, optional paths are confusing, we already have an `if` / branch fit-for-purpose.) --- .../include/nix/store/remote-fs-accessor.hh | 3 - src/libstore/remote-fs-accessor.cc | 60 +++++++++---------- 2 files changed, 28 insertions(+), 35 deletions(-) diff --git a/src/libstore/include/nix/store/remote-fs-accessor.hh b/src/libstore/include/nix/store/remote-fs-accessor.hh index 781af5d366a..62f4dd72602 100644 --- a/src/libstore/include/nix/store/remote-fs-accessor.hh +++ b/src/libstore/include/nix/store/remote-fs-accessor.hh @@ -33,9 +33,6 @@ class RemoteFSAccessor : public SourceAccessor std::filesystem::path makeCacheFile(const Hash & narHash, const std::string & ext); - ref - addToCache(const std::filesystem::path & cacheFile, const std::filesystem::path & listingFile, std::string && nar); - public: /** diff --git a/src/libstore/remote-fs-accessor.cc b/src/libstore/remote-fs-accessor.cc index abc19bbc5c1..1af82d279ce 100644 --- a/src/libstore/remote-fs-accessor.cc +++ b/src/libstore/remote-fs-accessor.cc @@ -27,32 +27,6 @@ std::filesystem::path RemoteFSAccessor::makeCacheFile(const Hash & narHash, cons return res; } -ref RemoteFSAccessor::addToCache( - const std::filesystem::path & cacheFile, const std::filesystem::path & listingFile, std::string && nar) -{ - if (!cacheFile.empty()) { - try { - /* FIXME: do this asynchronously. */ - writeFile(cacheFile, nar); - } catch (...) { - ignoreExceptionExceptInterrupt(); - } - } - - auto narAccessor = makeNarAccessor(std::move(nar)); - - if (!listingFile.empty()) { - try { - nlohmann::json j = listNarDeep(*narAccessor, CanonPath::root); - writeFile(listingFile, j.dump()); - } catch (...) { - ignoreExceptionExceptInterrupt(); - } - } - - return narAccessor; -} - std::pair, CanonPath> RemoteFSAccessor::fetch(const CanonPath & path) { auto [storePath, restPath] = store->toStorePath(store->storeDir + path.abs()); @@ -76,11 +50,15 @@ std::shared_ptr RemoteFSAccessor::accessObject(const StorePath & return accessor; }; - std::filesystem::path cacheFile, listingFile; + auto getNar = [&]() { + StringSink sink; + store->narFromPath(storePath, sink); + return std::move(sink.s); + }; if (cacheDir) { - cacheFile = makeCacheFile(info->narHash, "nar"); - listingFile = makeCacheFile(info->narHash, "ls"); + auto cacheFile = makeCacheFile(info->narHash, "nar"); + auto listingFile = makeCacheFile(info->narHash, "ls"); if (nix::pathExists(cacheFile)) { try { @@ -95,11 +73,29 @@ std::shared_ptr RemoteFSAccessor::accessObject(const StorePath & } catch (SystemError &) { } } + + auto nar = getNar(); + + try { + /* FIXME: do this asynchronously. */ + writeFile(cacheFile, nar); + } catch (...) { + ignoreExceptionExceptInterrupt(); + } + + auto narAccessor = makeNarAccessor(std::move(nar)); + + try { + nlohmann::json j = listNarDeep(*narAccessor, CanonPath::root); + writeFile(listingFile, j.dump()); + } catch (...) { + ignoreExceptionExceptInterrupt(); + } + + return cacheAccessor(narAccessor); } - StringSink sink; - store->narFromPath(storePath, sink); - return cacheAccessor(addToCache(cacheFile, listingFile, std::move(sink.s))); + return cacheAccessor(makeNarAccessor(getNar())); } std::optional RemoteFSAccessor::maybeLstat(const CanonPath & path)