Make ElectronTagger thread safe #8284
Conversation
Since the ElectronTagger can be obtained via the EventSetup, the const methods of the class must be thread safe. This required replacing TRandom3 with C++11 random facility (since TRandom3 uses a global seed) and using an std::mutex to protect MvaSoftEleEstimator since the way it interacts with TMVA::Reader is thread-unsafe.
|
This problem was found by helgrind. |
|
A new Pull Request was created by @Dr15Jones (Chris Jones) for CMSSW_7_4_X. Make ElectronTagger thread safe It involves the following packages: RecoBTag/SoftLepton @cmsbuild, @cvuosalo, @nclopezo, @slava77 can you please review it and eventually sign? Thanks. |
|
I guess the muon tagger should be fixed in a similar way since parts of the electron tagger code fixed by this PR are pretty much the same in the muon tagger. |
| @@ -11,13 +11,19 @@ float ElectronTagger::discriminator(const TagInfoHelper & tagInfo) const { | |||
| // default value, used if there are no leptons associated to this jet | |||
| float bestTag = - std::numeric_limits<float>::infinity(); | |||
| const reco::CandSoftLeptonTagInfo & info = tagInfo.get<reco::CandSoftLeptonTagInfo>(); | |||
|
|
|||
| std::default_random_engine random; | |||
There was a problem hiding this comment.
what's the meaning of default_random_engine?
Is it reproducible across platforms?
Looking at www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3551.pdf (just google search)
default_random_engine, an alias for an engine type selected by the
library vendor
... distinct vendors are free to select differently
... code that uses this alias need not
generate identical sequences across implementations
TRandom3 is Mersenne twister engine; I'm not sure this randomness quality is really required here, but at least the specifications for it are defined.
|
Please test |
|
I can switch to a set engine. |
|
The tests are being triggered in jenkins. |
Switch from the default random engine to the mersenne twister in order to have reproducibility across platforms.
|
Ping |
|
See #8304 for the Muon version |
|
Is there an ETA on approval? These fixes are needed to avoid problems in the threaded release. |
|
+1 for #8284 e5c6b6d there are no changes in monitored quantities, which suggests that the code affected by the random number selection/generation is actually not triggered (requires multiple candidates). I ran igprof on ttbar with pu35@25ns and observed some minor increase in CPU in the two affected modules (by this and #8304 PR) the increase is, unconvincingly, matching the igprof cost in MvaSoftEleEstimator::mvaValue, but maybe a jitter. The random engine-related elements are not showing up in igprof. |
|
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, @ktf, @smuzaffar |
|
+1 |
Make ElectronTagger thread safe
Since the ElectronTagger can be obtained via the EventSetup, the
const methods of the class must be thread safe. This required replacing
TRandom3 with C++11 random facility (since TRandom3 uses a global seed)
and using an std::mutex to protect MvaSoftEleEstimator since the
way it interacts with TMVA::Reader is thread-unsafe.