attempt to fix SiStrip DQM reproducibility issue: allign members common to root and full c++ #20050
Conversation
|
A new Pull Request was created by @slava77 (Slava Krutelyov) for master. It involves the following packages: DPGAnalysis/SiStripTools @cmsbuild, @monttj can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
@Dr15Jones please check this to perhaps confirm that the old code can have ill-defined behavior (e.g. ::mult method returning inappropriate data member/memory address value depending on the order of loaded libraries or some other external factors) |
|
Breaking the C++ 'one definition' rule is extremely tricky to determine what might happen. If any of these classes were used in a container or C array, bad things could happen. Exactly what problem was this conditional compilation attempting to fix? |
|
If this object is used as a member data to another object (e.g. |
|
On 8/3/17 12:13 PM, Chris Jones wrote:
Breaking the C++ 'one definition' rule is extremely tricky to determine
what might happen. If any of these classes were used in a container or C
array, bad things could happen.
Exactly what problem was this conditional compilation attempting to fix?
I suppose that it was for EDGetToken not known to ROOTCLING
…
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#20050 (comment)>, or
mute the thread
<https://github.com/notifications/unsubscribe-auth/AEdcbkCN1aLxzisjOx9qUVyYkne3MYEYks5sUhvHgaJpZM4Os2cq>.
|
|
On 8/3/17 12:15 PM, Chris Jones wrote:
If this object is used as a member data to another object (e.g.
|edm::Wrapper<>|) then the padding will be different and things would
also break.
OK.
I'd rather this "fix" get merged and the developers of this code should
migrate to formats not violating the EDM rules.
…
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#20050 (comment)>, or
mute the thread
<https://github.com/notifications/unsubscribe-auth/AEdcbi8Odr4sMaQ1IuZo75uEIWG2Xy1yks5sUhxEgaJpZM4Os2cq>.
|
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
Looking at 1000.0, 4.53 and 136.731, the histograms are filled in the baseline+thisPR (red) compared to the baseline (blue) with empty histograms This "confirms" that the fix is working. |
|
merge |
DPGAnalysis/SiStripTools/interface/Multiplicities.h has different class data member content depending on the compilation mode (used for dictionary generation or for execution in algorithms at run time). This PR makes the layout for common data members to be the same.
This should hopefully resolve https://its.cern.ch/jira/browse/CMSTRACK-151
This is meant to solve the following problem
We frequently have SiStrip DQM plots coming and going altogether in
AlCaReco/Run summary/SiStrip/MechanicalViewfolder.These are produced by SiStripCalZeroBiasMonitorCluster, which is running in pathALCARECOSiStripCalZeroBias right after LargeSiStripClusterEvents.
The reason for empty plots is that an otherwise the same setup can have this module running when needed or not running at all by simply rerunning cmsRun with the same config.
The most direct reason for SiStripCalZeroBiasMonitorCluster to be on/off is that LargeSiStripClusterEvents filter can be on or off (while other modules preceding in the path appear to give consistently reproducible results).
I was not able to get a setup that leads to a reproducible bad behavior to be able to confirm from first principles where the problem is.
The only meaningful reason that I could see for this behavior was in this duality of the data layout in SingleMultiplicity class.
Note that the problem did not show up at noticeable level until December 9 2016,
even though the relevant change in the member data layout happened in 2014 in #3094.
Most likely the appearance of the issue is related to #16821 merged Dec 5.
The evidence of a fix to the problem is based on rerunning [extended] short matrix several times and on different machines to have better sampling using CMSSW_9_3_0_pre3 as the baseline.
A better solution is to properly decouple classes defined in DPGAnalysis/SiStripTools/interface/Multiplicities.h to be data formats and update the logic of ByMultiplicityEventFilter.
This will also remove the self-aware data product violation mentioned already in #16821 (comment)