Centralise payload ptr ownership for condition updating workflows #35048

ggovi · 2021-08-27T15:16:37Z

PR description:

A review of the overall CMSSW client code of the DBOutputService interface for creating/updating condition data sets (sequences of pairs iov/payload ), has found that the clients are mostly assuming (wrongly) the service to take over the ownership, resulting in a generalised memory leak.
Only few classes are currently implementing the policy correctly.
To cope with that, I propose the following changes:

Implement a centralised policy in the service, taking over (as expected by most of the clients) the payload pointer ownership. This will happen by calling the already existing function signatures accepting a pointer as input: writeOne, createNewIOV, appendSinceTime. The called function will take care to free the allocated memory. For the caller, no change is strictly required if it does not free the memory after the call.
Introduce a new set of methods with a reference-based signature for writeOne, createNewIOV and appendSinceTime.
These new methods are used in this PR to migrate the code of the clients already holding the payload ptr ownership. In addition, all of the other clients, currently not freeing the payload memory, will be asked to move to this interface. This change will improve the overall memory management and the code readability.
Introduce a new method "writeMany" scoped to the atomic update of many iovs/payloads. In this PR, this is used for a new implementation of the PopCon workflow.
Modify the PopCon interface to provide a safer memory management: on the side of the existing vector m_to_transfer holding bare payload pointers has been added a new container ("m_iovs") of payloads based on shared_ptr. The client of PopCon are expected to migrate their code to replace the use of m_to_tranfer in favour of m_iovs.

PR validation:

Unit tests/Integration test/O2O tests

…interface

…utService interface

cmsbuild · 2021-08-27T15:22:52Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35048/24919

This PR adds an extra 80KB to repository

cmsbuild · 2021-08-27T15:23:15Z

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

It involves the following packages:

CalibCalorimetry/CaloMiscalibTools (alca)
CalibTracker/SiPixelQuality (alca)
CalibTracker/SiStripESProducers (alca)
Calibration/LumiAlCaRecoProducers (alca)
CommonTools/ConditionDBWriter (db)
CondCore/DBOutputService (db, alca)
CondCore/PopCon (db, alca)
CondFormats/MFObjects (alca)
CondTools/RunInfo (db)
CondTools/SiPixel (db)
CondTools/SiStrip (db)
IOMC/EventVertexGenerators (simulation)
RecoHI/HiJetAlgos (reconstruction)

@malbouis, @civanch, @yuanchao, @tlampen, @mdhildreth, @cmsbuild, @jpata, @slava77, @ggovi, @pohsun, @francescobrivio, @tvami can you please review it and eventually sign? Thanks.
@echabert, @yslai, @pieterdavid, @robervalwalsh, @OzAmram, @thomreis, @seemasharmafnal, @dgulhan, @yenjie, @ferencek, @mandrenguyen, @yetkinyilmaz, @tocheng, @VinInn, @simonepigazzini, @mmusich, @fabiocos, @jan-kaspar, @gbenelli, @jazzitup, @dkotlins, @kurtejung, @tvami 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

tvami · 2021-08-27T15:23:59Z

CommonTools/ConditionDBWriter/interface/ConditionDBWriter.h

@@ -333,7 +333,7 @@ class ConditionDBWriter : public edm::EDAnalyzer {
    edm::LogInfo("ConditionDBWriter") << "appending a new object to tag " << Record_ << " in since mode " << std::endl;

    // The Framework will take control over the DB object now, therefore the release.


Now this comment is not in line with what's below it, right?

Yes it is wrong. Maybe, since it is a file that I have modified, better remove it. Good catch.

ggovi · 2021-08-27T15:25:22Z

Please test

tvami · 2021-08-27T15:30:23Z

fixes #28699

ggovi · 2021-08-27T16:40:19Z

please test

cmsbuild · 2021-08-27T20:37:22Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ccfe70/18100/summary.html
COMMIT: 40f6c27
CMSSW: CMSSW_12_1_X_2021-08-27-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35048/18100/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS StaticAnalyzer warnings: There are 2 EventSetupRecord::get warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ccfe70/18100/llvm-analysis/esrget-sa.txt for details.
CMS StaticAnalyzer warnings: There are 2 inherits from legacy modules warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ccfe70/18100/llvm-analysis/legacy-mod-sa.txt for details.

slava77 · 2021-08-28T00:03:57Z

+reconstruction

for #35048 40f6c27

code changes in reco are for a straightforward update in RecoHI/HiJetAlgos/plugins/UETableProducer.cc for the interface change in createNewIOV
jenkins tests pass

cmsbuild · 2021-08-30T14:23:01Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35048/24954

This PR adds an extra 80KB to repository

cmsbuild · 2021-08-30T14:23:27Z

Pull request #35048 was updated. @malbouis, @yuanchao, @tlampen, @pohsun, @francescobrivio, @tvami can you please check and sign again.

tvami · 2021-08-30T14:27:42Z

+alca

cmsbuild · 2021-08-30T14:28:07Z

This pull request is fully signed and it will be integrated in one of the next master IBs after it passes the integration tests. This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

ggovi · 2021-08-30T14:28:11Z

please test

cmsbuild · 2021-08-30T17:57:50Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ccfe70/18134/summary.html
COMMIT: 0eada5d
CMSSW: CMSSW_12_1_X_2021-08-29-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35048/18134/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS StaticAnalyzer warnings: There are 2 EventSetupRecord::get warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ccfe70/18134/llvm-analysis/esrget-sa.txt for details.
CMS StaticAnalyzer warnings: There are 2 inherits from legacy modules warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ccfe70/18134/llvm-analysis/legacy-mod-sa.txt for details.

Comparison Summary

Summary:

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

tvami · 2021-08-30T18:17:28Z

@tvami now it's there. thanks... we need the signatures again...

Interesting that GitHub didn't want the signatures again! Maybe changing comments doesn't trigger new signatures?

perrotta · 2021-08-31T05:39:45Z

Calibration/LumiAlCaRecoProducers/plugins/CorrPCCProducer.cc

@@ -580,8 +580,6 @@ void CorrPCCProducer::dqmEndRunProduce(edm::Run const& runSeg, const edm::EventS
      throw std::runtime_error("PoolDBService required.");
    }

-    delete pccCorrections;


This deletion can (must) be avoided here because writeOne is used in its "deprecated" form: correct?
Shouldn't you try to move to the "non deprecated" form with the reference as argument, instead of the pointer?

@perrotta The idea was to minimise the changes in this PR, and leave to the various code authors the migration to the new interface. Here we clearly had to options: 1. Remove the delete and leave the old interface 2. Leave the delete and move to new (reference-based) interface. I think 2 would not be fully satisfactory since we would be left with the use of pointers that has no justification. So, we should also remove the use of pointers.

perrotta · 2021-08-31T10:19:36Z

It is curious that signatures were not reset after the last update in #35048 (comment) : @smuzaffar @mrodozov can you see any issue in the bot?
@cms-sw/reconstruction-l2 @cms-sw/simulation-l2 @cms-sw/db-l2 I imagine that you do not have any additional remark after that last update, which only removed an outdated comment... But we can wait a bit to merge, just in case you notice anything wrong after the final squash.

smuzaffar · 2021-08-31T11:24:44Z

@perrotta , bot uses the timestamp of the latest commit and based on that it reset's the signatures. I think in this case the commit pushed were older than the signature comments

perrotta · 2021-08-31T16:23:59Z

+1

Changes since last signature are only the removal of one outdated line
Nobody at the ORP meeting complained with it

ggovi added 4 commits August 27, 2021 16:33

Implement centralized pointer ownership and reference-based interface

b86c5be

Implement centralized payload pointer ownership and shared_ptr based …

24dc549

…interface

Adapted to new PopCon interface

beace2e

Moving clients holding the payload pointer ownership to the new DBOup…

40f6c27

…utService interface

cmsbuild added this to the CMSSW_12_1_X milestone Aug 27, 2021

cmsbuild added alca-pending code-checks-pending db-pending orp-pending pending-signatures reconstruction-pending simulation-pending tests-pending labels Aug 27, 2021

ggovi mentioned this pull request Aug 27, 2021

PopConSourceHandler leaks memory #28699

Closed

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 27, 2021

tvami reviewed Aug 27, 2021

View reviewed changes

cmsbuild added tests-started and removed tests-pending labels Aug 27, 2021

Cosmetics on comments...

0eada5d

cmsbuild added tests-approved and removed tests-started labels Aug 27, 2021

cmsbuild removed the reconstruction-pending label Aug 28, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 30, 2021

cmsbuild added alca-approved fully-signed and removed alca-pending pending-signatures labels Aug 30, 2021

cmsbuild mentioned this pull request Aug 30, 2021

ECAL esConsumes migration of CalibCalorimetry packages #35077

Merged

cmsbuild added tests-approved and removed tests-started labels Aug 30, 2021

perrotta reviewed Aug 31, 2021

View reviewed changes

cmsbuild added orp-approved and removed orp-pending labels Aug 31, 2021

cmsbuild merged commit d13529b into cms-sw:master Aug 31, 2021

francescobrivio mentioned this pull request Sep 15, 2021

DBouputService migration cms-AlCaDB/AlCaTools#28

Closed

17 tasks

thomreis mentioned this pull request Sep 29, 2021

Fix EcalDQMStatusWriter to watch runs and load the input file #35466

Merged

mmusich mentioned this pull request Oct 19, 2021

Fix pointers double deletions from PoolDBOutputService in CondTools subsystem #35731

Merged

ggovi mentioned this pull request Oct 22, 2021

Fixes for double delete in using DBOutputService #35781

Merged

This was referenced Nov 4, 2021

migrate SiStrip codes to new PoolDBOutputService methods #35999

Merged

migrate SiPixel codes to new PoolDBOutputService methods #36040

Merged

migrate Alignment codes to new PoolDBOutputService methods #36076

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Centralise payload ptr ownership for condition updating workflows #35048

Centralise payload ptr ownership for condition updating workflows #35048

ggovi commented Aug 27, 2021 •

edited

Loading

cmsbuild commented Aug 27, 2021

cmsbuild commented Aug 27, 2021

tvami Aug 27, 2021

ggovi Aug 27, 2021

ggovi commented Aug 27, 2021

tvami commented Aug 27, 2021

ggovi commented Aug 27, 2021

cmsbuild commented Aug 27, 2021

slava77 commented Aug 28, 2021

cmsbuild commented Aug 30, 2021

cmsbuild commented Aug 30, 2021

tvami commented Aug 30, 2021

cmsbuild commented Aug 30, 2021

ggovi commented Aug 30, 2021

cmsbuild commented Aug 30, 2021

tvami commented Aug 30, 2021

perrotta Aug 31, 2021

ggovi Aug 31, 2021 •

edited

Loading

perrotta commented Aug 31, 2021

smuzaffar commented Aug 31, 2021

perrotta commented Aug 31, 2021

		@@ -333,7 +333,7 @@ class ConditionDBWriter : public edm::EDAnalyzer {
		edm::LogInfo("ConditionDBWriter") << "appending a new object to tag " << Record_ << " in since mode " << std::endl;

		// The Framework will take control over the DB object now, therefore the release.

Centralise payload ptr ownership for condition updating workflows #35048

Centralise payload ptr ownership for condition updating workflows #35048

Conversation

ggovi commented Aug 27, 2021 • edited Loading

PR description:

PR validation:

cmsbuild commented Aug 27, 2021

cmsbuild commented Aug 27, 2021

tvami Aug 27, 2021

Choose a reason for hiding this comment

ggovi Aug 27, 2021

Choose a reason for hiding this comment

ggovi commented Aug 27, 2021

tvami commented Aug 27, 2021

ggovi commented Aug 27, 2021

cmsbuild commented Aug 27, 2021

slava77 commented Aug 28, 2021

cmsbuild commented Aug 30, 2021

cmsbuild commented Aug 30, 2021

tvami commented Aug 30, 2021

cmsbuild commented Aug 30, 2021

ggovi commented Aug 30, 2021

cmsbuild commented Aug 30, 2021

Comparison Summary

tvami commented Aug 30, 2021

perrotta Aug 31, 2021

Choose a reason for hiding this comment

ggovi Aug 31, 2021 • edited Loading

Choose a reason for hiding this comment

perrotta commented Aug 31, 2021

smuzaffar commented Aug 31, 2021

perrotta commented Aug 31, 2021

ggovi commented Aug 27, 2021 •

edited

Loading

ggovi Aug 31, 2021 •

edited

Loading