migrate modules used by the HLT menu to multithreading #10909

fwyzard · 2015-08-23T22:44:18Z

migrate the following modules to be stream or global modules:

EcalRecalibRecHitProducer
EgammaHLTNxNClusterProducer
HLTRegionalEcalResonanceFilter
IPTCorrector
IsolatedEcalPixelTrackCandidateProducer
IsolatedPixelTrackCandidateProducer
L1GlobalTrigger
L1TCaloUpgradeToGCTConverter
LaserAlignmentEventFilter
TriggerResultsFilter
TriggerResultsFilterFromDB

- change all configuration parameters into const members - initialise them in the member initializer list - remove unnecessary parameters - remove unnecessary caching of EventSetup products - remove unused methods and member variables - remove usage of exceptions in the FED check loop - change into a global::EDFilter

fwyzard · 2015-08-23T22:44:37Z

@cmsbuild please test

cmsbuild · 2015-08-23T22:46:12Z

The tests are being triggered in jenkins.

cmsbuild · 2015-08-23T22:46:13Z

A new Pull Request was created by @fwyzard (Andrea Bocci) for CMSSW_7_6_X.

migrate to modules used by the HLT menu multithreading

It involves the following packages:

Alignment/LaserAlignment
Calibration/HcalIsolatedTrackReco
HLTrigger/HLTfilters
HLTrigger/special
L1Trigger/GlobalTrigger
L1Trigger/L1TCalorimeter
RecoEgamma/EgammaHLTProducers
RecoLocalCalo/EcalRecProducers
RecoLocalMuon/RPCRecHit

@perrotta, @cmsbuild, @diguida, @cvuosalo, @fwyzard, @cerminar, @Martin-Grunewald, @slava77, @mmusich, @mulhearn can you please review it and eventually sign? Thanks.
@ghellwig, @Sam-Harper, @jhgoh, @argiro, @Martin-Grunewald, @bellan, @mschrode, @lgray 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.

slava77 · 2015-08-23T23:18:09Z

RecoLocalMuon/RPCRecHit/src/RPCPointProducer.cc

-  clock_gettime(CLOCK_REALTIME, &start_time);  
-  */
-
+void RPCPointProducer::produce(edm::StreamID, edm::Event& iEvent, const edm::EventSetup& iSetup) const {


this looks way too simple compared to the failed attempt in #9760
DTSegtoRPC etc are using global (static) cache variables

Thanks for pointing it out.
Indeed I have checked for static analysis reports only for the modules themselves, not for the other code that they use.
Is there a simple way to find all thread-safety issues related to a plugin ?

I know only of running helgrind
@Dr15Jones may know better

Best way to do it is to walk yourself through the code in CMSSW, lets you
catch everything but externals with problems and then really complicated
template code like stuff in reco::parser.

helgrind works but it is a bit noisey and thinks all of the std::atomic<>
variables are causing data races, hard to figure out what exactly is
happening.

On Mon, Aug 24, 2015 at 1:57 AM, Slava Krutelyov [email protected]
wrote:

In RecoLocalMuon/RPCRecHit/src/RPCPointProducer.cc
#10909 (comment):

-RPCPointProducer::~RPCPointProducer(){

-}

-void RPCPointProducer::produce(edm::Event& iEvent, const edm::EventSetup& iSetup){

/*

struct timespec start_time, stop_time;

time_t fs;

time_t fn;

time_t ls;

time_t ln;

clock_gettime(CLOCK_REALTIME, &start_time);

- */

+void RPCPointProducer::produce(edm::StreamID, edm::Event& iEvent, const edm::EventSetup& iSetup) const {

I know only of running helgrind
@Dr15Jones https://github.com/Dr15Jones may know better

—
Reply to this email directly or view it on GitHub
https://github.com/cms-sw/cmssw/pull/10909/files#r37712383.

cmsbuild · 2015-08-24T00:27:55Z

-1
Tested at: cf42e60
When I ran the RelVals I found an error in the following worklfows:
4.22 step1

DAS Error

1000.0 step1

DAS Error

1001.0 step1

DAS Error

1003.0 step1

DAS Error

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

fwyzard · 2015-08-24T10:23:36Z

I've dropped the migration of RPCPointProducer for the time being.

cmsbuild · 2015-08-24T10:26:34Z

Pull request #10909 was updated. @perrotta, @cmsbuild, @diguida, @cvuosalo, @fwyzard, @cerminar, @Martin-Grunewald, @slava77, @mmusich, @mulhearn can you please check and sign again.

Dr15Jones · 2015-08-24T11:51:32Z

Look at http://cms-sw.github.io/showIB.html and click on the link 'Modules to thread unsafe statics'. The link 'Modules to thread unsafe EventSetup products' isn't accurate because some safe EventSetup products are falsely marked

Dr15Jones · 2015-08-24T13:08:56Z

Calibration/HcalIsolatedTrackReco/interface/IsolatedEcalPixelTrackCandidateProducer.h

-  edm::EDGetTokenT<EcalRecHitCollection> tok_ee;
-  edm::EDGetTokenT<EcalRecHitCollection> tok_eb;
-  edm::EDGetTokenT<trigger::TriggerFilterObjectWithRefs> tok_trigcand;
+  virtual void beginJob() override;


The beingJob and endJob methods are empty, why not just get rid of them?

Dr15Jones · 2015-08-24T13:23:43Z

From a cursory look, everything seems fine. I also couldn't find any of the modified modules in the most recent 'module to statics' report:

https://cmssdt.cern.ch/SDT/jenkins-artifacts/ib-static-analysis/CMSSW_7_6_X_2015-08-23-2300/slc6_amd64_gcc493/reports/modules2statics.txt

cmsbuild · 2015-08-26T14:46:45Z

Pull request #10909 was updated. @perrotta, @cmsbuild, @diguida, @cvuosalo, @fwyzard, @cerminar, @Martin-Grunewald, @slava77, @mmusich, @mulhearn can you please check and sign again.

slava77 · 2015-08-26T14:48:17Z

uhm, a very timely rebase (just seconds after I signed)

slava77 · 2015-08-26T14:48:34Z

@cmsbuild please test

cmsbuild · 2015-08-26T14:51:40Z

The tests are being triggered in jenkins.

fwyzard · 2015-08-26T14:52:52Z

well, I did the rebase simply to fix the typo you spotted, do we really need to test it all again ?

slava77 · 2015-08-26T14:53:16Z

@fwyzard was it possible without a rebase?
I have no idea now what you changed (not from the web interface)

fwyzard · 2015-08-26T14:57:32Z

here is the diff between the version you signed (67f7701 ) and the new version ( 2259c5d ):

$ git diff 67f7701 2259c5d
diff --git a/RecoLocalCalo/EcalRecProducers/plugins/EcalRecalibRecHitProducer.cc b/RecoLocalCalo/EcalRecProducers/plugins/EcalRecalibRecHitProducer.cc
index e9fd90f..6b293d5 100644
--- a/RecoLocalCalo/EcalRecProducers/plugins/EcalRecalibRecHitProducer.cc
+++ b/RecoLocalCalo/EcalRecProducers/plugins/EcalRecalibRecHitProducer.cc
@@ -35,7 +35,7 @@ EcalRecalibRecHitProducer::EcalRecalibRecHitProducer(const edm::ParameterSet& ps
    EBRecHitCollection_(        ps.getParameter<edm::InputTag>("EBRecHitCollection") ),
    EERecHitCollection_(        ps.getParameter<edm::InputTag>("EERecHitCollection") ),
    EBRecHitToken_(             (not EBRecHitCollection_.label().empty()) ? consumes<EBRecHitCollection>(EBRecHitCollection_) : edm::EDGetTokenT<EBRecHitCollection>() ),
-   EERecHitToken_(             (not EERecHitCollection_.label().empty()) ? consumes<EBRecHitCollection>(EERecHitCollection_) : edm::EDGetTokenT<EERecHitCollection>() ),
+   EERecHitToken_(             (not EERecHitCollection_.label().empty()) ? consumes<EERecHitCollection>(EERecHitCollection_) : edm::EDGetTokenT<EERecHitCollection>() ),
    EBRecalibRecHitCollection_( ps.getParameter<std::string>("EBRecalibRecHitCollection") ),
    EERecalibRecHitCollection_( ps.getParameter<std::string>("EERecalibRecHitCollection") ),
    doEnergyScale_(             ps.getParameter<bool>("doEnergyScale") ),

fwyzard · 2015-08-26T15:00:31Z

By the way, if you prefer I can make a separate PR with only the RECO changes ?

slava77 · 2015-08-26T15:10:30Z

no particular preference, I can figure out which files are RECO-related.

cmsbuild · 2015-08-26T17:15:12Z

+1
Tested at: 2259c5d
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-10909/7665/summary.html

cmsbuild · 2015-08-26T17:51:31Z

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

slava77 · 2015-08-26T18:02:24Z

-1
superseded by a bunch.
Is there going to be a bunch in 75X as well?

Andrea, please close this one (or the others) so that we don't have duplicated features

fwyzard · 2015-08-27T06:05:47Z

OK, I've opened #10964 to keep track of all the related pull requests.

fwyzard added 2 commits August 23, 2015 23:25

EgammaHLTNxNClusterProducer: change into a stream::EDProducer

577f275

cmsbuild added this to the Next CMSSW_7_6_X milestone Aug 23, 2015

cmsbuild added reconstruction-pending l1-pending hlt-pending alca-pending pending-signatures tests-pending orp-pending comparison-pending labels Aug 23, 2015

fwyzard mentioned this pull request Aug 23, 2015

migrate modules used by the HLT menu to multithreading (75x) #10910

Closed

cmsbuild added tests-started and removed tests-pending labels Aug 23, 2015

slava77 reviewed Aug 23, 2015
View reviewed changes

cmsbuild added tests-rejected and removed tests-started labels Aug 24, 2015

fwyzard force-pushed the migrate_to_multithreading_75x branch from cf42e60 to eb41cda Compare August 24, 2015 10:21

cmsbuild added tests-pending and removed tests-rejected labels Aug 24, 2015

Dr15Jones reviewed Aug 24, 2015
View reviewed changes

cmsbuild added tests-started and removed tests-pending labels Aug 26, 2015

cmsbuild added tests-approved and removed tests-started labels Aug 26, 2015

cmsbuild added comparison-available and removed comparison-pending labels Aug 26, 2015

cmsbuild added reconstruction-rejected and removed reconstruction-pending labels Aug 26, 2015

fwyzard mentioned this pull request Aug 27, 2015

migrate modules used by the HLT menu to multithreading (76x) #10964

Closed

fwyzard closed this Aug 27, 2015

fwyzard deleted the migrate_to_multithreading_75x branch August 27, 2015 06:18

fwyzard changed the title ~~migrate to modules used by the HLT menu multithreading~~ migrate modules used by the HLT menu to multithreading Sep 4, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

migrate modules used by the HLT menu to multithreading #10909

migrate modules used by the HLT menu to multithreading #10909

fwyzard commented Aug 23, 2015

fwyzard commented Aug 23, 2015

cmsbuild commented Aug 23, 2015

cmsbuild commented Aug 23, 2015

slava77 Aug 23, 2015

fwyzard Aug 23, 2015

slava77 Aug 23, 2015

lgray Aug 24, 2015

cmsbuild commented Aug 24, 2015

fwyzard commented Aug 24, 2015

cmsbuild commented Aug 24, 2015

Dr15Jones commented Aug 24, 2015

Dr15Jones Aug 24, 2015

Dr15Jones commented Aug 24, 2015

cmsbuild commented Aug 26, 2015

slava77 commented Aug 26, 2015

slava77 commented Aug 26, 2015

cmsbuild commented Aug 26, 2015

fwyzard commented Aug 26, 2015

slava77 commented Aug 26, 2015

fwyzard commented Aug 26, 2015

fwyzard commented Aug 26, 2015

slava77 commented Aug 26, 2015

cmsbuild commented Aug 26, 2015

cmsbuild commented Aug 26, 2015

slava77 commented Aug 26, 2015

fwyzard commented Aug 27, 2015

migrate modules used by the HLT menu to multithreading #10909

migrate modules used by the HLT menu to multithreading #10909

Conversation

fwyzard commented Aug 23, 2015

fwyzard commented Aug 23, 2015

cmsbuild commented Aug 23, 2015

cmsbuild commented Aug 23, 2015

slava77 Aug 23, 2015

Choose a reason for hiding this comment

fwyzard Aug 23, 2015

Choose a reason for hiding this comment

slava77 Aug 23, 2015

Choose a reason for hiding this comment

lgray Aug 24, 2015

Choose a reason for hiding this comment

-RPCPointProducer::~RPCPointProducer(){

-}

- */

cmsbuild commented Aug 24, 2015

fwyzard commented Aug 24, 2015

cmsbuild commented Aug 24, 2015

Dr15Jones commented Aug 24, 2015

Dr15Jones Aug 24, 2015

Choose a reason for hiding this comment

Dr15Jones commented Aug 24, 2015

cmsbuild commented Aug 26, 2015

slava77 commented Aug 26, 2015

slava77 commented Aug 26, 2015

cmsbuild commented Aug 26, 2015

fwyzard commented Aug 26, 2015

slava77 commented Aug 26, 2015

fwyzard commented Aug 26, 2015

fwyzard commented Aug 26, 2015

slava77 commented Aug 26, 2015

cmsbuild commented Aug 26, 2015

cmsbuild commented Aug 26, 2015

slava77 commented Aug 26, 2015

fwyzard commented Aug 27, 2015