Faster setup of EventPrincipal #28505

Dr15Jones · 2019-11-28T21:06:47Z

PR description:

Several optimizations to make the call to fillEventPrincipal more efficient.

PR validation:

Framework unit tests pass.

The variable being moved was already being passed as a R value reference.

The map index was already a dense packed integral value, BranchListIndex.

The ProcessHistory is shared by all data products in the Event, therefore not needed by each data product. This avoids the need to set the history for each data product each time we have a new event.

The code is actually dependent on ProcessHistory and not on ProcessHistoryRegistry (which is just one particular way of getting the ProcessHistory).

The ProductProvenanceRetriever is created in the EventPrincipal's constructor and never changes. Therefore it is possible to set the value in the ProductResolvers at construction time as well.

If BranchListIndex does not change we do not update structures.

cmsbuild · 2019-11-28T21:07:21Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-11-28T21:13:44Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28505/12956

This PR adds an extra 248KB to repository

cmsbuild · 2019-11-28T21:15:49Z

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

It involves the following packages:

DataFormats/Common
DataFormats/FWLite
DataFormats/Provenance
FWCore/Common
FWCore/Framework
FWCore/Integration
FWCore/Modules
FWCore/Sources
FWCore/TFWLiteSelector
FWCore/TestProcessor
IOPool/Input
IOPool/Streamer

@cmsbuild, @smuzaffar, @Dr15Jones can you please review it and eventually sign? Thanks.
@makortel, @rovere, @wddgit this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

Dr15Jones · 2019-11-28T21:42:14Z

please test

cmsbuild · 2019-11-28T21:42:46Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/3688/console Started: 2019/11/28 22:43

cmsbuild · 2019-11-29T00:03:27Z

-1

Tested at: 76e4096

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

I found follow errors while testing this PR

Failed tests: Build

Build:

I found compilation error when building:

>> Entering Package DQMServices/FwkIO
Entering library rule at src/DQMServices/FwkIO/plugins
>> Compiling edm plugin /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_0_X_2019-11-28-1100/src/DQMServices/FwkIO/plugins/DQMRootOutputModule.cc 
>> Compiling edm plugin /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_0_X_2019-11-28-1100/src/DQMServices/FwkIO/plugins/DQMRootSource.cc 
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_0_X_2019-11-28-1100/src/DQMServices/FwkIO/plugins/DQMRootSource.cc: In member function 'virtual void DQMRootSource::readLuminosityBlock_(edm::LuminosityBlockPrincipal&)':
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_0_X_2019-11-28-1100/src/DQMServices/FwkIO/plugins/DQMRootSource.cc:615:73: error: no matching function for call to 'edm::LuminosityBlockPrincipal::fillLuminosityBlockPrincipal(edm::ProcessHistoryRegistry&)'
   lbCache.fillLuminosityBlockPrincipal(processHistoryRegistryForUpdate());
                                                                         ^
In file included from /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_0_X_2019-11-28-1100/src/DQMServices/FwkIO/plugins/DQMRootSource.cc:39:
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_0_X_2019-11-28-1100/src/FWCore/Framework/interface/LuminosityBlockPrincipal.h:43:10: note: candidate: 'void edm::LuminosityBlockPrincipal::fillLuminosityBlockPrincipal(const edm::ProcessHistory*, edm::DelayedReader*)'
     void fillLuminosityBlockPrincipal(ProcessHistory const* processHistory, DelayedReader* reader = nullptr);

cmsbuild · 2019-11-29T00:03:29Z

Comparison not run due to Build errors (RelVals and Igprof tests were also skipped)

Call new LuminosityBlockPrincipal::fillLuminosityBlock interface.

Dr15Jones · 2019-12-04T00:56:03Z

+1

cmsbuild · 2019-12-04T02:25:13Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 34
DQMHistoTests: Total histograms compared: 2793840
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2793498
DQMHistoTests: Total skipped: 341
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
Checked 147 log files, 16 edm output root files, 34 DQM output files

jfernan2 · 2019-12-04T07:41:45Z

+1

civanch · 2019-12-04T11:02:41Z

+1

perrotta · 2019-12-04T16:09:43Z

+1

Technical, jenkins tests pass

santocch · 2019-12-05T08:12:37Z

+1

fabiocos · 2019-12-05T17:38:43Z

@qliphy @agrohsje the change in the generator area is quite limited and technical, please have a look, I will integrate this PR in next IB

fabiocos · 2019-12-05T17:46:19Z

there is no real conflict with #28551

fabiocos · 2019-12-05T21:12:17Z

+1

fabiocos · 2019-12-05T21:12:28Z

merge

Dr15Jones added 7 commits November 25, 2019 12:47

Use std::move in EventPrincipal::fillEventPrincipal

5b75380

The variable being moved was already being passed as a R value reference.

Replace std::map with std::vector in EventPrincipal

6132100

The map index was already a dense packed integral value, BranchListIndex.

code format

ff4d56a

Removed ProcessHistory from StableProvenance

a50129d

The ProcessHistory is shared by all data products in the Event, therefore not needed by each data product. This avoids the need to set the history for each data product each time we have a new event.

Reduced dependency on ProcessHistoryRegistry

def96ce

The code is actually dependent on ProcessHistory and not on ProcessHistoryRegistry (which is just one particular way of getting the ProcessHistory).

Set ProductProvenanceRetriever only once

9976816

The ProductProvenanceRetriever is created in the EventPrincipal's constructor and never changes. Therefore it is possible to set the value in the ProductResolvers at construction time as well.

Optimize filling of EventPrincipal

76e4096

If BranchListIndex does not change we do not update structures.

cmsbuild added this to the CMSSW_11_0_X milestone Nov 28, 2019

cmsbuild added code-checks-pending comparison-pending core-pending orp-pending pending-signatures tests-pending labels Nov 28, 2019

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 28, 2019

cmsbuild added tests-started and removed tests-pending labels Nov 28, 2019

cmsbuild added comparison-notrun tests-rejected and removed comparison-pending tests-started labels Nov 29, 2019

Have DQMRootSource use new API

c1b42b3

Call new LuminosityBlockPrincipal::fillLuminosityBlock interface.

cmsbuild added core-approved and removed core-pending labels Dec 4, 2019

cmsbuild added comparison-available and removed comparison-pending labels Dec 4, 2019

cmsbuild added dqm-approved and removed dqm-pending labels Dec 4, 2019

cmsbuild added simulation-approved and removed simulation-pending labels Dec 4, 2019

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Dec 4, 2019

This was referenced Dec 4, 2019

Use TFile::GetUUID() for job report GUID in DQMRootOutputModule #28551

Merged

PFlow fix/improvement in the HF region for phase2 #28442

Merged

cmsbuild added analysis-approved and removed analysis-pending labels Dec 5, 2019

cmsbuild mentioned this pull request Dec 5, 2019

Add enforceGUIDInFileName option to PoolSource and EmbeddedRootSource #28561

Merged

cmsbuild added orp-approved and removed orp-pending labels Dec 5, 2019

cmsbuild merged commit d4151f4 into cms-sw:master Dec 5, 2019

Dr15Jones deleted the fasterEventPrincipal branch December 5, 2019 21:48

guitargeek mentioned this pull request Dec 8, 2019

Phase out old pre-GED GsfElectron sequence #28429

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Faster setup of EventPrincipal #28505

Faster setup of EventPrincipal #28505

Dr15Jones commented Nov 28, 2019

cmsbuild commented Nov 28, 2019

cmsbuild commented Nov 28, 2019

cmsbuild commented Nov 28, 2019

Dr15Jones commented Nov 28, 2019

cmsbuild commented Nov 28, 2019 •

edited

Loading

cmsbuild commented Nov 29, 2019

cmsbuild commented Nov 29, 2019

Dr15Jones commented Dec 4, 2019

cmsbuild commented Dec 4, 2019

jfernan2 commented Dec 4, 2019

civanch commented Dec 4, 2019

perrotta commented Dec 4, 2019

santocch commented Dec 5, 2019

fabiocos commented Dec 5, 2019

fabiocos commented Dec 5, 2019

fabiocos commented Dec 5, 2019

fabiocos commented Dec 5, 2019

Faster setup of EventPrincipal #28505

Faster setup of EventPrincipal #28505

Conversation

Dr15Jones commented Nov 28, 2019

PR description:

PR validation:

cmsbuild commented Nov 28, 2019

cmsbuild commented Nov 28, 2019

cmsbuild commented Nov 28, 2019

Dr15Jones commented Nov 28, 2019

cmsbuild commented Nov 28, 2019 • edited Loading

cmsbuild commented Nov 29, 2019

cmsbuild commented Nov 29, 2019

Dr15Jones commented Dec 4, 2019

cmsbuild commented Dec 4, 2019

jfernan2 commented Dec 4, 2019

civanch commented Dec 4, 2019

perrotta commented Dec 4, 2019

santocch commented Dec 5, 2019

fabiocos commented Dec 5, 2019

fabiocos commented Dec 5, 2019

fabiocos commented Dec 5, 2019

fabiocos commented Dec 5, 2019

cmsbuild commented Nov 28, 2019 •

edited

Loading