Preprocess EDAlias and SwitchProducer information for ConditionalTask #38730
Conversation
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38730/31032
|
|
A new Pull Request was created by @makortel (Matti Kortelainen) for master. It involves the following packages:
@cmsbuild, @smuzaffar, @Dr15Jones, @makortel can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild, please test |
|
@Dr15Jones, could you review this PR? |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a95a5d/26216/summary.html Comparison SummarySummary:
|
| auto const modnames = proc_pset.getParameter<vstring>(pathName); | ||
|
|
||
| //Pull out ConditionalTask modules | ||
| auto itCondBegin = std::find(modnames.begin(), modnames.end(), "#"); |
There was a problem hiding this comment.
It would be nice to find a way to share code between here and line 677
|
|
||
| std::multimap<std::string, AliasInfo> const& aliasMap() const { return aliasMap_; } | ||
|
|
||
| std::multimap<std::string, edm::BranchDescription const*> conditionalModuleBranches( |
There was a problem hiding this comment.
could this become a std::multimap<std::string_view, ...> to avoid possible copies? Also, what about std::multi_unordered_map?
There was a problem hiding this comment.
A hurdle with the std::string_view is that the element from this is used as a key for std::unordered_map<std::string>::find(), and before C++20 that key must be std::string (with C++20 anything that can be compared against std::string would work).
| switchEDAliases, allConditionalMods, proc_pset, processConfiguration.processName(), preg); | ||
| } | ||
|
|
||
| std::multimap<std::string, AliasInfo> aliasMap_; |
There was a problem hiding this comment.
what about std::unordered_multimap? Tends to have a smaller footprint and runs faster.
| @@ -166,10 +292,21 @@ namespace edm { | |||
| std::vector<std::string> const& pathNames = tns.getTrigPaths(); | |||
| std::vector<std::string> const& endPathNames = tns.getEndPaths(); | |||
|
|
|||
| ConditionalTaskHelper conditionalTaskHelper( | |||
There was a problem hiding this comment.
I can't quite remember, is the one StreamSchedule or one per stream? If the latter, then maybe ConditionalTaskHelper could be shared by them all? Depends on if workerManager_ is shared.
There was a problem hiding this comment.
There is indeed one StreamSchedule per stream, but each them have their own workerManager_. Maybe it/them could be shared, but it sounds like a potentially big task.
|
+1 I'll address the review comments in a subsequent PR given the urgency mentioned in #38731 (comment). |
|
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @qliphy, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
PR description:
Profiling #38725 in https://mkortela.web.cern.ch/mkortela/cgi-bin/navigator/issue38725/hlt_ConditionalTask_06_perf/12 showed that a lot of time in an HLT job was spent in
edm::ParameterSet::getParameter<edm::ParameterSet>()andedm::detail::processEDAliases(). These seemed to be caused by the processing of EDAliases and SwitchProducers for ConditionalTasks being repeated for all the Paths of the HLT menu (e.g. the SwitchProducer loop has N(paths) x N(modules) overall complexity).This PR attempts to speed up the job startup time by processing the EDAliases and SwitchProducers for ConditionalTasks only once for all Paths, leading to > 10x improvement in the
StreamScheduleconstructorhttps://mkortela.web.cern.ch/mkortela/cgi-bin/navigator/issue38725/hlt_ConditionalTask_36_perf/12
bringing the time close to what it would be if the HLT menu would use Tasks instead of ConditionalTasks
https://mkortela.web.cern.ch/mkortela/cgi-bin/navigator/issue38725/hlt_Task_06_perf/12
Resolves #38725
PR validation:
Framework unit tests run.