Allow caching of HFShowerLibrary #40278

Dr15Jones · 2022-12-09T18:52:39Z

PR description:

Modernized code related to HFShowerLibrary
decreased memory size of HFShowerPhoton by remove unnecessary virtualization
Added ability to cache entire contents of HFShowerLibrary file and share it safely across instances of modules. (Ability is off by default).

PR validation:

Added a unit test for HFShowerLibrary to show with and without caching give same results. Test also shows that all three existing data files can be read without errors.
Ran demonstrator which produces pileup files by running the GEN+SIM step for the minimum bias within the mixing module. This generated around 80,000 min bias per job (800 minibias/event with 100 events) using 8 threads without issues.

Comparing timing with and without caching showed a 1-2% speed improvement when using caching.

- removed virtual destructor to save 12 bytes of space when stored in a contiguous container. - explicitly have compiler create default constructor/operator=

- improved const correctness - moved member data which was only used as temporary storage to be function local variables (done in other classes as well). - encapsulate message logger calls within the log member function - made member data names conform to CMS standards - added unit tests - hold onto HFFibre directly, not via unique_ptr

- modified HFShowerLibrary to be easier to test by removing dependencies on conditions classes.

Use a helper class to manage the TBranch and how to read it back properly based on the file format.

The ability is off by default. The data can be safely shared by all HFShowerLibrary instances in a job.

Dr15Jones · 2022-12-09T18:52:53Z

please test

cmsbuild · 2022-12-09T19:00:18Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40278/33330

This PR adds an extra 60KB to repository

cmsbuild · 2022-12-09T19:00:47Z

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

It involves the following packages:

SimDataFormats/CaloHit (simulation)
SimG4CMS/Calo (simulation)

@civanch, @mdhildreth can you please review it and eventually sign? Thanks.
@youyingli, @lecriste, @apsallid, @felicepantaleo, @rovere, @missirol, @bsunanda, @thomreis, @simonepigazzini, @fabiocos, @slomeo this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

Dr15Jones · 2022-12-09T18:53:42Z

SimG4CMS/Calo/interface/HFFibre.h

-          const HcalSimulationParameters* hps,
-          edm::ParameterSet const& p);
-  ~HFFibre() = default;
+  HFFibre(const HcalDDDSimConstants* hcons, const HcalSimulationParameters* hps, edm::ParameterSet const& p);


The name was not used.

Dr15Jones · 2022-12-09T18:55:21Z

SimG4CMS/Calo/interface/HFFibre.h

+    std::array<double, 2> lambdaLimits_;
+  };
+
+  HFFibre(Params iP);


This helps with testing as one doesn't have to create a ParameterSet in order to setup the class.

Dr15Jones · 2022-12-09T18:55:51Z

SimG4CMS/Calo/interface/HFFibre.h


 private:
-  const HcalDDDSimConstants* hcalConstant_;


Removed variables were only used during construction so did not have to be kept around.

Dr15Jones · 2022-12-09T18:56:29Z

SimG4CMS/Calo/interface/HFShowerLibrary.h

+    int fileVersion_;
+    bool cacheBranches_ = false;
+  };
+  HFShowerLibrary(Params const &, FileParams const &, HFFibre::Params);


Helps with testing.

Dr15Jones · 2022-12-09T18:58:55Z

SimG4CMS/Calo/interface/HFShowerLibrary.h

+    [[nodiscard]] HFShowerPhotonCollection getRecord(int) const;
+
+  private:
+    HFShowerPhotonCollection photons_;


Photons for all entries are stored in the same collection in order to avoid overhead of many individual std::vector<>s. To find the entries one consults offsets_.

Dr15Jones · 2022-12-09T19:01:36Z

SimG4CMS/Calo/src/HFFibre.cc

-      ss1 << "\n";
+  nBinAtt_ = static_cast<int>(attL_.size());
+
+  edm::LogVerbatim("HFShower").log([&](auto& logger) {


LogVerbatim::log will only call the lambda if the message will potentially be sent. This avoids building the std::stringstream only to have its contents thrown away.

Dr15Jones · 2022-12-09T19:05:50Z

SimG4CMS/Calo/src/HFShowerLibrary.cc

+                                 const HcalSimulationParameters* hps,
+                                 edm::ParameterSet const& hfShower,
+                                 edm::ParameterSet const& hfShowerLibrary)
+    : HFShowerLibrary(paramsFrom(hfShower, hfShowerLibrary, hcons->getPhiTableHF().front()),


Having a constructor that takes the two different ParameterSets allowed those values to be passed to two different functions without having to call getParameterSet several times.

Dr15Jones · 2022-12-09T19:09:31Z

SimG4CMS/Calo/src/HFShowerLibrary.cc

+  if (nRecords > maxRecordsToCache) {
+    nRecords = maxRecordsToCache;
+  }
+  offsets_.reserve(nRecords + 1);


offsets_[0+record] returns the starting element in photons_ to read while offsets_[1+record] gives one passed the end. Hence why we need 1 more element.

Dr15Jones · 2022-12-09T19:10:54Z

SimG4CMS/Calo/src/HFShowerLibrary.cc


-void HFShowerLibrary::getRecord(int type, int record) {
+HFShowerPhotonCollection HFShowerLibrary::BranchReader::getRecordOldForm(TBranch* iBranch, int iEntry) {


Moved handling of the different file formats into their own functions.

cmsbuild · 2022-12-09T22:49:24Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9fae6b/29549/summary.html
COMMIT: caf555f
CMSSW: CMSSW_13_0_X_2022-12-09-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/40278/29549/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: 8 differences found in the comparisons
DQMHistoTests: Total files compared: 48
DQMHistoTests: Total histograms compared: 3421510
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3421482
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
Checked 206 log files, 158 edm output root files, 48 DQM output files
TriggerResults: no differences found

civanch · 2022-12-10T21:30:55Z

+1

For this type of modification regression means that nothing changed for Run2 and Run3. Class organisation is improved.

cmsbuild · 2022-12-10T21:31:16Z

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

perrotta · 2022-12-12T09:18:20Z

+1

Dr15Jones added 11 commits November 28, 2022 09:23

Changed member data to match CMS naming conventions

ab1d46c

Removed photon_ variable as photo_ can be used in the same places

645e235

Proper ownership of the TFile

26d212c

Replaced npe_ with pe_.size() in HFShowerLibrary

2d6399d

Improve HFShowerPhoton

c6ec962

- removed virtual destructor to save 12 bytes of space when stored in a contiguous container. - explicitly have compiler create default constructor/operator=

Removed photo_ as a member data and replace with local variables

3fce52c

Removed pe_ as a member data and replace with local variables

9da3e66

Added unit test for HFShowerLibrary

5d5561a

- modified HFShowerLibrary to be easier to test by removing dependencies on conditions classes.

Better TBranch handling in HFShowerLibrary

7f53f27

Use a helper class to manage the TBranch and how to read it back properly based on the file format.

Added ability to cache TBranch data in HFShowerLibrary

caf555f

The ability is off by default. The data can be safely shared by all HFShowerLibrary instances in a job.

cmsbuild added this to the CMSSW_13_0_X milestone Dec 9, 2022

cmsbuild added code-checks-pending orp-pending pending-signatures simulation-pending tests-started labels Dec 9, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 9, 2022

Dr15Jones commented Dec 9, 2022

View reviewed changes

cmsbuild added tests-approved and removed tests-started labels Dec 9, 2022

cmsbuild added fully-signed and removed simulation-pending pending-signatures labels Dec 10, 2022

cmsbuild added the simulation-approved label Dec 10, 2022

cmsbuild added orp-approved and removed orp-pending labels Dec 12, 2022

cmsbuild merged commit ce1f703 into cms-sw:master Dec 12, 2022

makortel mentioned this pull request Dec 12, 2022

Allow caching of HFShowerLibrary cms-sw/framework-team#468

Closed

Dr15Jones deleted the improveHFShowerLibrary branch December 13, 2022 16:18

abdoulline mentioned this pull request Dec 21, 2022

Weirdness in HFShowerLibrary handling of run 2 file #40218

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow caching of HFShowerLibrary #40278

Allow caching of HFShowerLibrary #40278

Dr15Jones commented Dec 9, 2022

Dr15Jones commented Dec 9, 2022

cmsbuild commented Dec 9, 2022

cmsbuild commented Dec 9, 2022

Dr15Jones Dec 9, 2022

Dr15Jones Dec 9, 2022

Dr15Jones Dec 9, 2022

Dr15Jones Dec 9, 2022

Dr15Jones Dec 9, 2022

Dr15Jones Dec 9, 2022

Dr15Jones Dec 9, 2022

Dr15Jones Dec 9, 2022

Dr15Jones Dec 9, 2022

cmsbuild commented Dec 9, 2022

civanch commented Dec 10, 2022

cmsbuild commented Dec 10, 2022

perrotta commented Dec 12, 2022


		void HFShowerLibrary::getRecord(int type, int record) {
		HFShowerPhotonCollection HFShowerLibrary::BranchReader::getRecordOldForm(TBranch* iBranch, int iEntry) {

Allow caching of HFShowerLibrary #40278

Allow caching of HFShowerLibrary #40278

Conversation

Dr15Jones commented Dec 9, 2022

PR description:

PR validation:

Dr15Jones commented Dec 9, 2022

cmsbuild commented Dec 9, 2022

cmsbuild commented Dec 9, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Dec 9, 2022

Comparison Summary

civanch commented Dec 10, 2022

cmsbuild commented Dec 10, 2022

perrotta commented Dec 12, 2022