const thread-safe packed classes #11706

Dr15Jones · 2015-10-09T03:49:01Z

Made the mutable members of pat::PackedCandidate and pat::PackedGenParticle being changed in const functions be thread-safe. Added unit tests.

Made the updating of mutable members in const functions of pat::PackedCandidate thread-safe. Added a unit test for the class.

Made the updating of mutable members in const functions of pat::PackedGenParticle thread-safe. Added a unit test for the class.

Dr15Jones · 2015-10-09T03:51:07Z

I didn't know of anyway to actually test that reading back the classes work correctly.

cmsbuild · 2015-10-09T03:51:11Z

A new Pull Request was created by @Dr15Jones (Chris Jones) for CMSSW_7_6_X.

const thread-safe packed classes

It involves the following packages:

DataFormats/PatCandidates

@cmsbuild, @vadler, @monttj can you please review it and eventually sign? Thanks.
@gpetruc 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.
If you are a L2 or a release manager you can ask for tests by saying 'please test' in the first line of a comment.
@Degano you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

Dr15Jones · 2015-10-09T03:51:27Z

please test

cmsbuild · 2015-10-09T03:56:08Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/8715/console

Dr15Jones · 2015-10-09T03:58:18Z

DataFormats/PatCandidates/interface/PackedGenParticle.h

      statusFlags_(c.statusFlags()) { pack(); }
-  explicit PackedGenParticle( const reco::GenParticle & c, const edm::Ref<reco::GenParticleCollection> &  mother)
-    : p4_(c.pt(), c.eta(), c.phi(), c.mass()), p4c_(p4_), vertex_(0,0,0), pdgId_(c.pdgId()), charge_(c.charge()), mother_(mother), unpacked_(true),


NOTE: the vertex is not taken from the GenParticle. Is this intended?

cmsbuild · 2015-10-09T04:47:01Z

-1
Tested at: 9b8d11b
I found an error when building:

>> Compiling edm plugin /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_7_6_X_2015-10-08-2300/src/PhysicsTools/PatAlgos/plugins/PATPhotonProducer.cc 
/tmp/cmsbuild/workspace/ib-any-integration/CMSSW_7_6_X_2015-10-08-2300/src/PhysicsTools/PatAlgos/plugins/PATPackedCandidateProducer.cc: In instantiation of 'pat::PATPackedCandidateProducer::sort_indexes(const std::vector<_RealType>&) const:: [with T = pat::PackedCandidate; size_t = long unsigned int]':
/tmp/cmsbuild/workspace/ib-any-integration/CMSSW_7_6_X_2015-10-08-2300/src/PhysicsTools/PatAlgos/plugins/PATPackedCandidateProducer.cc:61:52:   required from 'struct pat::PATPackedCandidateProducer::sort_indexes(const std::vector<_RealType>&) const [with T = pat::PackedCandidate]::'
/tmp/cmsbuild/workspace/ib-any-integration/CMSSW_7_6_X_2015-10-08-2300/src/PhysicsTools/PatAlgos/plugins/PATPackedCandidateProducer.cc:61:117:   required from 'std::vector pat::PATPackedCandidateProducer::sort_indexes(const std::vector<_RealType>&) const [with T = pat::PackedCandidate]'
/tmp/cmsbuild/workspace/ib-any-integration/CMSSW_7_6_X_2015-10-08-2300/src/PhysicsTools/PatAlgos/plugins/PATPackedCandidateProducer.cc:291:52:   required from here
/tmp/cmsbuild/workspace/ib-any-integration/CMSSW_7_6_X_2015-10-08-2300/src/PhysicsTools/PatAlgos/plugins/PATPackedCandidateProducer.cc:61:116: error: no matching function for call to 'pat::PackedCandidate::PackedCandidate(const value_type&)'
               std::sort(idx.begin(), idx.end(),[&v,this](size_t i1, size_t i2) { return candsOrdering(v[i1],v[i2]);});
                                                                                                                    ^
/tmp/cmsbuild/workspace/ib-any-integration/CMSSW_7_6_X_2015-10-08-2300/src/PhysicsTools/PatAlgos/plugins/PATPackedCandidateProducer.cc:61:116: note: candidate is:
In file included from /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_7_6_X_2015-10-08-2300/src/PhysicsTools/PatAlgos/plugins/PATPackedCandidateProducer.cc:9:0:
/tmp/cmsbuild/workspace/ib-any-integration/CMSSW_7_6_X_2015-10-08-2300/src/DataFormats/PatCandidates/interface/PackedCandidate.h:34:5: note: pat::PackedCandidate::PackedCandidate()

you can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-11706/8715/summary.html

Using explicit with copy and move constructors causes problems with some common patterns. The version generated by default by the compiler do not use explicit.

Dr15Jones · 2015-10-09T13:29:24Z

please test

cmsbuild · 2015-10-09T13:31:18Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/8725/console

cmsbuild · 2015-10-09T13:31:20Z

Pull request #11706 was updated. @cmsbuild, @vadler, @monttj can you please check and sign again.

cmsbuild · 2015-10-09T16:31:03Z

+1
Tested at: b4e937a
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-11706/8725/summary.html

cmsbuild · 2015-10-09T17:30:55Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-11706/8725/summary.html

const thread-safe packed classes

wmtan · 2015-10-19T18:10:17Z

@davidlange6 @Dr15Jones Chris asked me to look at this, but I have no idea how to reproduce the problem. I do not know which workflows reproduce the problem. nor how to generate and view the distributions that do not match.

Dr15Jones · 2015-10-19T18:35:37Z

@wmtan I just drilled through the comparison to baseline plots and found 50202.0_TTbar_13+TTbar_13+DIGIUP15_PU50+RECOUP15_PU50+HARVESTUP15_PU50 to have a big difference if one clicks on that link, then goes to the 'ParticleFlow' and click on that pie chart then click on the pie chart for packedGenParticlesValidation then click on the 'CompWithGenParticles' pie chart.

So I suggest running workflow 50202.0. I searched from CompWithGenParticles and hit
http://cmslxr.fnal.gov/source/Validation/RecoParticleFlow/python/miniAODValidation_cff.py#0014
So you could look at that module to see what differences it is looking at.

wmtan · 2015-10-19T23:27:41Z

@Dr15Jones The problem easily reproduces. In step3 of relval 50202.0, pat::PackedGenParticle is produced. The distributions of all data members (e.g. pt, Y, etc) are totally screwed up. The differences are far from subtle, and you can tell it is screwed up just by looking. No need to compare with the distributions prior to this PR to tell the distributions are wrong. I'll try to debug this tomorrow.
Some good news: The pat::PackedCandidate distributions look OK, so the problem is confined to
pat::PackedGenParticle.

davidlange6 · 2015-10-20T06:00:41Z

and this may or may not be related - but lots of warnings coming (I think) from these changes

https://cmssdt.cern.ch/SDT/cgi-bin/buildlogs/slc6_amd64_gcc493/CMSSW_7_6_X_2015-10-19-1100/DataFormats/PatCandidates

On Oct 20, 2015, at 1:27 AM, wmtan notifications@github.com wrote:

@Dr15Jones The problem easily reproduces. In step3 of relval 50202.0, pat::PackedGenParticle is produced. The distributions of all data members (e.g. pt, Y, etc) are totally screwed up. The differences are far from subtle, and you can tell it is screwed up just by looking. No need to compare with the distributions prior to this PR to tell the distributions are wrong. I'll try to debug this tomorrow.
Some good news: The pat::PackedCandidate distributions look OK, so the problem is confined to
pat::PackedGenParticle.

—
Reply to this email directly or view it on GitHub.

Dr15Jones · 2015-10-20T11:29:34Z

We can make the warnings go away by using the macro CMS_THREAD_GUARD instead if the attribute directly.

wmtan · 2015-10-20T19:28:25Z

#12000 fixes the discrepancies. It now also fixes the build warnings.

Dr15Jones added 2 commits October 9, 2015 04:40

Made pat::PackedCandidate const thread-safe

0618591

Made the updating of mutable members in const functions of pat::PackedCandidate thread-safe. Added a unit test for the class.

Made pat::PackedGenParticle const thread-safe

9b8d11b

Made the updating of mutable members in const functions of pat::PackedGenParticle thread-safe. Added a unit test for the class.

cmsbuild added this to the Next CMSSW_7_6_X milestone Oct 9, 2015

cmsbuild added analysis-pending pending-signatures tests-pending orp-pending comparison-pending labels Oct 9, 2015

Dr15Jones reviewed Oct 9, 2015
View reviewed changes

cmsbuild added tests-started and removed tests-pending labels Oct 9, 2015

cmsbuild added tests-rejected and removed tests-started labels Oct 9, 2015

Removed incorrect explicit from copy and move constructors

b4e937a

Using explicit with copy and move constructors causes problems with some common patterns. The version generated by default by the compiler do not use explicit.

cmsbuild added tests-pending and removed tests-rejected labels Oct 9, 2015

cmsbuild added tests-started and removed tests-pending labels Oct 9, 2015

cmsbuild added tests-approved and removed tests-started labels Oct 9, 2015

cmsbuild added comparison-available and removed comparison-pending labels Oct 9, 2015

davidlange6 added a commit that referenced this pull request Oct 15, 2015

Merge pull request #11706 from Dr15Jones/threadSafePackedCandidate

cfc2edc

const thread-safe packed classes

davidlange6 merged commit cfc2edc into cms-sw:CMSSW_7_6_X Oct 15, 2015

davidlange6 mentioned this pull request Oct 17, 2015

replace most LinkDef.h files with XML selection files #11661

Merged

Dr15Jones deleted the threadSafePackedCandidate branch October 19, 2015 14:08

wmtan mentioned this pull request Oct 20, 2015

Fix packed gen particle copy and move ctors #12000

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

const thread-safe packed classes #11706

const thread-safe packed classes #11706

Dr15Jones commented Oct 9, 2015

Dr15Jones commented Oct 9, 2015

cmsbuild commented Oct 9, 2015

Dr15Jones commented Oct 9, 2015

cmsbuild commented Oct 9, 2015

Dr15Jones Oct 9, 2015

cmsbuild commented Oct 9, 2015

Dr15Jones commented Oct 9, 2015

cmsbuild commented Oct 9, 2015

cmsbuild commented Oct 9, 2015

cmsbuild commented Oct 9, 2015

cmsbuild commented Oct 9, 2015

wmtan commented Oct 19, 2015

Dr15Jones commented Oct 19, 2015

wmtan commented Oct 19, 2015

davidlange6 commented Oct 20, 2015

Dr15Jones commented Oct 20, 2015

wmtan commented Oct 20, 2015

const thread-safe packed classes #11706

const thread-safe packed classes #11706

Conversation

Dr15Jones commented Oct 9, 2015

Dr15Jones commented Oct 9, 2015

cmsbuild commented Oct 9, 2015

Dr15Jones commented Oct 9, 2015

cmsbuild commented Oct 9, 2015

Dr15Jones Oct 9, 2015

Choose a reason for hiding this comment

cmsbuild commented Oct 9, 2015

Dr15Jones commented Oct 9, 2015

cmsbuild commented Oct 9, 2015

cmsbuild commented Oct 9, 2015

cmsbuild commented Oct 9, 2015

cmsbuild commented Oct 9, 2015

wmtan commented Oct 19, 2015

Dr15Jones commented Oct 19, 2015

wmtan commented Oct 19, 2015

davidlange6 commented Oct 20, 2015

Dr15Jones commented Oct 20, 2015

wmtan commented Oct 20, 2015