Consumes migration for PFSimParticleProducer in RecoParticleFlow/PFSimProducer

[schoef]

Consumes migration for PFSimParticleProducer in RecoParticleFlow/PFSimProducer

[cmsbuild]

A new Pull Request was created by @schoef for CMSSW_7_6_X.

It involves the following packages:

RecoParticleFlow/PFSimProducer

@cmsbuild, @cvuosalo, @davidlange6, @slava77 can you please review it and eventually sign? Thanks.
@mmarionncern, @bachtis, @lgray this is something you requested to watch as well.
@slava77, @Degano, @smuzaffar you are the release manager for this.

Following commands in first line of a comment are recognized

• +1|approve[d]|sign[ed]: L1/L2's to approve it
• -1|reject[ed]: L1/L2's to reject it
• assign <category>[,<category>[,...]]: L1/L2's to request signatures from other categories
• unassign <category>[,<category>[,...]]: L1/L2's to remove signatures from other categories
• hold: L1/all L2's/release manager to mark it as on hold
• unhold: L1/user who put this PR on hold
• merge: L1/release managers to merge this request
• [@cmsbuild,] please test: L1/L2 and selected users to start jenkins tests
• [@cmsbuild,] please test with cms-sw/cmsdist#<PR>: L1/L2 and selected users to start jenkins tests using externals from cmsdist

[slava77]

@cmsbuild please test

[cmsbuild]

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

[cmsbuild]

+1
Tested at: ae8fcae
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-12496/9856/summary.html

[cmsbuild]

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

[cvuosalo]

Shouldn't the EDProducer inheritance in PFSimParticleProducer.h be fixed for thread safety? The static analyzer complains about it.

line 44: class PFSimParticleProducer : public edm::EDProducer {

[schoef]

@cvuosalo Is this the fix?
class PFSimParticleProducer : public edm::stream::EDProducer<> {

[cvuosalo]

@schoef: Yes, I think that will work.

[cvuosalo]

@schoef: On second thought, ::stream won't won't be reliable if there is non-thread-safe code somewhere in this producer. When you make the change, please test in multi-threaded mode to check for problems. To do so, you can append the following to your config file:

echo -e "process.options.numberOfStreams = cms.untracked.uint32(0)\n process.options.numberOfThreads = cms.untracked.uint32(4)\n" >> configFile

[schoef]

@cvuosalo Indeed, that test fails.
Could you let me know what else needs to change?
Thank you.

[lgray]

@schoef This code will need to be (completely) rewritten to be even thread friendly. Recommend that for now you use edm::one instead of edm::stream.

[schoef]

@lgray OK, afaik this producer isn't currently used anywhere except in JME studies. That's why I'm making the small updates.
I've changed to edm::one

[cmsbuild]

Pull request #12496 was updated. @cmsbuild, @cvuosalo, @davidlange6, @slava77 can you please check and sign again.

[cmsbuild]

Pull request #12496 was updated. @cmsbuild, @cvuosalo, @davidlange6, @slava77 can you please check and sign again.

[cvuosalo]

@schoef: Chris says edm::stream::EDProducer<> should work for replacing the thread-unsafe EDProducer. Does it?

[schoef]

@cvuosalo Yes, that works.

[cmsbuild]

Pull request #12496 was updated. @cmsbuild, @cvuosalo, @davidlange6, @slava77 can you please check and sign again.

[slava77]

@cmsbuild please test

[cmsbuild]

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

[cmsbuild]

+1
Tested at: 7684cc2
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-12496/10261/summary.html

[cmsbuild]

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

[cvuosalo]

+1

For #12496 7684cc2

Migrating PFSimParticleProducer to support thread safety. There should be no change in monitored quantities.

The code changes are satisfactory, and Jenkins tests against baseline CMSSW_7_6_X_2015-12-10-2300 show no significant differences, as expected.

[cmsbuild]

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_6_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @Degano, @smuzaffar

[cvuosalo]

@schoef: Please make an 80X version of this PR. Thanks.

[davidlange6]

+1