Repository navigation
[v640][RF] Backport algorithmic complexity improvements - #23145
Merged
Merged
Conversation
`RooWorkspace::import()` deep-clones the imported computation graph twice: once to create a working copy, and then again "with renaming effective". The second clone is only needed because cloning re-resolves all server links by name, which is what makes the renaming take effect. If no node was actually renamed (no Rename*() command argument was given and there was no name conflict) the second clone is identical to the first one, which is then simply thrown away. This is the common case. Keep track of whether any node was really renamed and reuse the first working copy if none was. On a RooSimultaneous with 3200 channels (22403 nodes), `import()` goes from 0.77 s to 0.35 s. It also halves the peak memory usage, because both clone sets used to be alive at the same time. The varMap is changed from `std::map` to `std::unordered_map` as well, since it is only ever used for lookups and never iterated over. Also add a test for the different renaming modes, which were almost uncovered so far. Verified that the imported workspace contents (names, titles, server links, attributes and values) are byte-identical to before for all renaming modes. This change was motivated by reported performance bottlenecks in CMS Combine's `text2workspace` utility. (cherry picked from commit c58fbd9)
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 (cherry picked from commit efd3cd9)
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 (cherry picked from commit e779e55)
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 (cherry picked from commit fba704b)
Test Results 22 files 22 suites 3d 10h 36m 55s ⏱️ For more details on these failures, see this check. Results for commit 638ffee. |
dpiparo
merged commit Aug 25, 2026
c1dfb4f
into
root-project:v6-40-00-patches
33 of 37 checks passed
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.
Backporting the two PRs that improve algorithmic complexity in crucial places: