Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix thread safety in steppingHelixPropagator (supersede #3160) #3196

Merged

Conversation

slava77
Copy link
Contributor

@slava77 slava77 commented Apr 3, 2014

(by Chris) Removed mutables in SteppingHelixPropagator and instead put those data items on the stack when doing the operations.

Dr15Jones and others added 4 commits April 2, 2014 12:11
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.
@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 3, 2014

A new Pull Request was created by @slava77 (Slava Krutelyov) for CMSSW_7_1_X.

fix thread safety in steppingHelixPropagator (supersede #3160)

It involves the following packages:

Calibration/HcalAlCaRecoProducers
FastSimulation/MuonSimHitProducer
TrackPropagation/SteppingHelixPropagator
TrackingTools/TrackAssociator

@diguida, @lveldere, @cmsbuild, @anton-a, @thspeer, @rcastello, @giamman, @slava77, @Degano, @nclopezo can you please review it and eventually sign? Thanks.
@gpetruc, @cerati, @GiacomoSguazzoni, @rovere, @venturia this is something you requested to watch as well.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
@nclopezo, @ktf you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@slava77
Copy link
Contributor Author

slava77 commented Apr 4, 2014

This PR will not make it to pre6.
It will need more work to figure out remaining regressions

@@ -36,6 +36,22 @@
#include <sstream>
#include <typeinfo>

void SteppingHelixPropagator::initStateArraySHPSpecific(StateArray& svBuf, bool flagsOnly) const{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had hoped this wouldn't be necessary since I had hoped that anytime a new entry in the array was used it was first cleared. Oh well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, apparently the defaults for the propagator internals and the defaults created outside the propagator are different.
I don't quite remember all the logic behind it.
So, the solution is just to preserve the logic.

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 4, 2014

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 4, 2014

@Dr15Jones
Copy link
Contributor

This time the comparision looks dead on.

@slava77
Copy link
Contributor Author

slava77 commented Apr 4, 2014

you mean jenkins?
I have regressions in my tests (loss of reconstructed hits on hlt L2 muons)

I'm guessing the stats in jenkins tests are too low to detect problems in this case.

(In fact, I'm still suspicious of these relmon comparisons: on several occasions I observed serious regressions while jenkins reported zero changes;
and in some cases, like the diffs in
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_7_1_X_2014-04-02-0200+3160/1936/25.0_TTbar+TTbar+DIGI+RECO+HARVEST+ALCATT/comparisonResults/
the set of failed comparisons is indicative of a glitch in jenkins jobs than an actual difference.
I'll try to reproduce the situation with diffs in 3160 on the same stat of wflows that are run in jenkins.

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor Author

slava77 commented Apr 12, 2014

+1

for #3196 cdcb8d0

tested in CMSSW_7_1_X_2014-04-02-0200 test area sign333a.
remaining regressions, only in HLT L2 reco, are limited to 0.05% cases and affect just track parameters in a minor way. Debugging of the visible ones in 2K single-mu pt10 sample shows a problem in the old code.

@lveldere
Copy link
Contributor

+1

@rcastello
Copy link

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_1_X IBs unless changes (tests are also fine). @nclopezo, @ktf can you please take care of it?

ktf added a commit that referenced this pull request Apr 14, 2014
…readSafe

Multithreading -- Fix thread safety in steppingHelixPropagator (supersede #3160)
@ktf ktf merged commit 12d2d70 into cms-sw:CMSSW_7_1_X Apr 14, 2014
shervin86 pushed a commit to shervin86/cmssw that referenced this pull request May 11, 2015
…-shpThreadSafe

Multithreading -- Fix thread safety in steppingHelixPropagator (supersede cms-sw#3160)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants