Switch GetterOfProducts to use Event::getByToken #1591

Dr15Jones · 2013-11-25T23:17:29Z

Performance measurements by Vincenzo Innocente showed that the HLT was spending too much time in edm::GetterOfProducts. The time was caused by safety checks in Event::getByLabel. This code changes the class to use Event::getByToken which avoids those checks by design.

This also includes a fix for a typo in the name of EDGetToken*::isUninitialized as well as a bug fix which prevented getByToken to work for Runs and Lumis.

The test which makes sure the getByToken for a particular principal matches the type used in the ‘consumes’ was hard coded to only match Event. This change uses the proper type when doing the comparison.

Fixed typo in the function name EDGetToken*::isUninitialized.

Performance measurements of HLT by Vincenzo Innocente showed edm::GetterOfProducts was taking a large fraction of time. The slowness was from the checks done in Event::getByLabel. This changes switches the code to use getByToken when avoids those checks by construction.

This fixes the non-framework uses of the misspelled function name.

cmsbuild · 2013-11-25T23:18:03Z

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

Switch GetterOfProducts to use Event::getByToken

It involves the following packages:

HLTrigger/btau
FWCore/Utilities
DQM/L1TMonitor
HLTrigger/HLTanalyzers
FWCore/Framework
FWCore/Integration
HLTrigger/HLTcore

@perrotta, @cmsbuild, @Dr15Jones, @danduggan, @rovere, @Martin-Grunewald, @nclopezo, @deguio, @fwyzard, @eliasron, @ktf can you please review it and eventually sign? Thanks.
@cms-btv-pog, @wmtan 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.
@ktf you are the release manager for this.

Dr15Jones · 2013-11-25T23:19:03Z

+1
@ktf most of the non Core changes were just to fix a typo in a member function name and therefore causes no functional changes.

ktf · 2013-11-26T08:37:27Z

Ok. Testing this.

cmsbuild · 2013-11-26T10:39:12Z

-1
I found an error when building:

>> Compiling edm plugin /build/cmsbuild/jenkins-workarea/workspace/Pull-Request-Integration/ARCHITECTURE/slc5_amd64_gcc481/CMSSW_7_0_X_2013-11-26-0200/src/DQM/Physics/src/plugins.cc 
>> Compiling edm plugin /build/cmsbuild/jenkins-workarea/workspace/Pull-Request-Integration/ARCHITECTURE/slc5_amd64_gcc481/CMSSW_7_0_X_2013-11-26-0200/src/RecoRomanPot/RecoFP420/plugins/ClusterizerFP420.cc 
In file included from /build/cmsbuild/jenkins-workarea/workspace/Pull-Request-Integration/ARCHITECTURE/slc5_amd64_gcc481/CMSSW_7_0_X_2013-11-26-0200/src/DQM/Physics/src/SingleTopTChannelLeptonDQM.h:12:0,
                 from /build/cmsbuild/jenkins-workarea/workspace/Pull-Request-Integration/ARCHITECTURE/slc5_amd64_gcc481/CMSSW_7_0_X_2013-11-26-0200/src/DQM/Physics/src/SingleTopTChannelLeptonDQM.cc:7:
/build/cmsbuild/jenkins-workarea/workspace/Pull-Request-Integration/ARCHITECTURE/slc5_amd64_gcc481/CMSSW_7_0_X_2013-11-26-0200/src/DQM/Physics/interface/TopDQMHelpers.h: In member function 'bool SelectionStep::select(const edm::Event&)':
/build/cmsbuild/jenkins-workarea/workspace/Pull-Request-Integration/ARCHITECTURE/slc5_amd64_gcc481/CMSSW_7_0_X_2013-11-26-0200/src/DQM/Physics/interface/TopDQMHelpers.h:271:19: error: 'class edm::EDGetTokenTedm::ValueMap' has no member named 'isUnitialized'
   if(!electronId_.isUnitialized()){
                   ^
/build/cmsbuild/jenkins-workarea/workspace/Pull-Request-Integration/ARCHITECTURE/slc5_amd64_gcc481/CMSSW_7_0_X_2013-11-26-0200/src/DQM/Physics/interface/TopDQMHelpers.h:281:23: error: 'class edm::EDGetTokenTedm::ValueMap' has no member named 'isUnitialized'
       if( electronId_.isUnitialized() ? true : ((int)(*electronId)[src->refAt(idx)] & eidPattern_) ){  
                       ^
 
you can see the results of the tests here:

https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/1472/summary.html

…GetterOfProductsToGetByToken

Dr15Jones · 2013-11-26T13:49:59Z

Those changes were merged into the official release yesterday which is why I missed them. Sigh. I'm working on getting it fixed.

This code was updated to use getByToken at the same time as the name change was happening.

cmsbuild · 2013-11-26T14:03:01Z

Pull request #1591 was updated. @perrotta, @cmsbuild, @Dr15Jones, @danduggan, @rovere, @Martin-Grunewald, @nclopezo, @deguio, @fwyzard, @eliasron, @ktf can you please check and sign again.

Dr15Jones · 2013-11-26T14:04:11Z

+1
@ktf hopefully that was the last of them

cmsbuild · 2013-11-26T21:35:45Z

-1
When I ran the RelVals I found an error in the following worklfows:
4.22 step2

runTheMatrix-results/4.22_RunCosmics2011A+RunCosmics2011A+RECOCOSD+ALCACOSD+SKIMCOSD+HARVESTDC/step2_RunCosmics2011A+RunCosmics2011A+RECOCOSD+ALCACOSD+SKIMCOSD+HARVESTDC.log
----- Begin Fatal Exception 26-Nov-2013 22:18:21 CET-----------------------
An exception of category 'BranchTypeMismatch' occurred while
   [0] Processing run: 160960 luminosityBlock: 277
   [1] Calling endLuminosityBlock for unscheduled module BPhysicsOniaDQM/'bphysicsOniaDQM'
Exception Message:
A get using a EDGetToken was done in LuminosityBlock but the consumes call was for Event.
 Please modify the consumes call to use the correct branch type.
----- End Fatal Exception -------------------------------------------------

8.0 step3

runTheMatrix-results/8.0_BeamHalo+BeamHalo+DIGICOS+RECOCOS+ALCABH+HARVESTCOS/step3_BeamHalo+BeamHalo+DIGICOS+RECOCOS+ALCABH+HARVESTCOS.log
----- Begin Fatal Exception 26-Nov-2013 22:20:37 CET-----------------------
An exception of category 'BranchTypeMismatch' occurred while
   [0] Processing run: 1 luminosityBlock: 1
   [1] Calling endLuminosityBlock for unscheduled module BPhysicsOniaDQM/'bphysicsOniaDQM'
Exception Message:
A get using a EDGetToken was done in LuminosityBlock but the consumes call was for Event.
 Please modify the consumes call to use the correct branch type.
----- End Fatal Exception -------------------------------------------------

1001.0 step2

runTheMatrix-results/1001.0_RunMinBias2011A+RunMinBias2011A+TIER0EXP+ALCAEXP+ALCAHARVD/step2_RunMinBias2011A+RunMinBias2011A+TIER0EXP+ALCAEXP+ALCAHARVD.log
----- Begin Fatal Exception 26-Nov-2013 22:21:49 CET-----------------------
An exception of category 'BranchTypeMismatch' occurred while
   [0] Processing run: 165121 luminosityBlock: 62
   [1] Calling endLuminosityBlock for unscheduled module BPhysicsOniaDQM/'bphysicsOniaDQM'
Exception Message:
A get using a EDGetToken was done in LuminosityBlock but the consumes call was for Event.
 Please modify the consumes call to use the correct branch type.
----- End Fatal Exception -------------------------------------------------

1306.0 step3

runTheMatrix-results/1306.0_SingleMuPt1_UP15+SingleMuPt1_UP15+DIGIUP15+RECOUP15+HARVESTUP15/step3_SingleMuPt1_UP15+SingleMuPt1_UP15+DIGIUP15+RECOUP15+HARVESTUP15.log
----- Begin Fatal Exception 26-Nov-2013 22:22:02 CET-----------------------
An exception of category 'BranchTypeMismatch' occurred while
   [0] Processing run: 1 luminosityBlock: 1
   [1] Calling endLuminosityBlock for unscheduled module BPhysicsOniaDQM/'bphysicsOniaDQM'
Exception Message:
A get using a EDGetToken was done in LuminosityBlock but the consumes call was for Event.
 Please modify the consumes call to use the correct branch type.
----- End Fatal Exception -------------------------------------------------

1000.0 step2

runTheMatrix-results/1000.0_RunMinBias2011A+RunMinBias2011A+TIER0+SKIMD+HARVESTDfst2+ALCASPLIT/step2_RunMinBias2011A+RunMinBias2011A+TIER0+SKIMD+HARVESTDfst2+ALCASPLIT.log
----- Begin Fatal Exception 26-Nov-2013 22:22:06 CET-----------------------
An exception of category 'BranchTypeMismatch' occurred while
   [0] Processing run: 165121 luminosityBlock: 62
   [1] Calling endLuminosityBlock for unscheduled module BPhysicsOniaDQM/'bphysicsOniaDQM'
Exception Message:
A get using a EDGetToken was done in LuminosityBlock but the consumes call was for Event.
 Please modify the consumes call to use the correct branch type.
----- End Fatal Exception -------------------------------------------------

1003.0 step2

runTheMatrix-results/1003.0_RunMinBias2012A+RunMinBias2012A+RECODDQM+HARVESTDDQM/step2_RunMinBias2012A+RunMinBias2012A+RECODDQM+HARVESTDDQM.log
----- Begin Fatal Exception 26-Nov-2013 22:24:50 CET-----------------------
An exception of category 'BranchTypeMismatch' occurred while
   [0] Processing run: 191226 luminosityBlock: 398
   [1] Calling endLuminosityBlock for unscheduled module BPhysicsOniaDQM/'bphysicsOniaDQM'
Exception Message:
A get using a EDGetToken was done in LuminosityBlock but the consumes call was for Event.
 Please modify the consumes call to use the correct branch type.
----- End Fatal Exception -------------------------------------------------

25.0 step3

runTheMatrix-results/25.0_TTbar+TTbar+DIGI+RECO+HARVEST+ALCATT/step3_TTbar+TTbar+DIGI+RECO+HARVEST+ALCATT.log
----- Begin Fatal Exception 26-Nov-2013 22:30:37 CET-----------------------
An exception of category 'BranchTypeMismatch' occurred while
   [0] Processing run: 1 luminosityBlock: 1
   [1] Calling endLuminosityBlock for unscheduled module BPhysicsOniaDQM/'bphysicsOniaDQM'
Exception Message:
A get using a EDGetToken was done in LuminosityBlock but the consumes call was for Event.
 Please modify the consumes call to use the correct branch type.
----- End Fatal Exception -------------------------------------------------

4.53 step3

runTheMatrix-results/4.53_RunPhoton2012B+RunPhoton2012B+HLTD+RECODreHLT+HARVESTDreHLT/step3_RunPhoton2012B+RunPhoton2012B+HLTD+RECODreHLT+HARVESTDreHLT.log
----- Begin Fatal Exception 26-Nov-2013 22:35:13 CET-----------------------
An exception of category 'BranchTypeMismatch' occurred while
   [0] Processing run: 194533 luminosityBlock: 329
   [1] Calling endLuminosityBlock for unscheduled module BPhysicsOniaDQM/'bphysicsOniaDQM'
Exception Message:
A get using a EDGetToken was done in LuminosityBlock but the consumes call was for Event.
 Please modify the consumes call to use the correct branch type.
----- End Fatal Exception -------------------------------------------------

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

Dr15Jones · 2013-11-26T21:54:57Z

Turns out the new code caught an existing bug. I'll fix the user's code.

… instead of Lumi Changed the consumes call to say it would get LumiSummary from Lumi instead of from the default Event. This bug was only just found because the getByToken was only recently fixed to properly catch this.

cmsbuild · 2013-11-26T22:33:07Z

Pull request #1591 was updated. @perrotta, @cmsbuild, @Dr15Jones, @danduggan, @rovere, @Martin-Grunewald, @nclopezo, @deguio, @fwyzard, @eliasron, @ktf can you please check and sign again.

Dr15Jones · 2013-11-26T22:40:45Z

+1
I fixed the error in the user code

perrotta · 2013-11-27T10:24:40Z

-1
Indeed, all the HLT tests succeed, so for what we are specfically concerned it is fine.

But I still see an error while running step 4 of the matrix test 1001:

$ more step4_RunMinBias2011A+RunMinBias2011A+TIER0EXP+ALCAEXP+ALCAHARVD.log
globaltag = PRE_62_V8::All
271 DQMStore::DQMStore
27-Nov-2013 08:01:18 CET Initiating request to open file file:PromptCalibProd.root
27-Nov-2013 08:01:18 CET Successfully opened file file:PromptCalibProd.root
----- Begin Fatal Exception 27-Nov-2013 08:01:20 CET-----------------------
An exception of category 'BranchTypeMismatch' occurred while
[0] Processing run: 165121 luminosityBlock: 62
[1] Calling endLuminosityBlock for unscheduled module AlcaBeamSpotHarvester/'ALCAHARVESTBeamSpotByRun'
Exception Message:
A get using a EDGetToken was done in LuminosityBlock but the consumes call was for Event.
Please modify the consumes call to use the correct branch type.
----- End Fatal Exception -------------------------------------------------
27-Nov-2013 08:01:20 CET Closed file file:PromptCalibProd.root

MessageLogger Summary

type category sev module subroutine count total

1 Fatal Exception      -s AfterModGlobalEn                       1        1
2 fileAction           -s file_close                             1        1
3 fileAction           -s file_open                              2        2

type category Examples: run/evt run/evt run/evt

1 Fatal Exception      Run: 165121 Lumi                  
2 fileAction           PostEndRun                        
3 fileAction           pre-events       pre-events

Severity # Occurrences Total Occurrences

System 4 4

cmsbuild · 2013-11-27T11:19:30Z

-1
When I ran the RelVals I found an error in the following worklfows:
1001.0 step4

runTheMatrix-results/1001.0_RunMinBias2011A+RunMinBias2011A+TIER0EXP+ALCAEXP+ALCAHARVD/step4_RunMinBias2011A+RunMinBias2011A+TIER0EXP+ALCAEXP+ALCAHARVD.log
----- Begin Fatal Exception 27-Nov-2013 11:57:37 CET-----------------------
An exception of category 'BranchTypeMismatch' occurred while
   [0] Processing run: 165121 luminosityBlock: 62
   [1] Calling endLuminosityBlock for unscheduled module AlcaBeamSpotHarvester/'ALCAHARVESTBeamSpotByRun'
Exception Message:
A get using a EDGetToken was done in LuminosityBlock but the consumes call was for Event.
 Please modify the consumes call to use the correct branch type.
----- End Fatal Exception -------------------------------------------------

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

When calling consumes, one must specify if the data is coming from somewhere other than the Event. The framework check which enforced the match was previously broken but has now been fixed. This change properly registers with consumes the fact that the data is coming from the Lumi.

cmsbuild · 2013-11-27T17:48:02Z

Pull request #1591 was updated. @perrotta, @cmsbuild, @Dr15Jones, @demattia, @danduggan, @rovere, @Martin-Grunewald, @nclopezo, @rcastello, @deguio, @fwyzard, @eliasron, @ktf can you please check and sign again.

Dr15Jones · 2013-11-27T17:48:46Z

+1

Dr15Jones · 2013-11-27T17:51:46Z

I searched our code base for other cases where getByToken was being called on a LuminosityBlock or Run and where the consumes call was done using the default edm::InEvent

https://cmssdt.cern.ch/SDT/lxr/search?v=CMSSW_7_0_X_2013-11-27-0200&filestring=&string=umi.*getByTok

I only found two cases, both of which are fixed in the most recent commit.

Unfortunately, I've been unable to build a 'release' in a timely manner becuase of the many hundreds of packages which need to be built. This means I've not been able to do any runTheMatrix tests.

Dr15Jones · 2013-11-27T19:38:51Z

I still haven't been able to form a completely self-consistent build, however the runTheMatrix jobs I have been able to run (1001.0 4.22 1003.0) now succeed.

cmsbuild · 2013-11-28T10:55:59Z

+1
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-1591/1514/summary.html

perrotta · 2013-11-28T13:04:19Z

+1

deguio · 2013-11-28T13:23:13Z

+1

Dr15Jones · 2013-11-29T15:45:47Z

@demattia @rcastello @ktf Alca is the last holdout for signing this. It's a real shame it missed pre9.

ktf · 2013-11-29T15:50:56Z

It also does not merge anymore...

Dr15Jones · 2013-11-29T16:10:04Z

Sigh. I feared that might be the case given the recent HLT changes.

…GetterOfProductsToGetByToken. In the case of conflicts, took changes from official-cmssw/CMSSW_7_0_X Conflicts: HLTrigger/HLTanalyzers/src/EventHeader.cc HLTrigger/HLTanalyzers/src/HLTAnalyzer.cc HLTrigger/HLTanalyzers/src/HLTBitAnalyzer.cc HLTrigger/HLTanalyzers/src/HLTHeavyIon.cc

cmsbuild · 2013-11-29T19:08:03Z

Pull request #1591 was updated. @perrotta, @cmsbuild, @Dr15Jones, @demattia, @danduggan, @rovere, @Martin-Grunewald, @nclopezo, @rcastello, @deguio, @fwyzard, @eliasron, @ktf can you please check and sign again.

Dr15Jones · 2013-11-29T19:38:55Z

+1
The only change was to resolved conflicts. All conflicts were resolved by using the code which was already in CMSSW_7_0_X.

perrotta · 2013-11-29T21:57:17Z

+1
No changes in HLTrigger code wrt 7_0_X besides the renaming of the isUninitialized() method.

cmsbuild · 2013-12-02T11:22:08Z

+1
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-1591/1538/summary.html

Dr15Jones · 2013-12-02T13:05:08Z

@smuzaffar @davidlt @ktf Given that the only change this time was to deal with a merge conflict with the HLT and the HLT has signed off on this change, could you please bypass the rest of the (now redundant) signatures and finally get this in the release?

…Token Switch GetterOfProducts to use Event::getByToken

…n-v4.0.4 xrootd: fix version ("unknown" -> "v4.0.4")

Dr15Jones added 4 commits November 25, 2013 16:47

bug fix: getByToken only worked for Event

dec22bb

The test which makes sure the getByToken for a particular principal matches the type used in the ‘consumes’ was hard coded to only match Event. This change uses the proper type when doing the comparison.

EDGetToken::isUninitialized was misspelled

f1d9707

Fixed typo in the function name EDGetToken*::isUninitialized.

Further changes from EDGetToken::isUninitialized was misspelled

cf7bfce

This fixes the non-framework uses of the misspelled function name.

Merge remote-tracking branch 'official-cmssw/CMSSW_7_0_X' into switch…

67762b3

…GetterOfProductsToGetByToken

Further changes from EDGetToken::isUninitialized was misspelled

2b9063f

This code was updated to use getByToken at the same time as the name change was happening.

Dr15Jones mentioned this pull request Nov 27, 2013

get rid of last remaining getByLabel calls in HLTrigger/HLTanalyzers #1611

Merged

nclopezo added a commit that referenced this pull request Dec 2, 2013

Merge pull request #1591 from Dr15Jones/switchGetterOfProductsToGetBy…

99570ec

…Token Switch GetterOfProducts to use Event::getByToken

nclopezo merged commit 99570ec into cms-sw:CMSSW_7_0_X Dec 2, 2013

Dr15Jones deleted the switchGetterOfProductsToGetByToken branch December 12, 2013 20:48

ggovi pushed a commit to ggovi/cmssw that referenced this pull request Jan 11, 2017

Merge pull request cms-sw#1591 from davidlt/xrootd-fix-version-unknow…

5aa398e

…n-v4.0.4 xrootd: fix version ("unknown" -> "v4.0.4")

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch GetterOfProducts to use Event::getByToken #1591

Switch GetterOfProducts to use Event::getByToken #1591

Dr15Jones commented Nov 25, 2013

cmsbuild commented Nov 25, 2013

Dr15Jones commented Nov 25, 2013

ktf commented Nov 26, 2013

cmsbuild commented Nov 26, 2013

Dr15Jones commented Nov 26, 2013

cmsbuild commented Nov 26, 2013

Dr15Jones commented Nov 26, 2013

cmsbuild commented Nov 26, 2013

Dr15Jones commented Nov 26, 2013

cmsbuild commented Nov 26, 2013

Dr15Jones commented Nov 26, 2013

perrotta commented Nov 27, 2013

cmsbuild commented Nov 27, 2013

cmsbuild commented Nov 27, 2013

Dr15Jones commented Nov 27, 2013

Dr15Jones commented Nov 27, 2013

Dr15Jones commented Nov 27, 2013

cmsbuild commented Nov 28, 2013

perrotta commented Nov 28, 2013

deguio commented Nov 28, 2013

Dr15Jones commented Nov 29, 2013

ktf commented Nov 29, 2013

Dr15Jones commented Nov 29, 2013

cmsbuild commented Nov 29, 2013

Dr15Jones commented Nov 29, 2013

perrotta commented Nov 29, 2013

cmsbuild commented Dec 2, 2013

Dr15Jones commented Dec 2, 2013

Switch GetterOfProducts to use Event::getByToken #1591

Switch GetterOfProducts to use Event::getByToken #1591

Conversation

Dr15Jones commented Nov 25, 2013

cmsbuild commented Nov 25, 2013

Dr15Jones commented Nov 25, 2013

ktf commented Nov 26, 2013

cmsbuild commented Nov 26, 2013

Dr15Jones commented Nov 26, 2013

cmsbuild commented Nov 26, 2013

Dr15Jones commented Nov 26, 2013

cmsbuild commented Nov 26, 2013

Dr15Jones commented Nov 26, 2013

cmsbuild commented Nov 26, 2013

Dr15Jones commented Nov 26, 2013

perrotta commented Nov 27, 2013

cmsbuild commented Nov 27, 2013

cmsbuild commented Nov 27, 2013

Dr15Jones commented Nov 27, 2013

Dr15Jones commented Nov 27, 2013

Dr15Jones commented Nov 27, 2013

cmsbuild commented Nov 28, 2013

perrotta commented Nov 28, 2013

deguio commented Nov 28, 2013

Dr15Jones commented Nov 29, 2013

ktf commented Nov 29, 2013

Dr15Jones commented Nov 29, 2013

cmsbuild commented Nov 29, 2013

Dr15Jones commented Nov 29, 2013

perrotta commented Nov 29, 2013

cmsbuild commented Dec 2, 2013

Dr15Jones commented Dec 2, 2013