[RF] Avoid redundant second clone in RooWorkspace::import() - #23051
Open
guitargeek wants to merge 1 commit into
Open
[RF] Avoid redundant second clone in RooWorkspace::import()#23051guitargeek wants to merge 1 commit into
guitargeek wants to merge 1 commit into
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.
Test Results 23 files 23 suites 3d 14h 1m 53s ⏱️ Results for commit 4d4542b. |
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
1 participant
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.
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::maptostd::unordered_mapas 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
text2workspaceutility.