From d0a284284bc93014c98294292b7f4b95864f37ee Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Wed, 17 Jan 2024 16:54:45 +0100 Subject: [PATCH 1/3] refactor: Extract simply, awkwardly Store::queryPathInfoFromClientCache This is useful for determining quickly which substituters to query. An alternative would be for users to invoke the narinfo cache db directly, so why do we need this change? - It is easier to use. I believe Nix itself should also use it. - This way, the narinfo cache db remains an implementation detail. - Callers get to use the in-memory cache as well. --- src/libstore/store-api.cc | 64 +++++++++++++++++++++++---------------- src/libstore/store-api.hh | 12 ++++++++ 2 files changed, 50 insertions(+), 26 deletions(-) diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 0c37ecd30..66bc95625 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -685,6 +685,42 @@ static bool goodStorePath(const StorePath & expected, const StorePath & actual) && (expected.name() == Store::MissingName || expected.name() == actual.name()); } +bool Store::queryPathInfoFromClientCache(const StorePath & storePath, + Callback> & callback) +{ + auto hashPart = std::string(storePath.hashPart()); + + { + auto res = state.lock()->pathInfoCache.get(std::string(storePath.to_string())); + if (res && res->isKnownNow()) { + stats.narInfoReadAverted++; + if (!res->didExist()) + throw InvalidPath("path '%s' is not valid", printStorePath(storePath)); + callback(ref(res->value)); + return true; + } + } + + if (diskCache) { + auto res = diskCache->lookupNarInfo(getUri(), hashPart); + if (res.first != NarInfoDiskCache::oUnknown) { + stats.narInfoReadAverted++; + { + auto state_(state.lock()); + state_->pathInfoCache.upsert(std::string(storePath.to_string()), + res.first == NarInfoDiskCache::oInvalid ? PathInfoCacheValue{} : PathInfoCacheValue{ .value = res.second }); + if (res.first == NarInfoDiskCache::oInvalid || + !goodStorePath(storePath, res.second->path)) + throw InvalidPath("path '%s' is not valid", printStorePath(storePath)); + } + callback(ref(res.second)); + return true; + } + } + + return false; +} + void Store::queryPathInfo(const StorePath & storePath, Callback> callback) noexcept @@ -692,32 +728,8 @@ void Store::queryPathInfo(const StorePath & storePath, auto hashPart = std::string(storePath.hashPart()); try { - { - auto res = state.lock()->pathInfoCache.get(std::string(storePath.to_string())); - if (res && res->isKnownNow()) { - stats.narInfoReadAverted++; - if (!res->didExist()) - throw InvalidPath("path '%s' is not valid", printStorePath(storePath)); - return callback(ref(res->value)); - } - } - - if (diskCache) { - auto res = diskCache->lookupNarInfo(getUri(), hashPart); - if (res.first != NarInfoDiskCache::oUnknown) { - stats.narInfoReadAverted++; - { - auto state_(state.lock()); - state_->pathInfoCache.upsert(std::string(storePath.to_string()), - res.first == NarInfoDiskCache::oInvalid ? PathInfoCacheValue{} : PathInfoCacheValue{ .value = res.second }); - if (res.first == NarInfoDiskCache::oInvalid || - !goodStorePath(storePath, res.second->path)) - throw InvalidPath("path '%s' is not valid", printStorePath(storePath)); - } - return callback(ref(res.second)); - } - } - + if (queryPathInfoFromClientCache(storePath, callback)) + return; } catch (...) { return callback.rethrow(); } auto callbackPtr = std::make_shared(std::move(callback)); diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 9667b5e9e..2a1092d9e 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -282,6 +282,18 @@ public: void queryPathInfo(const StorePath & path, Callback> callback) noexcept; + /** + * NOTE: this is not the final interface - to be modified in next commit. + * + * Asynchronous version that only queries the local narinfo cache and not + * the actual store. + * + * @return true if the path was known and the callback invoked + * @return false if the path was not known and the callback not invoked + * @throw InvalidPathError if the path is known to be invalid + */ + bool queryPathInfoFromClientCache(const StorePath & path, Callback> & callback); + /** * Query the information about a realisation. */ From 8983ee8b2e0c10e6cac672a5a7ada4698235a62e Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Wed, 17 Jan 2024 17:54:03 +0100 Subject: [PATCH 2/3] refactor: Un-callback transform Store::queryPathInfoFromClientCache This part of the code was not necessarily callback based. Removing CPS is always nice; particularly if there's no loss of functionality, like here. --- src/libstore/store-api.cc | 18 +++++++++--------- src/libstore/store-api.hh | 8 ++++---- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 66bc95625..f237578e5 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -685,8 +685,7 @@ static bool goodStorePath(const StorePath & expected, const StorePath & actual) && (expected.name() == Store::MissingName || expected.name() == actual.name()); } -bool Store::queryPathInfoFromClientCache(const StorePath & storePath, - Callback> & callback) +std::optional> Store::queryPathInfoFromClientCache(const StorePath & storePath) { auto hashPart = std::string(storePath.hashPart()); @@ -696,8 +695,7 @@ bool Store::queryPathInfoFromClientCache(const StorePath & storePath, stats.narInfoReadAverted++; if (!res->didExist()) throw InvalidPath("path '%s' is not valid", printStorePath(storePath)); - callback(ref(res->value)); - return true; + return ref(res->value); } } @@ -713,12 +711,11 @@ bool Store::queryPathInfoFromClientCache(const StorePath & storePath, !goodStorePath(storePath, res.second->path)) throw InvalidPath("path '%s' is not valid", printStorePath(storePath)); } - callback(ref(res.second)); - return true; + return ref(res.second); } } - return false; + return std::nullopt; } @@ -728,8 +725,11 @@ void Store::queryPathInfo(const StorePath & storePath, auto hashPart = std::string(storePath.hashPart()); try { - if (queryPathInfoFromClientCache(storePath, callback)) - return; + auto r = queryPathInfoFromClientCache(storePath); + if (r.has_value()) { + ref & info = *r; + return callback(ref(info)); + } } catch (...) { return callback.rethrow(); } auto callbackPtr = std::make_shared(std::move(callback)); diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 2a1092d9e..e47f2c768 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -285,14 +285,14 @@ public: /** * NOTE: this is not the final interface - to be modified in next commit. * - * Asynchronous version that only queries the local narinfo cache and not + * Version of queryPathInfo() that only queries the local narinfo cache and not * the actual store. * - * @return true if the path was known and the callback invoked - * @return false if the path was not known and the callback not invoked + * @return `std::make_optional(vpi)` if the path is known + * @return `std::null_opt` if the path was not known to be valid or invalid * @throw InvalidPathError if the path is known to be invalid */ - bool queryPathInfoFromClientCache(const StorePath & path, Callback> & callback); + std::optional> queryPathInfoFromClientCache(const StorePath & path); /** * Query the information about a realisation. From d19627e8b4c3c09b0cc1329a9acaa8e5b070f26e Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 19 Jan 2024 17:00:39 +0100 Subject: [PATCH 3/3] refactor: Remove throw from queryPathInfoFromClientCache Return a value instead of throwing. Rather than the more trivial refactor of wrapping the return value in another std::optional, we retain the meaning of the outer optional: "we know at least something." So we have changed: return nullopt -> return nullopt throw InvalidPath -> return make_optional(nullptr) return vpi -> return make_optional(vpi) --- src/libstore/store-api.cc | 22 ++++++++++++++-------- src/libstore/store-api.hh | 10 ++++------ 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index f237578e5..2cd40d510 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -685,7 +685,8 @@ static bool goodStorePath(const StorePath & expected, const StorePath & actual) && (expected.name() == Store::MissingName || expected.name() == actual.name()); } -std::optional> Store::queryPathInfoFromClientCache(const StorePath & storePath) + +std::optional> Store::queryPathInfoFromClientCache(const StorePath & storePath) { auto hashPart = std::string(storePath.hashPart()); @@ -693,9 +694,10 @@ std::optional> Store::queryPathInfoFromClientCache(cons auto res = state.lock()->pathInfoCache.get(std::string(storePath.to_string())); if (res && res->isKnownNow()) { stats.narInfoReadAverted++; - if (!res->didExist()) - throw InvalidPath("path '%s' is not valid", printStorePath(storePath)); - return ref(res->value); + if (res->didExist()) + return std::make_optional(res->value); + else + return std::make_optional(nullptr); } } @@ -709,9 +711,10 @@ std::optional> Store::queryPathInfoFromClientCache(cons res.first == NarInfoDiskCache::oInvalid ? PathInfoCacheValue{} : PathInfoCacheValue{ .value = res.second }); if (res.first == NarInfoDiskCache::oInvalid || !goodStorePath(storePath, res.second->path)) - throw InvalidPath("path '%s' is not valid", printStorePath(storePath)); + return std::make_optional(nullptr); } - return ref(res.second); + assert(res.second); + return std::make_optional(res.second); } } @@ -727,8 +730,11 @@ void Store::queryPathInfo(const StorePath & storePath, try { auto r = queryPathInfoFromClientCache(storePath); if (r.has_value()) { - ref & info = *r; - return callback(ref(info)); + std::shared_ptr & info = *r; + if (info) + return callback(ref(info)); + else + throw InvalidPath("path '%s' is not valid", printStorePath(storePath)); } } catch (...) { return callback.rethrow(); } diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index e47f2c768..2f8a9440e 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -283,16 +283,14 @@ public: Callback> callback) noexcept; /** - * NOTE: this is not the final interface - to be modified in next commit. - * * Version of queryPathInfo() that only queries the local narinfo cache and not * the actual store. * - * @return `std::make_optional(vpi)` if the path is known - * @return `std::null_opt` if the path was not known to be valid or invalid - * @throw InvalidPathError if the path is known to be invalid + * @return `std::nullopt` if nothing is known about the path in the local narinfo cache. + * @return `std::make_optional(nullptr)` if the path is known to not exist. + * @return `std::make_optional(validPathInfo)` if the path is known to exist. */ - std::optional> queryPathInfoFromClientCache(const StorePath & path); + std::optional> queryPathInfoFromClientCache(const StorePath & path); /** * Query the information about a realisation.