Devirtualize PFBlockElement daughter classes

[marksan87]

PR description:

This PR addresses the performance issues discussed in issue #33083. The PFBlockElementTrack, PFBlockElementBrem, PFBlockElementCluster, PFBlockElementSuperCluster, and PFBlockElementGsfTrack daughter classes of PFBlockElement are devirtualized. Bitmasks are now used for PFBlockElementGsfTrack::isSecondary and in place of the logical operators used for PFBlockElementTrack::isLinkedToDisplacedVertex.

PR validation:

The performance was profiled with igprof using step 3 of matrix test 23434.21:

Base: https://msaunder.web.cern.ch/msaunder/cgi-bin/igprof-navigator/gitIssues/33083/step3_23434_21_base/1728
Final: https://msaunder.web.cern.ch/msaunder/cgi-bin/igprof-navigator/gitIssues/33083/step3_23434_21_final/3332

This is not a backport.

@hatakeyamak @bendavid @rappoccio @laurenhay

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33127/21481

• This PR adds an extra 20KB to repository

[cmsbuild]

A new Pull Request was created by @zeratul87 for master.

It involves the following packages:

DataFormats/ParticleFlowReco

@perrotta, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@cbernet, @lgray, @rovere, @hatakeyamak this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33127/21483

• This PR adds an extra 20KB to repository

[cmsbuild]

Pull request #33127 was updated. @perrotta, @jpata, @cmsbuild, @slava77 can you please check and sign again.

[slava77]

@cmsbuild please test

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ab0946/13394/summary.html
COMMIT: e2e2733
CMSSW: CMSSW_11_3_X_2021-03-09-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/33127/13394/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

• No significant changes to the logs found
• Reco comparison results: 3 differences found in the comparisons
• DQMHistoTests: Total files compared: 38
• DQMHistoTests: Total histograms compared: 2849195
• DQMHistoTests: Total failures: 8
• DQMHistoTests: Total nulls: 1
• DQMHistoTests: Total successes: 2849164
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.004 KiB( 37 files compared)
• DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
• Checked 160 log files, 37 edm output root files, 38 DQM output files

[VinInn]

Excellent. Would it make sense to have a look to all other daughters of "PFBlockElement"?

[slava77]

On 3/10/21 1:46 AM, Vincenzo Innocente wrote: Excellent. Would it make sense to have a look to all other daughters of "PFBlockElement"? —

I agree, it makes sense to check there as well

[slava77]

On 3/10/21 1:46 AM, Vincenzo Innocente wrote: Excellent. Would it make sense to have a look to all other daughters of "PFBlockElement"? —
I agree, it makes sense to check there as well

OTOH, nothing else similarly relevant shows up in the profiler report https://jpata.web.cern.ch/jpata/cgi-bin/igprof-navigator/releases/11_3_0_pre4/23434.21/step3/cpu/252

Perhaps a trivial finalization can still be done.

[slava77]

+1

for #33127 e2e2733

• code changes are in line with the PR description and the follow up review
• jenkins tests pass and comparisons with the baseline show no (relevant differences)
• as provided in the PR description and the followup checks in wf 23234.21 the cost of calling reco::PFBlockElementTrack::isLinkedToDisplacedVertex() is reduced by about a factor of 6 (from 1.69 to 0.29 ticks), which is about 0.12% of the total processing time.
• finalization of other classes deriving from PFBlockElement can be done in a follow-up PR (although it's welcome here as well)

@zeratul87 please update the PR description to include the latest profiler result corresponding to the current status of this PR

[marksan87]

I have added devirtualization for the other PFBlockElement daughter classes here since it is a minor change.

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33127/21505

• This PR adds an extra 20KB to repository

[cmsbuild]

Pull request #33127 was updated. @perrotta, @jpata, @cmsbuild, @slava77 can you please check and sign again.

[slava77]

@cmsbuild please test

@zeratul87 thank you for the quick update

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ab0946/13429/summary.html
COMMIT: 191b2d1
CMSSW: CMSSW_11_3_X_2021-03-09-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/33127/13429/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

• No significant changes to the logs found
• Reco comparison results: 3 differences found in the comparisons
• DQMHistoTests: Total files compared: 38
• DQMHistoTests: Total histograms compared: 2849195
• DQMHistoTests: Total failures: 6
• DQMHistoTests: Total nulls: 1
• DQMHistoTests: Total successes: 2849166
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.004 KiB( 37 files compared)
• DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
• Checked 160 log files, 37 edm output root files, 38 DQM output files

[slava77]

+1

for #33127 191b2d1

• other classes inheriting from PFBlockElement are now final
• this is a technical update since the last reco signoff in Devirtualize PFBlockElement daughter classes #33127 (comment) (no CPU performance changes are expected either)

[cmsbuild]

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

[qliphy]

+1