HBHE M2 = fix Memory Leak #17821

mariadalfonso · 2017-03-08T09:02:48Z

No changes are expected at relVal level

cmsbuild · 2017-03-08T09:03:04Z

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

It involves the following packages:

RecoLocalCalo/HcalRecAlgos

@perrotta, @cmsbuild, @slava77, @davidlange6 can you please review it and eventually sign? Thanks.
@argiro this is something you requested to watch as well.
@Muzaffar, @davidlange6, @smuzaffar you are the release manager for this.

cms-bot commands are listed here #13028

perrotta · 2017-03-08T09:52:36Z

RecoLocalCalo/HcalRecAlgos/src/PulseShapeFitOOTPileupCorrection.cc

-
+  // initialize for every different channel types (HPD vs SiPM)
+
+  if (!(&ps == currentPulseShape_ && isHPD == isCurrentChannelHPD_))


Both "currentPulseShape_" and "isCurrentChannelHPD_" are used uninitialized here

igv4321 · 2017-03-08T10:32:09Z

Maria, please initialize currentPulseShape_ in the constructor to nullptr.

You also do need to fix the memory leak. Replace the line
spfunctor_ = new ROOT::Math::Functor(...);
with
delete spfunctor_; spfunctor_ = new ROOT::Math::Functor(...);
The same for dpfunctor_ and tpfunctor_.

mariadalfonso · 2017-03-08T10:51:52Z

@igv4321 , fixed as you suggested, is ok now ?

cmsbuild · 2017-03-08T10:51:53Z

Pull request #17821 was updated. @perrotta, @cmsbuild, @slava77, @davidlange6 can you please check and sign again.

igv4321 · 2017-03-08T10:56:21Z

This will work

slava77 · 2017-03-08T16:44:58Z

@cmsbuild please test

cmsbuild · 2017-03-08T16:45:11Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/18250/console Started: 2017/03/08 17:50

Dr15Jones · 2017-03-08T17:00:51Z

RecoLocalCalo/HcalRecAlgos/src/PulseShapeFitOOTPileupCorrection.cc

-   spfunctor_    = new ROOT::Math::Functor(psfPtr_.get(),&FitterFuncs::PulseShapeFunctor::singlePulseShapeFunc, 3);
-   dpfunctor_    = new ROOT::Math::Functor(psfPtr_.get(),&FitterFuncs::PulseShapeFunctor::doublePulseShapeFunc, 5);
-   tpfunctor_    = new ROOT::Math::Functor(psfPtr_.get(),&FitterFuncs::PulseShapeFunctor::triplePulseShapeFunc, 7);
+   delete spfunctor_; spfunctor_    = new ROOT::Math::Functor(psfPtr_.get(),&FitterFuncs::PulseShapeFunctor::singlePulseShapeFunc, 3);


If you changed *functor_ to be std::unique_ptr<ROOT::Math::Functor> this would automatically handle deleting both here and in the destructor.

cmsbuild · 2017-03-08T17:39:43Z

+1
Tested at: ae88bd8
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-17821/18250/summary.html

cmsbuild · 2017-03-08T17:39:46Z

Comparison job queued.

cmsbuild · 2017-03-08T19:34:42Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-17821/18250/summary.html

Dr15Jones · 2017-03-09T01:13:18Z

RecoLocalCalo/HcalRecAlgos/src/PulseShapeFitOOTPileupCorrection.cc

-   spfunctor_    = new ROOT::Math::Functor(psfPtr_.get(),&FitterFuncs::PulseShapeFunctor::singlePulseShapeFunc, 3);
-   dpfunctor_    = new ROOT::Math::Functor(psfPtr_.get(),&FitterFuncs::PulseShapeFunctor::doublePulseShapeFunc, 5);
-   tpfunctor_    = new ROOT::Math::Functor(psfPtr_.get(),&FitterFuncs::PulseShapeFunctor::triplePulseShapeFunc, 7);
+   spfunctor_    = std::unique_ptr<ROOT::Math::Functor>( new ROOT::Math::Functor(psfPtr_.get(),&FitterFuncs::PulseShapeFunctor::singlePulseShapeFunc, 3) );


If you like, you could save yourself some typing by doing

spfunctor_ = std::make_unique<ROOT::Math::Functor>(psfPtr_.get(),&FitterFuncs::PulseShapeFunctor::singlePulseShapeFunc, 3);

Dr15Jones · 2017-03-09T01:14:58Z

The move to std::unique_ptr looks good to me. The use of std::make_unique is only a suggestion.

cmsbuild · 2017-03-09T01:37:56Z

+1
Tested at: dabac13
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-17821/18268/summary.html

cmsbuild · 2017-03-09T01:37:58Z

Comparison job queued.

cmsbuild · 2017-03-09T03:03:44Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-17821/18268/summary.html

perrotta · 2017-03-09T13:20:40Z

Still "isCurrentChannelHPD_" is used uninitialized at line (now) 280 of PulseShapeFitOOTPileupCorrection.cc

igv4321 · 2017-03-09T15:36:25Z

It is initialized on line 285. Good enough because in the expression A && B, B is never executed if A is false, and on the first comparison &ps == currentPulseShape_ is false (ps is a reference, so &ps is not a null pointer).

slava77 · 2017-03-09T15:44:06Z

@mariadalfonso @igv4321
please add a link or attach a valgrind report just to see what else is there

igv4321 · 2017-03-09T15:45:46Z

You are welcome to take a look at /uscms_data/d3/pedrok/hf/reco/CMSSW_8_4_0/src/10024.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2017_GenSimFull+DigiFull_2017+RecoFull_2017+ALCAFull_2017+HARVESTFull_2017/log_valgrind1.log

perrotta · 2017-03-09T16:13:23Z

Thank you @igv4321 : could you please copy the valgrind report on afs, or somewhere else publicly accessible?

kpedro88 · 2017-03-09T16:20:03Z

@perrotta: /afs/cern.ch/user/p/pedrok/public/log_valgrind1.log

Dr15Jones · 2017-03-09T17:20:40Z

The valgrind log pointed to this problem as well: #17861

slava77 · 2017-03-09T17:56:45Z

On 3/9/17 9:20 AM, Chris Jones wrote: The valgrind log pointed to this problem as well: #17861 <#17861>

Other than this BasicHepMC and the massive leak in M2 fixed here, I haven't noticed anything that will grow with the number of events.

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#17821 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEdcblV2yHQwiLmKc0bEyi48IrNPMtlsks5rkDTogaJpZM4MWhy8>.

perrotta · 2017-03-10T14:25:58Z

+1
Memory leak is cured, as intended.
No regression expected, no regression observed.

cmsbuild · 2017-03-10T14:26:14Z

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 requires discussion in the ORP meeting before it's merged. @Muzaffar, @davidlange6, @smuzaffar

davidlange6 · 2017-03-10T14:32:52Z

+1

fixMemLeakM2

6e6a96f

cmsbuild added this to the Next CMSSW_9_1_X milestone Mar 8, 2017

cmsbuild added comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Mar 8, 2017

mariadalfonso mentioned this pull request Mar 8, 2017

HBHE M2 = fix Memory Leak (90X) #17822

Merged

perrotta reviewed Mar 8, 2017

View reviewed changes

more fix

ae88bd8

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

Dr15Jones reviewed Mar 8, 2017

View reviewed changes

cmsbuild added tests-approved and removed tests-started labels Mar 8, 2017

cmsbuild added comparison-available and removed comparison-pending labels Mar 8, 2017

use std::unique_ptr

dabac13

cmsbuild removed comparison-available tests-approved labels Mar 9, 2017

cmsbuild added tests-started and removed tests-pending labels Mar 9, 2017

Dr15Jones reviewed Mar 9, 2017

View reviewed changes

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

cmsbuild added comparison-available and removed comparison-pending labels Mar 9, 2017

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Mar 10, 2017

cmsbuild added orp-approved and removed orp-pending labels Mar 10, 2017

cmsbuild merged commit 2780cc2 into cms-sw:master Mar 10, 2017

mariadalfonso mentioned this pull request Mar 10, 2017

HBHE M2 = fix Memory Leak (84X) #17885

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HBHE M2 = fix Memory Leak #17821

HBHE M2 = fix Memory Leak #17821

mariadalfonso commented Mar 8, 2017 •

edited

cmsbuild commented Mar 8, 2017

perrotta Mar 8, 2017

igv4321 commented Mar 8, 2017

mariadalfonso commented Mar 8, 2017

cmsbuild commented Mar 8, 2017

igv4321 commented Mar 8, 2017

slava77 commented Mar 8, 2017

cmsbuild commented Mar 8, 2017 •

edited

Dr15Jones Mar 8, 2017

cmsbuild commented Mar 8, 2017

cmsbuild commented Mar 8, 2017

cmsbuild commented Mar 8, 2017

Dr15Jones Mar 9, 2017

Dr15Jones commented Mar 9, 2017

cmsbuild commented Mar 9, 2017

cmsbuild commented Mar 9, 2017

cmsbuild commented Mar 9, 2017

perrotta commented Mar 9, 2017

igv4321 commented Mar 9, 2017

slava77 commented Mar 9, 2017

igv4321 commented Mar 9, 2017

perrotta commented Mar 9, 2017

kpedro88 commented Mar 9, 2017

Dr15Jones commented Mar 9, 2017

slava77 commented Mar 9, 2017 via email

perrotta commented Mar 10, 2017

cmsbuild commented Mar 10, 2017

davidlange6 commented Mar 10, 2017


		// initialize for every different channel types (HPD vs SiPM)

		if (!(&ps == currentPulseShape_ && isHPD == isCurrentChannelHPD_))

HBHE M2 = fix Memory Leak #17821

HBHE M2 = fix Memory Leak #17821

Conversation

mariadalfonso commented Mar 8, 2017 • edited

cmsbuild commented Mar 8, 2017

perrotta Mar 8, 2017

Choose a reason for hiding this comment

igv4321 commented Mar 8, 2017

mariadalfonso commented Mar 8, 2017

cmsbuild commented Mar 8, 2017

igv4321 commented Mar 8, 2017

slava77 commented Mar 8, 2017

cmsbuild commented Mar 8, 2017 • edited

Dr15Jones Mar 8, 2017

Choose a reason for hiding this comment

cmsbuild commented Mar 8, 2017

cmsbuild commented Mar 8, 2017

cmsbuild commented Mar 8, 2017

Dr15Jones Mar 9, 2017

Choose a reason for hiding this comment

Dr15Jones commented Mar 9, 2017

cmsbuild commented Mar 9, 2017

cmsbuild commented Mar 9, 2017

cmsbuild commented Mar 9, 2017

perrotta commented Mar 9, 2017

igv4321 commented Mar 9, 2017

slava77 commented Mar 9, 2017

igv4321 commented Mar 9, 2017

perrotta commented Mar 9, 2017

kpedro88 commented Mar 9, 2017

Dr15Jones commented Mar 9, 2017

slava77 commented Mar 9, 2017 via email

perrotta commented Mar 10, 2017

cmsbuild commented Mar 10, 2017

davidlange6 commented Mar 10, 2017

mariadalfonso commented Mar 8, 2017 •

edited

cmsbuild commented Mar 8, 2017 •

edited