mitigate high CPU cost of HBHE hit position calculation in EgammaHcalIsolation #35955

slava77 · 2021-11-02T14:57:54Z

costs of computing HCAL isolation in egamma has increased significantly after #33666

12_0_0_pre3 has ~0.14% of reco time in isolation
12_0_0_pre4 has 8.9%
- 12_1_0_pre4 has a similar fraction

This PR mitigates the situation by reordering the calls.

However, a better refactoring of the code should be considered in near future. Some options are

accumulate all needed isolation types in one loop over rechits
precompute (η,φ) of rechits in a separate producer in a ValueMap and use it when dR computations are needed
@wrtabb @SohamBhattacharya

slava77 · 2021-11-02T15:03:28Z

enable profiling

cmsbuild · 2021-11-02T15:04:16Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35955/26365

This PR adds an extra 12KB to repository

slava77 · 2021-11-02T15:04:32Z

@cmsbuild please test

cmsbuild · 2021-11-02T15:04:39Z

A new Pull Request was created by @slava77 (Slava Krutelyov) for master.

It involves the following packages:

RecoEgamma/EgammaIsolationAlgos (reconstruction)

@jpata, @slava77 can you please review it and eventually sign? Thanks.
@Sam-Harper, @jainshilpi, @lgray, @sobhatta, @afiqaize, @wrtabb, @varuns23, @ram1123 this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

cmsbuild · 2021-11-02T21:47:55Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0b0595/20187/summary.html
COMMIT: b00003d
CMSSW: CMSSW_12_2_X_2021-11-02-1100/slc7_amd64_gcc900
Additional Tests: PROFILING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35955/20187/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: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 42
DQMHistoTests: Total histograms compared: 2901890
DQMHistoTests: Total failures: 0
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2901868
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 41 files compared)
Checked 177 log files, 37 edm output root files, 42 DQM output files
TriggerResults: no differences found

slava77 · 2021-11-02T23:18:34Z

in IB igprof tests the perf counts in EgammaHcalIsolation::getHcalSum have reduced by about a factor of 7

7.46% or 31.34 perf counts in the earlier baseline
1.05% or 4.02 perf counts this PR tests

This is roughly expected from the early return after the depth check (7 depths are checked).

cmsbuild · 2021-11-03T00:08:47Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35955/26386

This PR adds an extra 16KB to repository

slava77 · 2021-11-03T00:09:05Z

@cmsbuild please test

cmsbuild · 2021-11-03T00:09:16Z

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

swagata87 · 2021-11-03T05:13:24Z

thank you Slava,
this will be very useful for EGM HLT also

cmsbuild · 2021-11-03T06:43:48Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0b0595/20209/summary.html
COMMIT: f33bdaa
CMSSW: CMSSW_12_2_X_2021-11-02-1100/slc7_amd64_gcc900
Additional Tests: PROFILING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35955/20209/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: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 42
DQMHistoTests: Total histograms compared: 2901890
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2901862
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 41 files compared)
Checked 177 log files, 37 edm output root files, 42 DQM output files
TriggerResults: no differences found

slava77 · 2021-11-03T12:29:48Z

thank you Slava, this will be very useful for EGM HLT also

does this imply a backport?
If so, to which release?

slava77 · 2021-11-03T14:27:24Z

in IB igprof tests the perf counts in EgammaHcalIsolation::getHcalSum have reduced by about a factor of 7

7.46% or 31.34 perf counts in the earlier baseline

1.05% or 4.02 perf counts this PR tests

This is roughly expected from the early return after the depth check (7 depths are checked).

the last update now has

0.91% or 3.53 perf counts last profiling as of
f33bdaa

So, slightly more than a factor of 8 speedup.

I think that further speedup is possible by refactoring the calling code. I added #35969 to track further progress with this still somewhat heavy code.

slava77 · 2021-11-03T14:32:18Z

+reconstruction

for #35955 f33bdaa

code changes are technical, as confirmed by the PR comparisons/tests

cmsbuild · 2021-11-03T14:32:42Z

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. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

perrotta · 2021-11-03T15:23:33Z

+1

swagata87 · 2021-11-03T15:24:43Z

does this imply a backport? If so, to which release?

no, I don't think a backport is needed

VinInn · 2021-11-12T14:24:00Z

RecoEgamma/EgammaIsolationAlgos/src/EgammaHcalIsolation.cc

+    return 0.;
+
+  const auto phit = caloGeometry_.getPosition(hit.detid());
+  const float phitEta = phit.eta();


use cashed eta in caloGeometry_

please clarify the member method
Geometry/CaloGeometry/interface/CaloGeometry.h has only xyz position

RhoEtaPhi const& repPos() const { return m_rep; }
virtual float rhoPos() const { return m_rep.rho(); }
virtual float etaPos() const { return m_rep.eta(); }
virtual float phiPos() const { return m_rep.phi(); }

eventually through CaloGeometry::getGeometry if not in the same SubDetector

one could extend the CaloGeometry interface to support repPos (copy paste code of getPosition)
In my opinion would be more efficient to check first if the CellGeometry is the same and avoid the overhead of the code (not inlined) to search for the subDetector for each single hit....

ah, so the suggestion is for an extension to the geometry interface; I thought at first that this is something that exists already.

IIRC, there is one rechit per cell in ECAL/HCAL

it exits already in CaloCellGeometry that is what CaloGeometry access. please have a look to the implementation in
https://cmssdt.cern.ch/dxr/CMSSW/source/Geometry/CaloGeometry/src/CaloGeometry.cc#50
here locally one can invoke
caloGeometry_.getGeometry(hit.detid())->repPos();

here https://github.com/slava77/cmssw/blob/f33bdaa8be9c4aa4a3f336dbf23501b059a8988b/RecoEgamma/EgammaIsolationAlgos/src/EgammaHcalIsolation.cc#L188
thre is a loop on the hits in HBorHE. one can verify if a hit is in the same SubDet of the previous one and avoid all the search logic in CaloGeometry and acess directly CaloCellGeometry

VinInn · 2021-11-12T14:24:20Z

RecoEgamma/EgammaIsolationAlgos/src/EgammaHcalIsolation.cc

+  const float phitEta = phit.eta();
+
+  if (extIncRule_ == InclusionRule::withinConeAroundCluster or intIncRule_ == InclusionRule::withinConeAroundCluster) {
+    auto const dR2 = deltaR2(pcluEta, pcluPhi, phitEta, phit.phi());


use cashed phi in caloGeometry

VinInn · 2021-11-12T18:09:52Z

RecoEgamma/EgammaIsolationAlgos/src/EgammaHcalIsolation.cc

+  if (!(goodHBe or goodHEe))
+    return 0.;
+
+  const auto phit = caloGeometry_.getPosition(hit.detid());


suggested change:

auto const & phit = caloGeometry_.getGeometry(hit.detid())->repPos();

suggested change:

Thanks. Testing.
PR soon.

mitigate high CPU cost of HBHE hit position calculation

b00003d

cmsbuild added this to the CMSSW_12_2_X milestone Nov 2, 2021

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Nov 2, 2021

slava77 changed the title ~~mitigate high CPU cost of HBHE hit position calculation~~ mitigate high CPU cost of HBHE hit position calculation in EgammaHcalIsolation Nov 2, 2021

cmsbuild added code-checks-approved tests-started and removed tests-pending code-checks-pending labels Nov 2, 2021

cmsbuild added tests-approved and removed tests-started labels Nov 2, 2021

refactor goodHitEnergy to keep minimize eta/phi (re)computations

f33bdaa

cmsbuild added code-checks-pending tests-pending and removed tests-approved code-checks-approved labels Nov 3, 2021

cmsbuild added code-checks-approved and removed tests-pending code-checks-pending labels Nov 3, 2021

cmsbuild added the tests-started label Nov 3, 2021

cmsbuild added tests-approved and removed tests-started labels Nov 3, 2021

slava77 mentioned this pull request Nov 3, 2021

need to speedup/refactor EgammaHcalIsolation #35969

Open

cmsbuild added fully-signed reconstruction-approved and removed reconstruction-pending pending-signatures labels Nov 3, 2021

cmsbuild added orp-approved and removed orp-pending labels Nov 3, 2021

cmsbuild merged commit 69ae98c into cms-sw:master Nov 3, 2021

VinInn reviewed Nov 12, 2021

View reviewed changes

slava77 mentioned this pull request Nov 12, 2021

speedup: use cached RhoEtaPhi from caloCellGeometry in EgammaHcalIsolation #36108

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mitigate high CPU cost of HBHE hit position calculation in EgammaHcalIsolation #35955

mitigate high CPU cost of HBHE hit position calculation in EgammaHcalIsolation #35955

slava77 commented Nov 2, 2021

slava77 commented Nov 2, 2021

cmsbuild commented Nov 2, 2021

slava77 commented Nov 2, 2021

cmsbuild commented Nov 2, 2021

cmsbuild commented Nov 2, 2021

slava77 commented Nov 2, 2021

cmsbuild commented Nov 3, 2021

slava77 commented Nov 3, 2021

cmsbuild commented Nov 3, 2021

swagata87 commented Nov 3, 2021

cmsbuild commented Nov 3, 2021

slava77 commented Nov 3, 2021

slava77 commented Nov 3, 2021

slava77 commented Nov 3, 2021

cmsbuild commented Nov 3, 2021

perrotta commented Nov 3, 2021

swagata87 commented Nov 3, 2021

VinInn Nov 12, 2021

slava77 Nov 12, 2021

VinInn Nov 12, 2021

VinInn Nov 12, 2021

VinInn Nov 12, 2021

slava77 Nov 12, 2021

VinInn Nov 12, 2021 •

edited

VinInn Nov 12, 2021 •

edited

VinInn Nov 12, 2021

VinInn Nov 12, 2021 •

edited

slava77 Nov 12, 2021

mitigate high CPU cost of HBHE hit position calculation in EgammaHcalIsolation #35955

mitigate high CPU cost of HBHE hit position calculation in EgammaHcalIsolation #35955

Conversation

slava77 commented Nov 2, 2021

slava77 commented Nov 2, 2021

cmsbuild commented Nov 2, 2021

slava77 commented Nov 2, 2021

cmsbuild commented Nov 2, 2021

cmsbuild commented Nov 2, 2021

Comparison Summary

slava77 commented Nov 2, 2021

cmsbuild commented Nov 3, 2021

slava77 commented Nov 3, 2021

cmsbuild commented Nov 3, 2021

swagata87 commented Nov 3, 2021

cmsbuild commented Nov 3, 2021

Comparison Summary

slava77 commented Nov 3, 2021

slava77 commented Nov 3, 2021

slava77 commented Nov 3, 2021

cmsbuild commented Nov 3, 2021

perrotta commented Nov 3, 2021

swagata87 commented Nov 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VinInn Nov 12, 2021 • edited

Choose a reason for hiding this comment

VinInn Nov 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VinInn Nov 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VinInn Nov 12, 2021 •

edited

VinInn Nov 12, 2021 •

edited

VinInn Nov 12, 2021 •

edited