Fix thread safety problem in PhysicsTools/MVAComputer #7332
Conversation
ProcForeach was using mutable member data to keep track of the state of the looping it was doing. Given that multiple threads could be updating those values concurrently, this lead to crashes. The code has now been changed to store the looping state externally via VarProcessor::LoopCtx on the stack inside MVAComputer::evalInternal. This allows each thread to have its own copy.
Theoretically, making the allocator static could lead to an order of initialization problem. In reality the default allocator is stateless so wasn't a problem. However not being static simplifies understanding the code. Since non of the member functions are const we don't need to use mutable.
|
A new Pull Request was created by @Dr15Jones (Chris Jones) for CMSSW_7_4_X. Fix thread safety problem in PhysicsTools/MVAComputer It involves the following packages: PhysicsTools/MVAComputer @cmsbuild, @vadler, @nclopezo, @monttj can you please review it and eventually sign? Thanks. |
|
@Slava This problem was affecting RECO so you might wish to run your own tests. |
The static analyzer found that the registries were being accessed at event processing time by some modules. Given that we could get a shared library load happening while some other code was reading the registry, we needed to make the lookup and registration process threads safe. This was easy to accomplish using tbb::concurrent_unordered_map.
|
I took the opportunity to also fix a threading problem related to registration found by the static analyzer. I have no further changes I plan on making. |
|
@Dr15Jones you are tagging the wrong person |
|
@slava77 You might want to check this out since the problem was originally see in RECO. |
|
@cmsbuild please test |
1 similar comment
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
When initializing the value of 'size' for the LoopCtx we need to check that the 'size' has not yet been set. The bug only changed the value of 'size' if it had already been set.
|
@slava77 this last commit fixed the bug demonstrated by my unit test. Hopefully now everything should compare identically. Thanks for catching the problem. |
|
please test |
|
@cmsbuild please test maybe the bot listens to me |
|
nope, it didn't listen to me either :( |
|
Hi Chris, |
|
Woo hoo! Thanks for doing all the checking. |
|
Bypassing analysis signature. @monttj complain if not ok. |
Fix thread safety problem in PhysicsTools/MVAComputer
The CMSSW_7_4_THREADED_X IBs have been crashing due to MVA threading issues. The problem stemmed from the use of mutable in ProcForeach. These mutables were being read/written indirectly by GenericMVAJetTagComputer which is an EventSetup product. The simultaneous writes were causing the crashes.
The solution was to move the mutables to be external to the MVA class and instead be passed in as an argument to the functions. Encapsolating the loop state into variables that live on the stack allows each thread to have its own copy.