From d1f20e3510ef39bc36287773cff35383e824ebc0 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Mon, 16 Dec 2024 14:58:25 +0100 Subject: [PATCH 1/3] Make FetchedFlake a struct --- src/libflake/flake/flake.cc | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/src/libflake/flake/flake.cc b/src/libflake/flake/flake.cc index bc62e0bc5..0bdaeb789 100644 --- a/src/libflake/flake/flake.cc +++ b/src/libflake/flake/flake.cc @@ -21,7 +21,12 @@ using namespace flake; namespace flake { -typedef std::pair FetchedFlake; +struct FetchedFlake +{ + FlakeRef lockedRef; + StorePath storePath; +}; + typedef std::vector> FlakeCache; static std::optional lookupInFlakeCache( @@ -32,7 +37,7 @@ static std::optional lookupInFlakeCache( for (auto & i : flakeCache) { if (flakeRef == i.first) { debug("mapping '%s' to previously seen input '%s' -> '%s", - flakeRef, i.first, i.second.second); + flakeRef, i.first, i.second.lockedRef); return i.second; } } @@ -51,7 +56,8 @@ static std::tuple fetchOrSubstituteTree( if (!fetched) { if (originalRef.input.isDirect()) { - fetched.emplace(originalRef.fetchTree(state.store)); + auto [storePath, lockedRef] = originalRef.fetchTree(state.store); + fetched.emplace(FetchedFlake{.lockedRef = lockedRef, .storePath = storePath}); } else { if (allowLookup) { resolvedRef = originalRef.resolve( @@ -61,10 +67,12 @@ static std::tuple fetchOrSubstituteTree( to resolve indirect flakerefs. */ return type == fetchers::Registry::Flag || type == fetchers::Registry::Global; }); - auto fetchedResolved = lookupInFlakeCache(flakeCache, originalRef); - if (!fetchedResolved) fetchedResolved.emplace(resolvedRef.fetchTree(state.store)); - flakeCache.push_back({resolvedRef, *fetchedResolved}); - fetched.emplace(*fetchedResolved); + fetched = lookupInFlakeCache(flakeCache, originalRef); + if (!fetched) { + auto [storePath, lockedRef] = resolvedRef.fetchTree(state.store); + fetched.emplace(FetchedFlake{.lockedRef = lockedRef, .storePath = storePath}); + } + flakeCache.push_back({resolvedRef, *fetched}); } else { throw Error("'%s' is an indirect flake reference, but registry lookups are not allowed", originalRef); @@ -73,16 +81,14 @@ static std::tuple fetchOrSubstituteTree( flakeCache.push_back({originalRef, *fetched}); } - auto [storePath, lockedRef] = *fetched; - debug("got tree '%s' from '%s'", - state.store->printStorePath(storePath), lockedRef); + state.store->printStorePath(fetched->storePath), fetched->lockedRef); - state.allowPath(storePath); + state.allowPath(fetched->storePath); - assert(!originalRef.input.getNarHash() || storePath == originalRef.input.computeStorePath(*state.store)); + assert(!originalRef.input.getNarHash() || fetched->storePath == originalRef.input.computeStorePath(*state.store)); - return {std::move(storePath), resolvedRef, lockedRef}; + return {fetched->storePath, resolvedRef, fetched->lockedRef}; } static void forceTrivialValue(EvalState & state, Value & value, const PosIdx pos) From d2e1d4916a7a74ba75dbbdff0f696acb0859dbfd Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 10 Dec 2024 15:30:55 +0100 Subject: [PATCH 2/3] lookupInFlakeCache(): Fix O(n) time lookup --- src/libfetchers/fetchers.hh | 5 +++++ src/libflake/flake/flake.cc | 21 ++++++++------------- src/libflake/flake/flakeref.hh | 2 ++ src/libutil/types.hh | 7 +++---- 4 files changed, 18 insertions(+), 17 deletions(-) diff --git a/src/libfetchers/fetchers.hh b/src/libfetchers/fetchers.hh index b28ec4568..94b2320f9 100644 --- a/src/libfetchers/fetchers.hh +++ b/src/libfetchers/fetchers.hh @@ -104,6 +104,11 @@ public: bool operator ==(const Input & other) const noexcept; + auto operator <=>(const Input & other) const + { + return attrs <=> other.attrs; + } + bool contains(const Input & other) const; /** diff --git a/src/libflake/flake/flake.cc b/src/libflake/flake/flake.cc index 0bdaeb789..29090b900 100644 --- a/src/libflake/flake/flake.cc +++ b/src/libflake/flake/flake.cc @@ -27,22 +27,17 @@ struct FetchedFlake StorePath storePath; }; -typedef std::vector> FlakeCache; +typedef std::map FlakeCache; static std::optional lookupInFlakeCache( const FlakeCache & flakeCache, const FlakeRef & flakeRef) { - // FIXME: inefficient. - for (auto & i : flakeCache) { - if (flakeRef == i.first) { - debug("mapping '%s' to previously seen input '%s' -> '%s", - flakeRef, i.first, i.second.lockedRef); - return i.second; - } - } - - return std::nullopt; + auto i = flakeCache.find(flakeRef); + if (i == flakeCache.end()) return std::nullopt; + debug("mapping '%s' to previously seen input '%s' -> '%s", + flakeRef, i->first, i->second.lockedRef); + return i->second; } static std::tuple fetchOrSubstituteTree( @@ -72,13 +67,13 @@ static std::tuple fetchOrSubstituteTree( auto [storePath, lockedRef] = resolvedRef.fetchTree(state.store); fetched.emplace(FetchedFlake{.lockedRef = lockedRef, .storePath = storePath}); } - flakeCache.push_back({resolvedRef, *fetched}); + flakeCache.insert_or_assign(resolvedRef, *fetched); } else { throw Error("'%s' is an indirect flake reference, but registry lookups are not allowed", originalRef); } } - flakeCache.push_back({originalRef, *fetched}); + flakeCache.insert_or_assign(originalRef, *fetched); } debug("got tree '%s' from '%s'", diff --git a/src/libflake/flake/flakeref.hh b/src/libflake/flake/flakeref.hh index 80013e87e..2ba01c72b 100644 --- a/src/libflake/flake/flakeref.hh +++ b/src/libflake/flake/flakeref.hh @@ -49,6 +49,8 @@ struct FlakeRef bool operator ==(const FlakeRef & other) const = default; + auto operator <=>(const FlakeRef &) const = default; + FlakeRef(fetchers::Input && input, const Path & subdir) : input(std::move(input)), subdir(subdir) { } diff --git a/src/libutil/types.hh b/src/libutil/types.hh index 325e3ea73..f30dd910e 100644 --- a/src/libutil/types.hh +++ b/src/libutil/types.hh @@ -43,10 +43,9 @@ template struct Explicit { T t; - bool operator ==(const Explicit & other) const - { - return t == other.t; - } + bool operator ==(const Explicit & other) const = default; + + auto operator <=>(const Explicit & other) const = default; }; From b167e2c415684518626265abfbdb3b756f04ee1b Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 11 Dec 2024 20:59:59 +0100 Subject: [PATCH 3/3] Work around clang/libc++ issue --- src/libfetchers/fetchers.hh | 4 ++-- src/libflake/flake/flakeref.hh | 5 ++++- src/libutil/types.hh | 5 ++++- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/libfetchers/fetchers.hh b/src/libfetchers/fetchers.hh index 94b2320f9..ff04d5551 100644 --- a/src/libfetchers/fetchers.hh +++ b/src/libfetchers/fetchers.hh @@ -104,9 +104,9 @@ public: bool operator ==(const Input & other) const noexcept; - auto operator <=>(const Input & other) const + bool operator <(const Input & other) const { - return attrs <=> other.attrs; + return attrs < other.attrs; } bool contains(const Input & other) const; diff --git a/src/libflake/flake/flakeref.hh b/src/libflake/flake/flakeref.hh index 2ba01c72b..ec755399d 100644 --- a/src/libflake/flake/flakeref.hh +++ b/src/libflake/flake/flakeref.hh @@ -49,7 +49,10 @@ struct FlakeRef bool operator ==(const FlakeRef & other) const = default; - auto operator <=>(const FlakeRef &) const = default; + bool operator <(const FlakeRef & other) const + { + return std::tie(input, subdir) < std::tie(other.input, other.subdir); + } FlakeRef(fetchers::Input && input, const Path & subdir) : input(std::move(input)), subdir(subdir) diff --git a/src/libutil/types.hh b/src/libutil/types.hh index f30dd910e..9f5c75827 100644 --- a/src/libutil/types.hh +++ b/src/libutil/types.hh @@ -45,7 +45,10 @@ struct Explicit { bool operator ==(const Explicit & other) const = default; - auto operator <=>(const Explicit & other) const = default; + bool operator <(const Explicit & other) const + { + return t < other.t; + } };