From 9e797796d8ae85649457fb61375d6f8763ba98bd Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Mon, 24 Aug 2026 07:42:23 +0000 Subject: [PATCH 1/3] [RF] Make RooAbsCollection::sortTopologically() run in linear time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous implementation repeatedly moved single elements into place, each time with a linear search plus a linear-time vector insertion, which is quadratic in the collection size. For the large computation graphs of binned likelihood models, e.g. built by HistFactory, this sort dominated the construction time of every RooRealIntegral (it is also used in the RooFit::Evaluator setup). Replace it with an iterative depth-first post-order traversal over the server links that visits every arg and server edge only once. The traversal reproduces the previous ordering, including the substitution of same-name server instances, which matters for graphs containing cloned sub-trees (e.g. from RooAbsReal::createProjection()). Collections with multiple same-name entries fall back to the original algorithm, which preserves their size. The new implementation also terminates on cyclic graphs instead of looping forever. 🤖 Done with the help of AI --- roofit/roofitcore/src/RooAbsCollection.cxx | 93 ++++++++++++++++++---- 1 file changed, 76 insertions(+), 17 deletions(-) diff --git a/roofit/roofitcore/src/RooAbsCollection.cxx b/roofit/roofitcore/src/RooAbsCollection.cxx index 31759db42bc42..9fbc7f73ef916 100644 --- a/roofit/roofitcore/src/RooAbsCollection.cxx +++ b/roofit/roofitcore/src/RooAbsCollection.cxx @@ -48,7 +48,7 @@ implemented using the container denoted by RooAbsCollection::Storage_t. #include #include #include - +#include namespace RooFit { namespace Detail { @@ -1559,28 +1559,87 @@ void RooAbsCollection::sort(bool reverse) { /// that RooAbsArg in the collection. void RooAbsCollection::sortTopologically() { - std::unordered_set seenArgs; - for (std::size_t iArg = 0; iArg < _list.size(); ++iArg) { - RooAbsArg *arg = _list[iArg]; - bool movedArg = false; - for (RooAbsArg *server : arg->servers()) { - if (seenArgs.find(server->namePtr()) == seenArgs.end()) { - auto found = std::find_if(_list.begin(), _list.end(), - [server](RooAbsArg *elem) { return elem->namePtr() == server->namePtr(); }); - if (found != _list.end()) { - _list.erase(found); - _list.insert(_list.begin() + iArg, server); - movedArg = true; - break; + // Depth-first post-order traversal over the server edges, seeded with the + // args in collection order. Like in the original insertion-based + // implementation, servers are matched to collection members by name, with + // an encountered server instance standing in for a same-name member. + // Every arg and server link is visited only once. + std::unordered_set memberNames; + memberNames.reserve(_list.size()); + bool hasDuplicates = false; + for (RooAbsArg *arg : _list) { + hasDuplicates |= !memberNames.insert(arg->namePtr()).second; + } + + // The traversal emits each name once, which would shrink a collection + // with multiple same-name entries (allowed in a RooArgList). Fall back to + // the original algorithm in that case. + if (hasDuplicates) { + std::unordered_set seenArgsLegacy; + for (std::size_t iArg = 0; iArg < _list.size(); ++iArg) { + RooAbsArg *arg = _list[iArg]; + bool movedArg = false; + for (RooAbsArg *server : arg->servers()) { + if (seenArgsLegacy.find(server->namePtr()) == seenArgsLegacy.end()) { + auto found = std::find_if(_list.begin(), _list.end(), + [server](RooAbsArg *elem) { return elem->namePtr() == server->namePtr(); }); + if (found != _list.end()) { + _list.erase(found); + _list.insert(_list.begin() + iArg, server); + movedArg = true; + break; + } } } + if (movedArg) { + --iArg; + continue; + } + seenArgsLegacy.insert(arg->namePtr()); } - if (movedArg) { - --iArg; + return; + } + + Storage_t sorted; + sorted.reserve(_list.size()); + std::unordered_set seenArgs; + seenArgs.reserve(_list.size()); + // Names on the current traversal path, to skip cyclic server edges + // instead of looping forever. + std::unordered_set onStack; + + // Pairs of the arg to process and the index of the next of its servers to look at + std::vector> stack; + for (RooAbsArg *arg : _list) { + if (seenArgs.count(arg->namePtr())) continue; + stack.emplace_back(arg, 0); + onStack.insert(arg->namePtr()); + while (!stack.empty()) { + RooAbsArg *node = stack.back().first; + bool descended = false; + auto const &servers = node->servers(); + while (stack.back().second < servers.size()) { + RooAbsArg *server = servers[stack.back().second++]; + TNamed const *serverName = server->namePtr(); + if (memberNames.count(serverName) && !seenArgs.count(serverName) && !onStack.count(serverName)) { + stack.emplace_back(server, 0); + onStack.insert(serverName); + descended = true; + break; + } + } + if (descended) + continue; + if (seenArgs.insert(node->namePtr()).second) { + sorted.push_back(node); + } + onStack.erase(node->namePtr()); + stack.pop_back(); } - seenArgs.insert(arg->namePtr()); } + + _list = std::move(sorted); } //////////////////////////////////////////////////////////////////////////////// From 573d37b728a396e4cf3731510156bcc287d6a2af Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Mon, 24 Aug 2026 07:42:24 +0000 Subject: [PATCH 2/3] [RF] Avoid quadratic lookups in the RooRealIntegral constructor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The server analysis in getValueAndShapeServers() looked up graph nodes with the linear-scan RooAbsCollection::index(), and the recursive marking of dependent value clients revisited args once per path through the graph. Together with the previously quadratic topological sort, this made constructing a RooRealIntegral scale quadratically with the graph size, and the Asimov dataset generation the dominant cost of MakeModelAndMeasurementsFast() for HistFactory models. Build hash maps from the graph nodes to their index once and use them for all lookups, and turn the marking recursion into an iterative traversal that visits every arg at most once. Like before, dependent clients are matched by instance and servers by name, which matters for graphs containing cloned same-name sub-trees (e.g. from RooAbsReal::createProjection()). 🤖 Done with the help of AI --- roofit/roofitcore/src/RooRealIntegral.cxx | 54 +++++++++++++++++------ 1 file changed, 41 insertions(+), 13 deletions(-) diff --git a/roofit/roofitcore/src/RooRealIntegral.cxx b/roofit/roofitcore/src/RooRealIntegral.cxx index efecfb2fb3c42..b4d901facf4bd 100644 --- a/roofit/roofitcore/src/RooRealIntegral.cxx +++ b/roofit/roofitcore/src/RooRealIntegral.cxx @@ -46,7 +46,7 @@ integration is performed in the various implementations of the RooAbsIntegrator #include #include - +#include namespace { @@ -131,14 +131,26 @@ void addParameterToServers(RooAbsReal const &function, RooAbsArg &leaf, std::vec enum class MarkedState { Dependent, Independent, AlreadyAdded }; /// Mark all args that recursively are value clients of "dep". -void unmarkDepValueClients(RooAbsArg const &dep, RooArgSet const &args, std::vector &marked) +void unmarkDepValueClients(RooAbsArg const &dep, std::unordered_map const &indexMap, + std::vector &marked) { - assert(args.size() == marked.size()); - auto index = args.index(dep); - if (index >= 0) { - marked[index] = MarkedState::Dependent; - for (RooAbsArg *client : dep.valueClients()) { - unmarkDepValueClients(*client, args, marked); + auto found = indexMap.find(&dep); + if (found == indexMap.end()) + return; + marked[found->second] = MarkedState::Dependent; + + // Iterative depth-first traversal of the value clients within the + // computation graph, visiting every arg at most once. + std::vector stack{&dep}; + while (!stack.empty()) { + RooAbsArg const *arg = stack.back(); + stack.pop_back(); + for (RooAbsArg *client : arg->valueClients()) { + auto foundClient = indexMap.find(client); + if (foundClient != indexMap.end() && marked[foundClient->second] != MarkedState::Dependent) { + marked[foundClient->second] = MarkedState::Dependent; + stack.push_back(client); + } } } } @@ -154,6 +166,19 @@ getValueAndShapeServers(RooAbsReal const &function, RooArgSet const &depList, co RooArgSet allArgs{allArgsList}; allArgs.sortTopologically(); + // Maps from arg to index in allArgs for constant-time lookups. Two maps, + // because the graph can contain same-name instances (e.g. cloned sub-trees + // from projections): dependent client chains are matched by instance, + // while the final server matching is done by name. + std::unordered_map indexMap; + std::unordered_map indexMapByName; + indexMap.reserve(allArgs.size()); + indexMapByName.reserve(allArgs.size()); + for (std::size_t i = 0; i < allArgs.size(); ++i) { + indexMap.emplace(allArgs[i], i); + indexMapByName.emplace(allArgs[i]->namePtr(), i); + } + // Figure out what are all the value servers only RooArgList allValueArgsList; function.treeNodeServerList(&allValueArgsList, nullptr, true, true, /*valueOnly=*/true, false); @@ -161,7 +186,10 @@ getValueAndShapeServers(RooAbsReal const &function, RooArgSet const &depList, co // All "marked" args will be added as value servers to the integral std::vector marked(allArgs.size(), MarkedState::Independent); - marked.back() = MarkedState::Dependent; // We don't want to consider the function itself + // We don't want to consider the function itself + if (auto foundFunc = indexMap.find(&function); foundFunc != indexMap.end()) { + marked[foundFunc->second] = MarkedState::Dependent; + } // Mark all args that are (indirect) value servers of the integration // variable or the integration variable itself. If something was marked, @@ -169,7 +197,7 @@ getValueAndShapeServers(RooAbsReal const &function, RooArgSet const &depList, co // add it to the server list. for (RooAbsArg *dep : depList) { if (RooAbsArg *depInArgs = allArgs.find(dep->GetName())) { - unmarkDepValueClients(*depInArgs, allArgs, marked); + unmarkDepValueClients(*depInArgs, indexMap, marked); addObservableToServers(function, *depInArgs, serversToAdd, rangeName); } } @@ -179,10 +207,10 @@ getValueAndShapeServers(RooAbsReal const &function, RooArgSet const &depList, co for (std::size_t i = 0; i < allArgs.size(); ++i) { if (marked[i] == MarkedState::Dependent) { for (RooAbsArg *server : allArgs[i]->servers()) { - int index = allArgs.index(server->GetName()); - if (index >= 0 && marked[index] == MarkedState::Independent) { + auto found = indexMapByName.find(server->namePtr()); + if (found != indexMapByName.end() && marked[found->second] == MarkedState::Independent) { addParameterToServers(function, *server, serversToAdd, !allValueArgs.find(*server)); - marked[index] = MarkedState::AlreadyAdded; + marked[found->second] = MarkedState::AlreadyAdded; } } } From 881516a0c617fc1587147f8b8e80e4edb7732b5b Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Mon, 24 Aug 2026 07:42:24 +0000 Subject: [PATCH 3/3] [RF] Skip cloning recycled nodes in RooWorkspace::import() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When importing with RooFit::RecycleConflictNodes() and no renaming, import() deep-cloned the entire incoming computation graph, only to discard the clones of all nodes that already exist in the workspace. For workspaces that are built up incrementally, like in HistFactory, almost the whole graph is already present during later imports, so this was wasted work that scaled quadratically with the model size. In this mode, traverse the graph like the snapshot would, but clone only the nodes whose names are not in the workspace yet and connect them directly to the existing workspace nodes. Like before, new nodes below recycled ones are still imported, the workspace copies win for conflicting names, and the expensive-object cache payloads of recycled names are imported from the global cache instance. One deliberate difference: importWorkspaceHook() and the class-code import no longer run for recycled nodes, since there is no clone to run them on; their only lasting effect could have been auxiliary objects imported for a clone that was thrown away right after. For the HistFactory benchmark from issue #18683 (one channel with 100 samples, 200 systematics, and 50 bins), the three commits together reduce the runtime of MakeModelAndMeasurementsFast() from 38 s to about 6 s, and from 128 s to 30 s for five such channels. 🤖 Done with the help of AI --- roofit/roofitcore/src/RooWorkspace.cxx | 108 +++++++++++++++++--- roofit/roofitcore/test/testRooWorkspace.cxx | 101 ++++++++++++++++++ 2 files changed, 196 insertions(+), 13 deletions(-) diff --git a/roofit/roofitcore/src/RooWorkspace.cxx b/roofit/roofitcore/src/RooWorkspace.cxx index ab113a23f0da9..cdde1432ff2a9 100644 --- a/roofit/roofitcore/src/RooWorkspace.cxx +++ b/roofit/roofitcore/src/RooWorkspace.cxx @@ -88,6 +88,7 @@ and try reading again. #include #include #include +#include namespace { @@ -489,20 +490,32 @@ bool RooWorkspace::import(const RooAbsArg& inArg, } } - // Make list of conflicting nodes + // When existing nodes are recycled and no renaming is requested, nodes + // whose names already exist in the workspace are not cloned at all: the + // existing nodes are used anyway, and the clones would be discarded right + // after the import. One deliberate difference to the general code path: + // importWorkspaceHook() and the class-code import don't run for recycled + // nodes anymore, since there is no clone to run them on. + const bool pruneClones = useExistingNodes && !suffix && varMap.empty() && !noRecursion; + + // Make list of conflicting nodes. With useExistingNodes, this list stays + // empty, and the branch set is only otherwise used by the rename-all mode + // that excludes pruning, so the scan is skipped when pruning is active. RooArgSet conflictNodes ; RooArgSet branchSet ; - if (noRecursion) { - branchSet.add(inArg) ; - } else { - inArg.branchNodeServerList(&branchSet) ; - } + if (!pruneClones) { + if (noRecursion) { + branchSet.add(inArg); + } else { + inArg.branchNodeServerList(&branchSet); + } - for (const auto branch : branchSet) { - RooAbsArg* wsbranch = _allOwnedNodes.find(branch->GetName()) ; - if (wsbranch && wsbranch!=branch && !branch->getAttribute("RooWorkspace::Recycle") && !useExistingNodes) { - conflictNodes.add(*branch) ; - } + for (const auto branch : branchSet) { + RooAbsArg *wsbranch = _allOwnedNodes.find(branch->GetName()); + if (wsbranch && wsbranch != branch && !branch->getAttribute("RooWorkspace::Recycle") && !useExistingNodes) { + conflictNodes.add(*branch); + } + } } // Terminate here if there are conflicts and no resolution protocol @@ -515,7 +528,73 @@ bool RooWorkspace::import(const RooAbsArg& inArg, // Now create a working copy of the incoming object tree RooArgSet cloneSet; cloneSet.useHashMapForFind(true); // Accelerate finding - RooArgSet(inArg).snapshot(cloneSet, !noRecursion); + // Nodes of the input graph whose names already exist in the workspace and + // that are hence recycled instead of cloned. + std::vector recycledSources; + if (pruneClones) { + // Traverse the whole graph like the snapshot in the general code path + // does (depth-first pre-order over the server links, keeping the first + // encountered instance for each name), but clone only the nodes whose + // names are not in the workspace yet. + std::vector toClone; + std::unordered_set visited; + visited.insert(inArg.namePtr()); + std::vector stack{&inArg}; + while (!stack.empty()) { + RooAbsArg const *arg = stack.back(); + stack.pop_back(); + if (RooAbsArg *wsnode = _allOwnedNodes.find(arg->GetName())) { + if (!silence) { + coutI(ObjectHandling) << "RooWorkspace::import(" << GetName() << ") using existing copy of " + << wsnode->ClassName() << "::" << wsnode->GetName() << " for import of " + << inArg.ClassName() << "::" << inArg.GetName() << std::endl; + } + recycledSources.push_back(arg); + } else { + toClone.push_back(arg); + } + // Push the servers in reverse order so that they are popped in + // forward order, matching the traversal order of the snapshot. + auto const &servers = arg->servers().containedObjects(); + for (auto it = servers.rbegin(); it != servers.rend(); ++it) { + if (visited.insert((*it)->namePtr()).second) { + stack.push_back(*it); + } + } + } + for (RooAbsArg const *arg : toClone) { + auto clone = std::unique_ptr{static_cast(arg->Clone())}; + clone->setAttribute("SnapShot_ExtRefClone"); + cloneSet.addOwned(std::move(clone)); + } + // Redirect the server links of the clones to the other clones or to the + // existing nodes in the workspace. + RooArgSet redirectSet; + redirectSet.useHashMapForFind(true); + redirectSet.add(cloneSet); + for (RooAbsArg const *arg : recycledSources) { + redirectSet.add(*_allOwnedNodes.find(arg->GetName())); + } + for (RooAbsArg *clone : cloneSet) { + clone->redirectServers(redirectSet, true); + } + } else { + RooArgSet(inArg).snapshot(cloneSet, !noRecursion); + } + + // Import the expensive-object cache payloads for recycled nodes like the + // general code path does via the discarded clones, which always hand over + // the global cache instance because the cache pointer of a RooAbsArg is + // transient and not copied when cloning. + for (RooAbsArg const *arg : recycledSources) { + _eocache.importCacheObjects(RooExpensiveObjectCache::instance(), arg->GetName(), true); + } + + if (pruneClones && cloneSet.empty()) { + // The whole computation graph is already in the workspace. + return false; + } + RooAbsArg* cloneTop = cloneSet.find(inArg.GetName()) ; // Mark all nodes for renaming if we are not in conflictOnly mode @@ -528,7 +607,10 @@ bool RooWorkspace::import(const RooAbsArg& inArg, // Track whether any node in cloneSet actually gets renamed below: only then // does the working copy have to be cloned a second time (see there). bool nodesRenamed = false ; - string topName2 = cloneTop->GetName() ; + // With clone pruning, the top-level node itself may have been recycled, in + // which case it has no clone in cloneSet. No renaming can happen then, so + // the original name is already final. + string topName2 = cloneTop ? cloneTop->GetName() : inArg.GetName(); if (!renameConflictOrig) { // Mark all nodes to be imported for renaming following conflict resolution protocol for (const auto cnode : conflictNodes) { diff --git a/roofit/roofitcore/test/testRooWorkspace.cxx b/roofit/roofitcore/test/testRooWorkspace.cxx index 34d924e84e0b8..88f12353a9882 100644 --- a/roofit/roofitcore/test/testRooWorkspace.cxx +++ b/roofit/roofitcore/test/testRooWorkspace.cxx @@ -547,3 +547,104 @@ TEST(RooWorkspace, ImportRenamingModes) expectSelfContained(ws); } } + +/// Importing with RecycleConflictNodes() has to connect the imported nodes to +/// the same-name nodes already in the workspace instead of duplicating them. +/// This mode is used heavily when building workspaces incrementally, like in +/// HistFactory, so it takes a shortcut that skips cloning the already-imported +/// parts of the computation graph. Check that the resulting workspace is +/// correctly wired in the scenarios that shortcut has to handle. +TEST(RooWorkspace, ImportRecycleConflictNodes) +{ + RooHelpers::LocalChangeMsgLevel chmsglvl{RooFit::ERROR}; + + // Incremental build: import components first, then a top-level pdf reusing + // them. Only the top-level node is new. + { + ImportTestModel m; + RooWorkspace ws{"ws", "ws"}; + ws.import(m.gauss, RooFit::Silence()); + ws.import(m.expo, RooFit::Silence()); + ws.import(m.model, RooFit::RecycleConflictNodes(), RooFit::Silence()); + + ASSERT_NE(ws.pdf("model"), nullptr); + EXPECT_TRUE(hasServer(*ws.pdf("model"), *ws.pdf("gauss"))); + EXPECT_TRUE(hasServer(*ws.pdf("model"), *ws.pdf("expo"))); + EXPECT_DOUBLE_EQ(ws.pdf("model")->getVal(), m.model.getVal()); + expectSelfContained(ws); + } + + // Re-importing an already existing graph must be a no-op. + { + ImportTestModel m; + RooWorkspace ws{"ws", "ws"}; + ws.import(m.model, RooFit::Silence()); + RooAbsPdf *modelBefore = ws.pdf("model"); + const std::size_t nComponents = ws.components().size(); + EXPECT_FALSE(ws.import(m.model, RooFit::RecycleConflictNodes(), RooFit::Silence())); + + EXPECT_EQ(ws.pdf("model"), modelBefore); + EXPECT_EQ(ws.components().size(), nComponents); + expectSelfContained(ws); + } + + // The values of recycled nodes must come from the workspace copies, not + // from the incoming objects. + { + ImportTestModel m; + RooWorkspace ws{"ws", "ws"}; + ws.import(m.gauss, RooFit::Silence()); + m.mean.setVal(7.); // changed after the import: the workspace copy stays at 5 + ws.import(m.model, RooFit::RecycleConflictNodes(), RooFit::Silence()); + + EXPECT_DOUBLE_EQ(ws.var("mean")->getVal(), 5.); + expectSelfContained(ws); + } + + // When a same-name node conflicts, the workspace copy wins and keeps its + // own structure, but new nodes below the conflicting node are still + // imported (as unreferenced nodes), like in the general import code path. + { + RooWorkspace ws{"ws", "ws"}; + { + RooRealVar x{"x", "x", 0., 10.}; + RooRealVar mean{"mean", "mean", 5., 0., 10.}; + RooRealVar sigma{"sigma", "sigma", 1.0, 0.1, 5.0}; + RooGaussian sub{"sub", "sub", x, mean, sigma}; + ws.import(sub, RooFit::Silence()); + } + RooRealVar x{"x", "x", 0., 10.}; + RooRealVar theta{"theta", "theta", -0.1, -1.0, 0.0}; + RooExponential sub{"sub", "sub", x, theta}; // same name, different structure + RooRealVar f{"f", "f", 0.4, 0.0, 1.0}; + RooGaussian other{"other", "other", x, 1.0, 2.0}; + RooAddPdf top{"top", "top", RooArgList{sub, other}, f}; + ws.import(top, RooFit::RecycleConflictNodes(), RooFit::Silence()); + + // The workspace copy of "sub" keeps its structure ... + EXPECT_NE(ws.var("mean"), nullptr); + EXPECT_TRUE(hasServer(*ws.pdf("top"), *ws.pdf("sub"))); + EXPECT_TRUE(hasServer(*ws.pdf("sub"), *ws.var("mean"))); + // ... and the new parameter below the conflicting node is imported + EXPECT_NE(ws.var("theta"), nullptr); + expectSelfContained(ws); + } + + // A deeper boundary: the top-level pdf of the previous import becomes an + // intermediate node of the newly imported graph. + { + ImportTestModel m; + RooWorkspace ws{"ws", "ws"}; + ws.import(m.model, RooFit::Silence()); + RooRealVar y{"y", "y", 1.0, 0.0, 10.0}; + RooGaussian gaussy{"gaussy", "gaussy", y, m.mean, m.sigma}; + RooProdPdf prod{"prod", "prod", RooArgList{m.model, gaussy}}; + ws.import(prod, RooFit::RecycleConflictNodes(), RooFit::Silence()); + + ASSERT_NE(ws.pdf("prod"), nullptr); + EXPECT_TRUE(hasServer(*ws.pdf("prod"), *ws.pdf("model"))); + ASSERT_NE(ws.pdf("gaussy"), nullptr); + EXPECT_TRUE(hasServer(*ws.pdf("gaussy"), *ws.var("mean"))); + expectSelfContained(ws); + } +}