Fix thread safety of SteppingHelixPropagator #3160
Conversation
Removed mutables in SteppingHelixPropagator and instead put those data items on the stack when doing the operations.
This change avoids returning a copy and instead taking the value by argument. This only affects the methods new to SteppingHelixPropagator.
|
A new Pull Request was created by @Dr15Jones (Chris Jones) for CMSSW_7_1_X. Fix thread safety of SteppingHelixPropagator It involves the following packages: Calibration/HcalAlCaRecoProducers @diguida, @lveldere, @cmsbuild, @anton-a, @thspeer, @rcastello, @giamman, @slava77, @Degano, @nclopezo can you please review it and eventually sign? Thanks. |
|
+1 |
| @@ -80,7 +80,7 @@ void CachedTrajectory::propagate(SteppingHelixStateInfo& state, const Plane& pla | |||
| if( const SteppingHelixPropagator* shp = dynamic_cast<const SteppingHelixPropagator*>(propagator_) ) | |||
| { | |||
| try { | |||
| state = shp->propagate(state, plane); | |||
| shp->propagate(state, plane, state); | |||
There was a problem hiding this comment.
both "state" are refs and the second one is modified, while the first one is just read for info .. the result is not very predictable.
Need to define stateLast and stateNext to decouple these and then stateLast=stateNext after the call (or smth like that).
This place is most likely the reason for the regressions that I observe.
Chris, would you like to try to fix this or should I take over this PR?
There was a problem hiding this comment.
I'm fine with you taking over the pull, especially since you have ways of testing :). Thanks.
There was a problem hiding this comment.
One note, the first argument I thought was copied to a temporary area as the first step of the propagate call so the state shouldn't have interfered with itself. That's why I left it that way. However, it would have been easy for me to miss a corner case where the state initial value was used directly.
There was a problem hiding this comment.
indeed, the state is copied. Something else is wrong then :(
|
some regressions are visible, most likely due to the way the TrackingTools/TrackAssociator/src/CachedTrajectory.cc was modified. |
|
-1 superseded by #3196 |
…readSafe Multithreading -- Fix thread safety in steppingHelixPropagator (supersede #3160)
…-shpThreadSafe Multithreading -- Fix thread safety in steppingHelixPropagator (supersede cms-sw#3160)
Removed mutables in SteppingHelixPropagator and instead put those data items on the stack when doing the operations.