Do less cloning of TH1 objects in the DQMStore. #9066
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).
|
A new Pull Request was created by @dmitrijus (Dmitrijus) for CMSSW_7_5_X. Do less cloning of TH1 objects in the DQMStore. It involves the following packages: DQMServices/Core @cmsbuild, @danduggan, @nclopezo, @deguio can you please review it and eventually sign? Thanks. |
Not sure why MonitorElement.{cc,h} had mode 755.
|
Pull request #9066 was updated. @cmsbuild, @danduggan, @nclopezo, @deguio can you please check and sign again. |
|
please test |
|
The tests are being triggered in jenkins. |
|
Seems good to me |
cmsbuild
added
comparison-pending
and removed
comparison-pending
dqm-pending
orp-pending
pending-signatures
tests-started
labels
|
thanks @dmitrijus |
|
+1 |
|
This pull request is fully signed and it will be integrated in one of the next CMSSW_7_5_X IBs unless changes (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @nclopezo, @smuzaffar |
|
+1 |
Do less cloning of TH1 objects in the DQMStore.
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).
Please test before accepting, I am not that familiar with all the newest c++ stuff.
I will also ask someone to review the code today.
Dmitrijus