-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Do less cloning of TH1 objects in the DQMStore (backport to 74x) #9267
Conversation
Previously, DQMStore would implicitly create a number of TH1 copies: func DQMStore::mergeAndResetMEsRunSummaryCache: ... MonitorElement global_me(*i); <- constructor makes an internal copy of th1 (fixed: 1) global_me.globalize() ... gme = data_.insert(global_me) <- insert calls the same copy constructor (fixed: 2) global_me is used as a key (in a set) to find an actual object to work on. It is not used outside that, and once the actual object is found, global_me is just left to be destroyed at the end of the loop. This is not a leak, but the TH1::Clone is just useless, since the key comparison does not depend on it. So I've implemented two things: (1) Special tagged constructor which does not clone the internal th1 object, and just sets it to nullptr (perfect for searching in a set). (2) Special move constructor which will move the TH1 object. The old object will have TH1 unset. This makes std::set::insert not to make a clone of th1 internally when inserting a transient (on the stack) object. (3) Disabled the assignment operator to avoid confusion/complexity. It was never actually used and move-constructor is complex enough already. Result: running 25.0 (with 1000 events) on 8 threads: - ~300mb less memory (peak rss) - 60s faster (5%) the code runs at end of the job, so in theory, the number of events does not matter. Only the number of streams used (it shouldn't improve anything on a single stream job). (cherry picked from commit 962a4b7)
Not sure why MonitorElement.{cc,h} had mode 755. (cherry picked from commit a85d0ce)
A new Pull Request was created by @dmitrijus (Dmitrijus) for CMSSW_7_4_X. Do less cloning of TH1 objects in the DQMStore (backport to 74x) It involves the following packages: DQMServices/Core @cmsbuild, @danduggan, @nclopezo, @deguio can you please review it and eventually sign? Thanks. |
+1 |
please test |
The tests are being triggered in jenkins. |
This pull request is fully signed and it will be integrated in one of the next CMSSW_7_4_X IBs unless changes or unless it breaks tests. This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @Degano, @smuzaffar |
This pull request is fully signed and it will be integrated in one of the next CMSSW_7_4_X IBs unless changes or unless it breaks tests. This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @Degano, @smuzaffar |
This pull request is fully signed and it will be integrated in one of the next CMSSW_7_4_X IBs unless changes (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @Degano, @smuzaffar |
+1 |
Do less cloning of TH1 objects in the DQMStore (backport to 74x)
(backport from #9066)
Previously, DQMStore would implicitly create a number of TH1 copies:
global_me is used as a key (in a set) to find an actual object to work on.
It is not used outside that and once the actual object is found,
global_me is just left to be destroyed at the end of the loop.
This is not a leak, but the TH1::Clone is just useless,
since the key comparison does not depend on it.
So I've implemented:
and just sets it to nullptr (perfect for searching in a set).
The old object will have TH1 unset.
This makes std::set::insert not to make a clone of TH1 internally
when inserting a transient (on the stack) object.
It was never actually used and move-constructor is complex enough already.
Result: running 25.0 (with 1000 events) on 8 threads:
The code runs at end of the job, so in theory, the number of events does not matter.
Only the number of streams used (it shouldn't improve anything on a single stream job).
Dmitrijus