Repository navigation
[RF] Fix quadratic scaling when building large workspaces - #23139
Merged
Merged
Conversation
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
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
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 root-project#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
Test Results 23 files 23 suites 3d 11h 55m 17s ⏱️ For more details on these failures, see this check. Results for commit 881516a. |
dpiparo
self-requested a review
August 25, 2026 08:33
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR addresses the pathological scaling of HistFactory's
MakeModelAndMeasurementsFast()with model size reported in #18683. Rather than parallelizing over channels as the issue suggests, it removes the underlying quadratic bottlenecks, which profiling showed sit in RooFit core and are not HistFactory-specific:RooAbsCollection::sortTopologically()repeatedly moved single elements into place with linear searches and vector insertions. It is now an iterative depth-first post-order traversal that visits every arg and server edge once, reproducing the previous ordering -- including the same-name server instance substitution that graphs with cloned sub-trees (e.g. fromcreateProjection()) rely on. Collections with duplicate names fall back to the previous algorithm.RooRealIntegralconstructor used linearRooAbsCollection::index()lookups and a recursive client-marking that revisited args once per path through the graph. Hash-map lookups and an iterative traversal make it linear in the graph size; the by-instance vs. by-name matching semantics are unchanged.RooWorkspace::import()withRecycleConflictNodes()deep-cloned the whole incoming graph only to discard the clones of nodes already in the workspace. It now clones only the new nodes and connects them directly to the existing ones. One deliberate behavior change:importWorkspaceHook()and the class-code import no longer run for recycled nodes, since there is no clone to run them on.Benchmark from the issue (per channel: 100 samples, 200 systematics, 50 bins), runtime of
MakeModelAndMeasurementsFast():Verified with the full RooFit test suite, all
stressRooFitvariants, andstressHistFactory; workspaces built with and without the import pruning were checked to be identical. A new test,RooWorkspace.ImportRecycleConflictNodes, covers the pruned import path, including recycled top-level nodes and re-imports of existing graphs.A known remaining hotspot, the linear
RooSTLRefCountList::Add/Remove(~23 % of the improved runtime), is left for a follow-up since fixing it touches server-order semantics.Closes #18683
🤖 Done with the help of AI