Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid freeing memory owned by TMVA::Reader #17266

Merged
merged 3 commits into from Feb 3, 2017

Conversation

dan131riley
Copy link

Older versions of TMVA::Reader didn't clean up classification methods, and so leaked memory. Some CMS code cleaned up the memory itself to avoid leaking. Starting with ROOT 6.07+, TMVA::Reader now cleans up after itself, so the CMS cleanups are resulting in double frees and potentially corrupted memory. This (rather ugly) PR patches all the places I could easily find to not assume ownership of the objects. More details are in issue #17256

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @dan131riley (Dan Riley) for CMSSW_9_0_X.

It involves the following packages:

EgammaAnalysis/ElectronTools
PhysicsTools/MVAComputer
RecoEgamma/ElectronIdentification
RecoEgamma/PhotonIdentification
RecoJets/JetProducers
RecoParticleFlow/PFProducer
RecoParticleFlow/PFTracking

@cmsbuild, @cvuosalo, @slava77, @monttj, @davidlange6 can you please review it and eventually sign? Thanks.
@rappoccio, @yslai, @TaiSakuma, @ahinzmann, @mmarionncern, @imarches, @makortel, @acaudron, @lgray, @jdolen, @ferencek, @Sam-Harper, @rafaellopesdesa, @nhanvtran, @gkasieczka, @schoef, @JyothsnaKomaragiri, @mverzett, @cbernet, @mariadalfonso, @pvmulder, @bachtis this is something you requested to watch as well.
@davidlange6, @smuzaffar you are the release manager for this.

cms-bot commands are listed here #13028

@slava77
Copy link
Contributor

slava77 commented Jan 24, 2017

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 24, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/17417/console Started: 2017/01/24 22:38

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Jan 27, 2017

+1

for #17266 3ca8416

  • implementation is in line with the description
  • jenkins tests pass and comparisons with baseline show no differences
    • we started using ROOT 6.08/02 since about 10 days in 90X IBs; so, the new code branches are executed and appear OK

@slava77
Copy link
Contributor

slava77 commented Jan 31, 2017

@davidlange6
please clarify if some extra work is needed here or are you just waiting for the missing analysis signature?
Thanks

@davidlange6
Copy link
Contributor

davidlange6 commented Jan 31, 2017 via email

@monttj
Copy link
Contributor

monttj commented Jan 31, 2017

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 2, 2017

Pull request #17266 was updated. @cmsbuild, @cvuosalo, @slava77, @monttj, @davidlange6 can you please check and sign again.

@Dr15Jones
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 2, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/17590/console Started: 2017/02/02 17:14

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 2, 2017

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 2, 2017

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 2, 2017

@slava77
Copy link
Contributor

slava77 commented Feb 3, 2017

+1

for #17266 91be322

  • sign again after the support for ROOT version earlier than 6.07 was removed
  • jenkins tests pass and comparisons show no differences

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants