Better destructor behavior for SiPixelTemplateStore2D #33105

OzAmram · 2021-03-08T20:08:29Z

This PR updates the SiPixelTemplate2D objects to use std::vectors rather than arrays allocated directly on the heap. This removes the need for the destroy() function. We expect no changes in output.

This addresses issue #32567 that was raised in PR #32494, which had a similar change for the 1D template.

@mmusich @tvami @pmaksim1 @tsusa

…icit memory management

cmsbuild · 2021-03-08T20:15:11Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33105/21436

This PR adds an extra 36KB to repository

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

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33105/21436/code-format.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33105/21436/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2021-03-08T20:57:49Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33105/21439

This PR adds an extra 36KB to repository

cmsbuild · 2021-03-08T20:58:08Z

A new Pull Request was created by @OzAmram (Oz Amram) for master.

It involves the following packages:

CondFormats/SiPixelTransient
RecoLocalTracker/SiPixelRecHits

@ggovi, @perrotta, @cmsbuild, @jpata, @slava77 can you please review it and eventually sign? Thanks.
@mtosi, @makortel, @felicepantaleo, @GiacomoSguazzoni, @JanFSchulte, @rovere, @VinInn, @OzAmram, @ferencek, @dkotlins, @gpetruc, @mmusich, @threus, @ebrondol, @seemasharmafnal, @tvami 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

slava77 · 2021-03-08T23:03:18Z

@cmsbuild please test

cmsbuild · 2021-03-09T02:12:10Z

+1

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

Comparison Summary

@slava77 comparisons for the following workflows were not done due to missing matrix map:

/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-134677/34634.0_TTbar_14TeV+2026D76+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal
/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-134677/34834.999_TTbar_14TeV+2026D76PU_PMXS1S2PR+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+PREMIX_PremixHLBeamSpot14PU+DigiTriggerPU+RecoGlobalPU+HARVESTGlobalPU

Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 38
DQMHistoTests: Total histograms compared: 2849195
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2849172
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 37 files compared)
Checked 160 log files, 37 edm output root files, 38 DQM output files

slava77 · 2021-03-10T02:25:56Z

CondFormats/SiPixelTransient/interface/SiPixelTemplate2D.h

-    }
-  }
+  std::vector<std::vector<SiPixelTemplateEntry2D>>
+      entry;  //!< use 2d entry to store BPix and FPix entries [dynamically allocated]


please move the comment above the code to avoid the line split

slava77 · 2021-03-10T02:30:10Z

CondFormats/SiPixelTransient/src/SiPixelTemplate2D.cc

+    for (int iy = 0; iy < theCurrentTemp.head.NTyx; ++iy)
+      theCurrentTemp.entry[iy].resize(theCurrentTemp.head.NTxx);


Suggested change

for (int iy = 0; iy < theCurrentTemp.head.NTyx; ++iy)

theCurrentTemp.entry[iy].resize(theCurrentTemp.head.NTxx);

for (auto& item : theCurrentTemp.entry)

item.resize(theCurrentTemp.head.NTxx);

is marginally shorter to write and may even be epsilon faster

slava77 · 2021-03-10T02:31:27Z

CondFormats/SiPixelTransient/src/SiPixelTemplate2D.cc

+    for (int iy = 0; iy < theCurrentTemp.head.NTyx; ++iy)
+      theCurrentTemp.entry[iy].resize(theCurrentTemp.head.NTxx);


Suggested change

for (int iy = 0; iy < theCurrentTemp.head.NTyx; ++iy)

theCurrentTemp.entry[iy].resize(theCurrentTemp.head.NTxx);

for (auto& item : theCurrentTemp.entry)

item.resize(theCurrentTemp.head.NTxx);

as above

cmsbuild · 2021-03-10T16:53:28Z

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

slava77 · 2021-03-10T18:17:47Z

@cmsbuild please test

cmsbuild · 2021-03-10T20:47:31Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-134677/13423/summary.html
COMMIT: 0e22d13
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/33105/13423/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: 5 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

slava77 · 2021-03-15T12:54:20Z

@cmsbuild please test

There is an apparent random difference in ticlTrackstersEM id probabilities wf D76 PMX 34834.999.
I'd like to confirm that this is transient.

slava77 · 2021-03-15T13:28:26Z

+1

for #33105 0e22d13

code changes are in line with the PR description and the follow up review. The changes are technical, no changes in physics are expected.
jenkins tests pass and comparisons with the baseline show no (relevant) differences

cmsbuild · 2021-03-15T15:33:16Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-134677/13514/summary.html
COMMIT: 0e22d13
CMSSW: CMSSW_11_3_X_2021-03-14-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/33105/13514/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: 37
DQMHistoTests: Total histograms compared: 2635087
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2635064
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
Checked 155 log files, 37 edm output root files, 37 DQM output files

ggovi · 2021-03-18T13:10:02Z

+1

cmsbuild · 2021-03-18T13:10:24Z

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 · 2021-03-18T13:15:02Z

+1

Changed to 2d vector for entries of 2d template object, to avoid expl…

5dff210

…icit memory management

cmsbuild added this to the CMSSW_11_3_X milestone Mar 8, 2021

cmsbuild added code-checks-pending db-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Mar 8, 2021

cmsbuild added code-checks-rejected and removed code-checks-pending labels Mar 8, 2021

Code format changes

3f6eee4

cmsbuild added code-checks-pending and removed code-checks-rejected labels Mar 8, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 8, 2021

cmsbuild added tests-started and removed tests-pending labels Mar 8, 2021

cmsbuild added tests-approved and removed tests-started labels Mar 9, 2021

slava77 reviewed Mar 10, 2021

View reviewed changes

Minor code style changes

0e22d13

cmsbuild added code-checks-pending tests-pending and removed code-checks-approved tests-approved labels Mar 10, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 10, 2021

cmsbuild added tests-started and removed tests-pending labels Mar 10, 2021

cmsbuild added tests-approved and removed tests-started labels Mar 10, 2021

cmsbuild added tests-started and removed tests-approved labels Mar 15, 2021

slava77 mentioned this pull request Mar 15, 2021

non-reproducibility in ticlTrackstersEM id probabilities wf D76 PMX 34834.999 #33180

Open

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Mar 15, 2021

cmsbuild added tests-approved and removed tests-started labels Mar 15, 2021

cmsbuild added db-approved fully-signed and removed db-pending pending-signatures labels Mar 18, 2021

cmsbuild added orp-approved and removed orp-pending labels Mar 18, 2021

cmsbuild merged commit df9b098 into cms-sw:master Mar 18, 2021

tvami mentioned this pull request Oct 5, 2021

SiPixelTemplateStore2D modernization #32567

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better destructor behavior for SiPixelTemplateStore2D #33105

Better destructor behavior for SiPixelTemplateStore2D #33105

OzAmram commented Mar 8, 2021

cmsbuild commented Mar 8, 2021

cmsbuild commented Mar 8, 2021

cmsbuild commented Mar 8, 2021

slava77 commented Mar 8, 2021

cmsbuild commented Mar 9, 2021

slava77 Mar 10, 2021

OzAmram Mar 10, 2021

slava77 Mar 10, 2021

OzAmram Mar 10, 2021

slava77 Mar 10, 2021

cmsbuild commented Mar 10, 2021

slava77 commented Mar 10, 2021

cmsbuild commented Mar 10, 2021

slava77 commented Mar 15, 2021

slava77 commented Mar 15, 2021

cmsbuild commented Mar 15, 2021

ggovi commented Mar 18, 2021

cmsbuild commented Mar 18, 2021

qliphy commented Mar 18, 2021

		for (int iy = 0; iy < theCurrentTemp.head.NTyx; ++iy)
		theCurrentTemp.entry[iy].resize(theCurrentTemp.head.NTxx);

Better destructor behavior for SiPixelTemplateStore2D #33105

Better destructor behavior for SiPixelTemplateStore2D #33105

Conversation

OzAmram commented Mar 8, 2021

cmsbuild commented Mar 8, 2021

cmsbuild commented Mar 8, 2021

cmsbuild commented Mar 8, 2021

slava77 commented Mar 8, 2021

cmsbuild commented Mar 9, 2021

Comparison Summary

slava77 Mar 10, 2021

Choose a reason for hiding this comment

OzAmram Mar 10, 2021

Choose a reason for hiding this comment

slava77 Mar 10, 2021

Choose a reason for hiding this comment

OzAmram Mar 10, 2021

Choose a reason for hiding this comment

slava77 Mar 10, 2021

Choose a reason for hiding this comment

cmsbuild commented Mar 10, 2021

slava77 commented Mar 10, 2021

cmsbuild commented Mar 10, 2021

Comparison Summary

slava77 commented Mar 15, 2021

slava77 commented Mar 15, 2021

cmsbuild commented Mar 15, 2021

Comparison Summary

ggovi commented Mar 18, 2021

cmsbuild commented Mar 18, 2021

qliphy commented Mar 18, 2021