Migrate to the new ref. based writeOneIOV() in Alignment package #35711

yuanchao · 2021-10-17T20:40:44Z

PR description:

The new reference based method of the DBOutputService, writeOneIOV(), is introduced to keep the ownership of DB payload at the client side. The client will need to take care of the object memory. This PR migrates the old method, writeOne(), to writeOneIOV() in the following code:

Alignment/CommonAlignmentAlgorithm/plugins/SiPixelLorentzAngleCalibration.cc
Alignment/CommonAlignmentAlgorithm/plugins/SiStripBackplaneCalibration.cc
Alignment/CommonAlignmentAlgorithm/plugins/SiStripLorentzAngleCalibration.cc
Alignment/CommonAlignmentProducer/plugins/GlobalTrackerMuonAlignment.cc
Alignment/CommonAlignmentProducer/src/AlignmentProducerBase.cc
Alignment/LaserAlignment/plugins/LaserAlignment.cc
Alignment/MuonAlignment/plugins/MuonMisalignedProducer.cc
Alignment/MuonAlignment/src/MuonAlignment.cc
Alignment/OfflineValidation/plugins/TrackerGeometryCompare.cc
Alignment/SurveyAnalysis/plugins/SurveyDBUploader.cc
Alignment/SurveyAnalysis/plugins/SurveyInputTrackerFromDB.cc
Alignment/TrackerAlignment/plugins/MisalignedTrackerESProducer.cc
Alignment/TrackerAlignment/plugins/TrackerSystematicMisalignments.cc
Alignment/TrackerAlignment/src/TrackerAlignment.cc

** Please note that for payload objects created with 'new' function are not freed!

PR validation:

Code compiles.

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

Not a backport and no backport is foreseen.

cmsbuild · 2021-10-17T20:49:16Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35711/26019

This PR adds an extra 92KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File Alignment/MuonAlignment/src/MuonAlignment.cc modified in PR(s): [Draft] esConsumes migration #35659

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-35711/26019/code-format.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35711/26019/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

tvami · 2021-10-18T08:11:52Z

Alignment/CommonAlignmentAlgorithm/plugins/SiStripBackplaneCalibration.cc

@@ -377,7 +377,7 @@ void SiStripBackplaneCalibration::endOfJob() {
    if (saveToDB_) {  // If requested, write out to DB
      edm::Service<cond::service::PoolDBOutputService> dbService;
      if (dbService.isAvailable()) {
-        dbService->writeOne(output, firstRunOfIOV, recordNameDBwrite_);
+        dbService->writeOneIOV(*output, firstRunOfIOV, recordNameDBwrite_);


Suggested change

dbService->writeOneIOV(*output, firstRunOfIOV, recordNameDBwrite_);

dbService->writeOneIOV(&output, firstRunOfIOV, recordNameDBwrite_);

L333 SiStripBackPlaneCorrection *output = new SiStripBackPlaneCorrection;
Here output is a pointer.

Yes, and we want to pass the reference to that pointer instead, as you write in the title of the PR. We can chat about this in our skype if you want @yuanchao

@tvami Then "output" should be created as a normal instance of SiStripBackPlaneCorrection. And the rest of the code needs to be modify to cope with it. Or the client needs to delete this obj. somewhere later when no further usage. (ex. move L383 "delete output" out of the else{} before L387) That's what @GiacomoSguazzoni and I just discussed on skype. (in either cases, it can't be done easily with scripts)

I am not sure this piece of code is actually tested in unit test.
How can we make sure it's not broken by the updates?

the Call with *output is the one correct, but make sure the object is deleted at the end of its use. The method by reference (writeOnIOV) does not 'see' pointers, so it can't take care about the delete!

tvami · 2021-10-18T09:08:56Z

Hi @yuanchao I think you changed to the pointer many time when you should have kept the reference, no?

cmsbuild · 2021-10-18T09:11:17Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35711/26024

This PR adds an extra 92KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File Alignment/MuonAlignment/src/MuonAlignment.cc modified in PR(s): [Draft] esConsumes migration #35659

cmsbuild · 2021-10-18T09:11:41Z

A new Pull Request was created by @yuanchao (Yuan CHAO) for master.

It involves the following packages:

Alignment/CommonAlignmentAlgorithm (alca)
Alignment/CommonAlignmentProducer (alca)
Alignment/LaserAlignment (alca)
Alignment/MuonAlignment (alca)
Alignment/OfflineValidation (alca)
Alignment/SurveyAnalysis (alca)
Alignment/TrackerAlignment (alca)

@cmsbuild, @malbouis, @tvami, @yuanchao, @francescobrivio can you please review it and eventually sign? Thanks.
@pakhotin, @adewit, @abbiendi, @jhgoh, @tocheng, @tlampen, @mmusich, @trocino 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

yuanchao · 2021-10-18T09:31:55Z

@cmsbuild please test

cmsbuild · 2021-10-18T13:02:38Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7367f9/19706/summary.html
COMMIT: 372e6a3
CMSSW: CMSSW_12_1_X_2021-10-17-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35711/19706/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS StaticAnalyzer warnings: There are 11 EventSetupRecord::get warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7367f9/19706/llvm-analysis/esrget-sa.txt for details.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 8 differences found in the comparisons
DQMHistoTests: Total files compared: 40
DQMHistoTests: Total histograms compared: 2751113
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2751084
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.004 KiB( 39 files compared)
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 170 log files, 37 edm output root files, 40 DQM output files
TriggerResults: no differences found

tvami · 2021-10-19T14:10:43Z

-alca

discussed privately that the PR is to be closed
we'll ask the subsystems to look at their files instead of a central migration (it also seems we'll have a bit of more time in 12_1_X, more info after ORP)

perrotta · 2021-10-20T15:41:54Z

@yuanchao could you please close this PR, if that is the decision taken for it?

migrate to ref. based writeOneIOV()

8fddb95

cmsbuild added this to the CMSSW_12_1_X milestone Oct 17, 2021

cmsbuild added alca-pending code-checks-pending orp-pending pending-signatures tests-pending labels Oct 17, 2021

cmsbuild added code-checks-rejected and removed code-checks-pending labels Oct 17, 2021

fixed code-check errors

372e6a3

tvami reviewed Oct 18, 2021

View reviewed changes

cmsbuild added code-checks-pending and removed code-checks-rejected labels Oct 18, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 18, 2021

cmsbuild added tests-started and removed tests-pending labels Oct 18, 2021

cmsbuild added tests-approved and removed tests-started labels Oct 18, 2021

cmsbuild mentioned this pull request Oct 18, 2021

Migrate to esConsumes muon alignment #35719

Merged

cmsbuild added alca-rejected and removed alca-pending labels Oct 19, 2021

mmusich mentioned this pull request Oct 21, 2021

Deprecated ES get functions in Alignment/CommonAlignmentAlgorithm cms-AlCaDB/AlCaTools#44

Closed

This was referenced Oct 22, 2021

Fixes for double delete in using DBOutputService #35781

Merged

Improve various Patatrack Kernels #35598

Closed

Common alignment algorithm consumes migration #35861

Merged

smuzaffar modified the milestones: CMSSW_12_1_X, CMSSW_12_2_X Oct 29, 2021

cmsbuild mentioned this pull request Nov 5, 2021

[DD4hep] use DD4hep consistently in configurations #36004

Merged

qliphy closed this Nov 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to the new ref. based writeOneIOV() in Alignment package #35711

Migrate to the new ref. based writeOneIOV() in Alignment package #35711

yuanchao commented Oct 17, 2021

cmsbuild commented Oct 17, 2021

tvami Oct 18, 2021

yuanchao Oct 18, 2021

tvami Oct 18, 2021

yuanchao Oct 18, 2021 •

edited

Loading

mmusich Oct 18, 2021

ggovi Oct 19, 2021

tvami commented Oct 18, 2021

cmsbuild commented Oct 18, 2021

cmsbuild commented Oct 18, 2021

yuanchao commented Oct 18, 2021

cmsbuild commented Oct 18, 2021

tvami commented Oct 19, 2021

perrotta commented Oct 20, 2021

	dbService->writeOneIOV(*output, firstRunOfIOV, recordNameDBwrite_);
	dbService->writeOneIOV(&output, firstRunOfIOV, recordNameDBwrite_);

Migrate to the new ref. based writeOneIOV() in Alignment package #35711

Migrate to the new ref. based writeOneIOV() in Alignment package #35711

Conversation

yuanchao commented Oct 17, 2021

PR description:

PR validation:

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

cmsbuild commented Oct 17, 2021

tvami Oct 18, 2021

Choose a reason for hiding this comment

yuanchao Oct 18, 2021

Choose a reason for hiding this comment

tvami Oct 18, 2021

Choose a reason for hiding this comment

yuanchao Oct 18, 2021 • edited Loading

Choose a reason for hiding this comment

mmusich Oct 18, 2021

Choose a reason for hiding this comment

ggovi Oct 19, 2021

Choose a reason for hiding this comment

tvami commented Oct 18, 2021

cmsbuild commented Oct 18, 2021

cmsbuild commented Oct 18, 2021

yuanchao commented Oct 18, 2021

cmsbuild commented Oct 18, 2021

Comparison Summary

tvami commented Oct 19, 2021

perrotta commented Oct 20, 2021

yuanchao Oct 18, 2021 •

edited

Loading