Avoid extra memory use in CaloSubdetectorGeometry

[Dr15Jones]

PR description:

• Created CaloCellGeometry*Ptr classes for controlling access to cells held or created by Calo geometry classes
• PFRecHit still holds a std::shared_ptr as ROOT requires the class to have a working copy constructor.
• Fixed possible out of bounds reads on containers where index is identical to the size of the container.

This was based on the issue #46433

PR validation:

Code compiles. New unit test passes.

[cmsbuild]

cms-bot internal usage

[Dr15Jones]

please test

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46507/42362

• There are other open Pull requests which might conflict with changes you have proposed:
  • File RecoEcal/EgammaCoreTools/src/EcalClustersGraph.cc modified in PR(s): Zero-padding optimization for DeepSC inference #40934

[cmsbuild]

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

It involves the following packages:

• Geometry/CaloGeometry (geometry)
• Geometry/EcalAlgo (geometry)
• Geometry/EcalTestBeam (geometry)
• Geometry/ForwardGeometry (geometry)
• Geometry/HGCalGeometry (geometry, upgrade)
• Geometry/HcalTowerAlgo (geometry)
• RecoEcal/EgammaCoreTools (reconstruction)
• RecoEgamma/EgammaTools (reconstruction)
• RecoMET/METAlgorithms (reconstruction)
• RecoParticleFlow/PFClusterProducer (reconstruction)
• RecoParticleFlow/PFRecHitProducer (reconstruction)

@Dr15Jones, @Moanwar, @bsunanda, @civanch, @jfernan2, @kpedro88, @makortel, @mandrenguyen, @mdhildreth, @srimanob, @subirsarkar can you please review it and eventually sign? Thanks.
@Prasant1993, @ReyerBand, @Sam-Harper, @a-kapoor, @afiqaize, @ahinzmann, @argiro, @bsunanda, @fabiocos, @felicepantaleo, @gkasieczka, @gouskos, @hatakeyamak, @jainshilpi, @jdamgov, @jdolen, @lgray, @mariadalfonso, @martinamalberti, @missirol, @mmarionncern, @nhanvtran, @ram1123, @rchatter, @rovere, @sameasy, @schoef, @seemasharmafnal, @sobhatta, @thomreis, @valsdav, @varuns23, @wang0jin this is something you requested to watch as well.
@antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

[Dr15Jones]

I would have liked to remove the use of std::shared_ptr in PFRecHit. The problem is HGCalGeometry::getGeometry can return newly made cell which will need to be deleted (hence the existence of CaloCellGeometryMayOwnPtr. I couldn't get PFRecHit to use a CaloCellGeometryMayOwnPtr as the latter has no viable copy constructor (only a move constructor) and ROOT requires all storable classes (for which PFRecHit is one) to HAVE a copy constructor.

To make a workable copy constructor for PFRecHit would mean either

1. All CaloCellGeometry classes would need a clone method to be able to make new copies as needed which are then uniquely owned or
2. PFRecHit needs to be able to share ownership of a CaloCellGeometry (which is why the use of std::shared_ptr works).

A shared ownership could be achieved if CaloCellGeometry were changed to have an embedded reference count and then CaloCellGeometryMayOwnPtr could use that reference count to decide when the cell might have to be deleted.

Looking at the CaloCellGeometry classes, it does look like they might have extra padding bytes so adding the reference count might not change their actual memory footprint.

[Dr15Jones]

NOTE: as an experiment I added an int member data to the end of FlatHexagon and FlatTrd (those are the only two that get created on calls to getGeometry). Added the member did cause it to grow by 16 bytes so it doesn't look like those classes have any padding at the end. I then changed the base class CaloCellGeometry to have an extra int at the end and in that case neither of the previous two classes had their size changed. So it looks like there are extra padding bits between where data for CaloCellGeometry ends and where those other two classes begin.

[makortel]

type performance-improvements

[cmsbuild]

+1

Size: This PR adds an extra 104KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6a24b3/42388/summary.html
COMMIT: 6a9a979
CMSSW: CMSSW_14_2_X_2024-10-24-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/46507/42388/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

• You potentially removed 1 lines from the logs
• Reco comparison results: 15 differences found in the comparisons
• DQMHistoTests: Total files compared: 46
• DQMHistoTests: Total histograms compared: 3566343
• DQMHistoTests: Total failures: 424
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 3565899
• DQMHistoTests: Total skipped: 20
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 45 files compared)
• Checked 201 log files, 171 edm output root files, 46 DQM output files
• TriggerResults: no differences found

[bsunanda]

+geometry

[makortel]

The ~8 MB increase in maxmemory is likely related to #46359 and the substantial number of packages being checked out.

Looking at the logs directly, e.g. 141.046 (EGamma 2023D) step 3 (reco) the total number of allocations decreases from 2998 million to 2775 million, i.e. a reduction of 222 million allocations, or 7.4 %.

[makortel]

In 29634.0 (TTbar 2026D110) step 3 (reco) the number of allocations decreases from 250 million to 245 million, i.e. a reduction of 4.9 million or 2.0 %.

[jfernan2]

+1

[Moanwar]

+Upgrade

[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. @sextonkennedy, @rappoccio, @mandrenguyen, @antoniovilela (and backports should be raised in the release meeting by the corresponding L2)

[makortel]

@Dr15Jones Could you remove the [RFC] from the title?

[Dr15Jones]

I removed the [RFC]. I am working on doing the reference count version of this code, but there isn't really a reason to wait for that.

[mandrenguyen]

+1