From 13b4512cbe1b13984e5242ab86523bbada9edadd Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 27 Nov 2025 18:56:38 -0500 Subject: [PATCH 1/3] topoSort: Optimize templating - No `std::function` overhead - Don't copy if not necessary Co-authored-by: Sergei Zimmerman --- src/libstore/local-store.cc | 8 +-- src/libstore/misc.cc | 14 ++--- src/libstore/unix/build/derivation-builder.cc | 52 +++++++++---------- src/libutil/include/nix/util/topo-sort.hh | 8 +-- 4 files changed, 42 insertions(+), 40 deletions(-) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index ab242bd84..f04ec79a6 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -989,10 +989,10 @@ void LocalStore::registerValidPaths(const ValidPathInfos & infos) error if a cycle is detected and roll back the transaction. Cycles can only occur when a derivation has multiple outputs. */ - auto topoSortResult = topoSort(paths, {[&](const StorePath & path) { - auto i = infos.find(path); - return i == infos.end() ? StorePathSet() : i->second.references; - }}); + auto topoSortResult = topoSort(paths, [&](const StorePath & path) { + auto i = infos.find(path); + return i == infos.end() ? StorePathSet() : i->second.references; + }); std::visit( overloaded{ diff --git a/src/libstore/misc.cc b/src/libstore/misc.cc index 9d11648d2..f9a339a00 100644 --- a/src/libstore/misc.cc +++ b/src/libstore/misc.cc @@ -313,13 +313,13 @@ MissingPaths Store::queryMissing(const std::vector & targets) StorePaths Store::topoSortPaths(const StorePathSet & paths) { - auto result = topoSort(paths, {[&](const StorePath & path) { - try { - return queryPathInfo(path)->references; - } catch (InvalidPath &) { - return StorePathSet(); - } - }}); + auto result = topoSort(paths, [&](const StorePath & path) { + try { + return queryPathInfo(path)->references; + } catch (InvalidPath &) { + return StorePathSet(); + } + }); return std::visit( overloaded{ diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index 333c4dff8..428430086 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -1470,32 +1470,32 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs() outputStats.insert_or_assign(outputName, std::move(st)); } - auto topoSortResult = topoSort(outputsToSort, {[&](const std::string & name) { - auto orifu = get(outputReferencesIfUnregistered, name); - if (!orifu) - throw BuildError( - BuildResult::Failure::OutputRejected, - "no output reference for '%s' in build of '%s'", - name, - store.printStorePath(drvPath)); - return std::visit( - overloaded{ - /* Since we'll use the already installed versions of these, we - can treat them as leaves and ignore any references they - have. */ - [&](const AlreadyRegistered &) { return StringSet{}; }, - [&](const PerhapsNeedToRegister & refs) { - StringSet referencedOutputs; - /* FIXME build inverted map up front so no quadratic waste here */ - for (auto & r : refs.refs) - for (auto & [o, p] : scratchOutputs) - if (r == p) - referencedOutputs.insert(o); - return referencedOutputs; - }, - }, - *orifu); - }}); + auto topoSortResult = topoSort(outputsToSort, [&](const std::string & name) { + auto orifu = get(outputReferencesIfUnregistered, name); + if (!orifu) + throw BuildError( + BuildResult::Failure::OutputRejected, + "no output reference for '%s' in build of '%s'", + name, + store.printStorePath(drvPath)); + return std::visit( + overloaded{ + /* Since we'll use the already installed versions of these, we + can treat them as leaves and ignore any references they + have. */ + [&](const AlreadyRegistered &) { return StringSet{}; }, + [&](const PerhapsNeedToRegister & refs) { + StringSet referencedOutputs; + /* FIXME build inverted map up front so no quadratic waste here */ + for (auto & r : refs.refs) + for (auto & [o, p] : scratchOutputs) + if (r == p) + referencedOutputs.insert(o); + return referencedOutputs; + }, + }, + *orifu); + }); auto sortedOutputNames = std::visit( overloaded{ diff --git a/src/libutil/include/nix/util/topo-sort.hh b/src/libutil/include/nix/util/topo-sort.hh index 285c34316..fb918117b 100644 --- a/src/libutil/include/nix/util/topo-sort.hh +++ b/src/libutil/include/nix/util/topo-sort.hh @@ -3,6 +3,7 @@ #include "nix/util/error.hh" #include +#include namespace nix { @@ -16,8 +17,9 @@ struct Cycle template using TopoSortResult = std::variant, Cycle>; -template -TopoSortResult topoSort(std::set items, std::function(const T &)> getChildren) +template F> + requires std::same_as>, std::set> +TopoSortResult topoSort(std::set items, F && getChildren) { std::vector sorted; decltype(items) visited, parents; @@ -34,7 +36,7 @@ TopoSortResult topoSort(std::set items, std::function Date: Tue, 25 Nov 2025 16:52:43 -0500 Subject: [PATCH 2/3] perf(libstore/derivation-builder): pre-compute outputGraph for linear complexity Build the inverse of `scratchOuputs` before running topoSort, avoiding quadratic complexity when determining which outputs reference each other. This fixes the FIXME comment about building the inverted map up front. Inspired by Lix commit 10c04ce84 / Change Id Ibdd46e7b2e895bfeeebc173046d1297b41998181, but ended up being completely different code. Co-Authored-By: Maximilian Bosch Co-Authored-By: Bernardo Meurer Costa --- src/libstore/unix/build/derivation-builder.cc | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index 428430086..ebd53472b 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -1398,6 +1398,11 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs() StorePathSet refs; }; + /* inverse map of scratchOutputs for efficient lookup */ + std::map scratchOutputsInverse; + for (auto & [outputName, path] : scratchOutputs) + scratchOutputsInverse.insert_or_assign(path, outputName); + std::map> outputReferencesIfUnregistered; std::map outputStats; for (auto & [outputName, _] : drv.outputs) { @@ -1486,11 +1491,9 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs() [&](const AlreadyRegistered &) { return StringSet{}; }, [&](const PerhapsNeedToRegister & refs) { StringSet referencedOutputs; - /* FIXME build inverted map up front so no quadratic waste here */ for (auto & r : refs.refs) - for (auto & [o, p] : scratchOutputs) - if (r == p) - referencedOutputs.insert(o); + if (auto * o = get(scratchOutputsInverse, r)) + referencedOutputs.insert(*o); return referencedOutputs; }, }, From c33b2c583466b14d23bc85caa35f737886842e21 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 25 Nov 2025 17:01:23 -0500 Subject: [PATCH 3/3] perf(libstore/derivation-builder): Futher simplify / maybe optimize We can precompute the exact information we need for topo sorting and store it in `PerhapsNeedToRegister`. Depending on how `topoSort` works, this is easy a performance improvement or just completely harmless. Co-Authored-By: Bernardo Meurer Costa --- src/libstore/unix/build/derivation-builder.cc | 33 ++++++++++++------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index ebd53472b..2d66eb405 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -1396,6 +1396,11 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs() struct PerhapsNeedToRegister { StorePathSet refs; + /** + * References to other outputs. Built by looking up in + * `scratchOutputsInverse`. + */ + StringSet otherOutputs; }; /* inverse map of scratchOutputs for efficient lookup */ @@ -1471,12 +1476,24 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs() references = scanForReferences(blank, actualPath, referenceablePaths); } - outputReferencesIfUnregistered.insert_or_assign(outputName, PerhapsNeedToRegister{.refs = references}); + StringSet referencedOutputs; + for (auto & r : references) + if (auto * o = get(scratchOutputsInverse, r)) + referencedOutputs.insert(*o); + + outputReferencesIfUnregistered.insert_or_assign( + outputName, + PerhapsNeedToRegister{ + .refs = references, + .otherOutputs = referencedOutputs, + }); outputStats.insert_or_assign(outputName, std::move(st)); } - auto topoSortResult = topoSort(outputsToSort, [&](const std::string & name) { - auto orifu = get(outputReferencesIfUnregistered, name); + StringSet emptySet; + + auto topoSortResult = topoSort(outputsToSort, [&](const std::string & name) -> const StringSet & { + auto * orifu = get(outputReferencesIfUnregistered, name); if (!orifu) throw BuildError( BuildResult::Failure::OutputRejected, @@ -1488,14 +1505,8 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs() /* Since we'll use the already installed versions of these, we can treat them as leaves and ignore any references they have. */ - [&](const AlreadyRegistered &) { return StringSet{}; }, - [&](const PerhapsNeedToRegister & refs) { - StringSet referencedOutputs; - for (auto & r : refs.refs) - if (auto * o = get(scratchOutputsInverse, r)) - referencedOutputs.insert(*o); - return referencedOutputs; - }, + [&](const AlreadyRegistered &) -> const StringSet & { return emptySet; }, + [&](const PerhapsNeedToRegister & refs) -> const StringSet & { return refs.otherOutputs; }, }, *orifu); });