Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PFRecHit navigator for HCAL with dense index #29049

Merged

Conversation

hatakeyamak
Copy link
Contributor

@hatakeyamak hatakeyamak commented Feb 26, 2020

PR description:

This is an update for PFRecHit navigator for HCAL using a vector of neighbor information filled with HCAL dense index. This is expected to cut down the PFRecHitProducer for HCAL by 30-40%.

Using HLT's OnLine_HLT_GRun.py on:
/eos/cms/store/data/Run2018D/EphemeralHLTPhysics5/RAW/v1/000/324/897/00000/F5F13777-D664-C747-88AA-0459C1E35254.root
(1000 events)

[Before]
FastReport   CPU time avg.      when run  Real time avg.      when run     Alloc. avg.      when run   Dealloc. avg.      when run  Modules
FastReport         4.0 ms         4.0 ms         4.1 ms         4.1 ms        +607 kB        +607 kB        -512 kB        -512 kB    hltParticleFlowRecHitHBHE
FastReport         3.9 ms         3.9 ms         4.0 ms         4.0 ms       +1154 kB       +1154 kB        -907 kB        -907 kB    hltParticleFlowRecHitHF

[With this PR]
FastReport         2.3 ms         2.3 ms         2.4 ms         2.4 ms        +608 kB        +608 kB        -513 kB        -513 kB    hltParticleFlowRecHitHBHE
FastReport         2.7 ms         2.7 ms         2.9 ms         2.9 ms       +1148 kB       +1148 kB        -903 kB        -903 kB    hltParticleFlowRecHitHF

Offline setup

[Before]
Run3_0PU
FastReport   CPU time avg.      when run  Real time avg.      when run     Alloc. avg.      when run   Dealloc. avg.      when run  Modules
FastReport         1.3 ms         1.3 ms         1.5 ms         1.5 ms        +130 kB        +130 kB         -57 kB         -57 kB    particleFlowRecHitHBHE
FastReport         1.6 ms         1.6 ms         2.3 ms         2.3 ms        +266 kB        +266 kB        -233 kB        -233 kB    particleFlowRecHitHF
FastReport         0.1 ms         0.1 ms         0.1 ms         0.1 ms         +60 kB         +60 kB         -59 kB         -59 kB    particleFlowRecHitHO

Run3 PU
FastReport        16.1 ms        16.1 ms        18.0 ms        18.0 ms       +1558 kB       +1558 kB        -604 kB        -604 kB    particleFlowRecHitHBHE
FastReport         4.6 ms         4.6 ms         4.8 ms         4.8 ms       +1547 kB       +1547 kB       -1298 kB       -1298 kB    particleFlowRecHitHF
FastReport         0.1 ms         0.1 ms         0.1 ms         0.1 ms        +662 kB        +662 kB        -660 kB        -660 kB    particleFlowRecHitHO

Phase2 PU
FastReport        36.7 ms        36.7 ms        36.9 ms        36.9 ms      +38833 kB      +38833 kB      -37100 kB      -37100 kB    particleFlowRecHitHBHE
FastReport         7.0 ms         7.0 ms         7.0 ms         7.0 ms      +29003 kB      +29003 kB      -28734 kB      -28734 kB    particleFlowRecHitHF
FastReport         0.9 ms         0.9 ms         0.9 ms         0.9 ms      +36056 kB      +36056 kB      -36052 kB      -36052 kB    particleFlowRecHitHO

[With this PR]
Run3_0PU
FastReport         1.0 ms         1.0 ms         2.5 ms         2.5 ms        +128 kB        +128 kB         -52 kB         -52 kB    particleFlowRecHitHBHE
FastReport         1.8 ms         1.8 ms         2.6 ms         2.6 ms        +260 kB        +260 kB        -225 kB        -225 kB    particleFlowRecHitHF
FastReport         0.1 ms         0.1 ms         0.4 ms         0.4 ms         +57 kB         +57 kB         -56 kB         -56 kB    particleFlowRecHitHO

Run3 PU
FastReport         8.0 ms         8.0 ms         9.2 ms         9.2 ms       +1512 kB       +1512 kB        -553 kB        -553 kB    particleFlowRecHitHBHE
FastReport         4.2 ms         4.2 ms         4.4 ms         4.4 ms       +1561 kB       +1561 kB       -1313 kB       -1313 kB    particleFlowRecHitHF
FastReport         0.2 ms         0.2 ms         0.2 ms         0.2 ms        +683 kB        +683 kB        -682 kB        -682 kB    particleFlowRecHitHO

Phase2 PU
FastReport        19.3 ms        19.3 ms        21.1 ms        21.1 ms      +28332 kB      +28332 kB      -26446 kB      -26446 kB    particleFlowRecHitHBHE
FastReport         4.4 ms         4.4 ms         4.8 ms         4.8 ms      +23704 kB      +23704 kB      -23432 kB      -23432 kB    particleFlowRecHitHF
FastReport         0.3 ms         0.3 ms         0.3 ms         0.3 ms      +25473 kB      +25473 kB      -25469 kB      -25469 kB    particleFlowRecHitHO

Also, we used this opportunity to clean up the unused module (PFCTRecHitProducer), clean up unused config parameters, and some of the initialization related to topology was moved from event-level to beginLuminosityBlock for efficiency.

PR validation:

Run validation based on a few matrix tests (ttbar for 2018 and 2021) and HLT test based on 2018 data (originally from):
https://fwyzard.web.cern.ch/fwyzard/patatrack/hlt_paths/original.py
and checked that PF candidate outputs are identical.

if this PR is a backport please specify the original PR and why you need to backport that PR:

This is not backport.

@bendavid @jsalfeld @mariadalfonso @bsunanda @abdoulline

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29049/13915

  • This PR adds an extra 88KB to repository

  • Found files with invalid states:

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@hatakeyamak
Copy link
Contributor Author

Thank you @perrotta . I think I incorporated your suggestions. Some further possible cleanup of time-related parameters may be deferred to another PR, as you say.

@perrotta
Copy link
Contributor

perrotta commented Mar 4, 2020

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 4, 2020

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/4992/console Started: 2020/03/04 17:10

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 4, 2020

+1
Tested at: 29da198
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8a0812/4992/summary.html
CMSSW: CMSSW_11_1_X_2020-03-04-1100
SCRAM_ARCH: slc7_amd64_gcc820

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 4, 2020

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 4, 2020

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 2 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2680577
  • DQMHistoTests: Total failures: 40
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2680218
  • DQMHistoTests: Total skipped: 319
  • 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

@fwyzard
Copy link
Contributor

fwyzard commented Mar 5, 2020

I've reformatted the FastReport lines to be more readable:

Before

CPU time avg. when run Real time avg. when run Alloc. avg. when run Dealloc. avg. when run Modules
4.0 ms 4.0 ms 4.1 ms 4.1 ms +607 kB +607 kB -512 kB -512 kB hltParticleFlowRecHitHBHE
3.9 ms 3.9 ms 4.0 ms 4.0 ms +1154 kB +1154 kB -907 kB -907 kB hltParticleFlowRecHitHF

With this PR

CPU time avg. when run Real time avg. when run Alloc. avg. when run Dealloc. avg. when run Modules
2.2 ms 2.2 ms 2.3 ms 2.3 ms +612 kB +612 kB -517 kB -517 kB hltParticleFlowRecHitHBHE
2.7 ms 2.7 ms 2.7 ms 2.7 ms +1147 kB +1147 kB -901 kB -901 kB hltParticleFlowRecHitHF

@hatakeyamak
Copy link
Contributor Author

hatakeyamak commented Mar 5, 2020

I've reformatted the FastReport lines to be more readable:

Thx! I propagated the reformat to the PR description.

@perrotta
Copy link
Contributor

perrotta commented Mar 6, 2020

In the initialization of the HCAL navigator a vector of vectors (dimension 9*(denseIdHcalMax_ - denseIdHcalMin_ + 1)) is stored in memory to speed up the navigation at every recHit.

I have evaluated with IgProf the live memory allocated for such an initialization, in the Phase2 wf 21234.0 (TTbar_14TeV+TTbar_14TeV_TuneCP5_2026D44), which including HGCal should represent a "worst case scenario" for it. IgProf says that 1 MB of memory will be allocated to store the neighboursHcal_ vectors of vectors:

            0.1  .............      1'079'832 / 1'079'832             14'699 / 14'699           PFHCALDenseIdNavigator<HcalDetId, HcalTopology, false>::init(edm::EventSetup const&) [2617]

I've also compared the CPU timing in that wf with and without this PR: in my comparison no significant difference is visible in the reco step. However, that was done with an extremely limited sample (10 evts). For a larger stat, I rather keep the timing measurement reported in the PR description for HLT. By the way, @hatakeyamak , do you also have a comparison for the timing in offline reco?

@hatakeyamak
Copy link
Contributor Author

@perrotta I added the offline timing test in the PR description as well. The difference is visible when we are running on PU samples in which rechit multiplicity is larger.

@perrotta
Copy link
Contributor

perrotta commented Mar 7, 2020

+1

  • The PFRecHit navigator for HCal/HGCal get speed up by caching at every lumi section the position of the neighbouring cells: this allocates up to 1 MB in memory for Phase2 events with HGCal
  • Such a speed up is more visible in the high PU Phase2, as reported in the PR description
  • Jenkins tests pass and show no differences with the current implementation

@hatakeyamak
Copy link
Contributor Author

Thank you @perrotta. I should have made it clear: this PR won't affect the navigator for HGCAL, and it speeds up only for HCAL.

@Martin-Grunewald
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 9, 2020

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

@silviodonato
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 761c1de into cms-sw:master Mar 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants