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

[RF] Do not stream RooAbsArg eocache to avoid memory leak when reading back workspaces #12024

Merged
merged 1 commit into from
Jan 13, 2023

Conversation

will-cern
Copy link
Contributor

This memory leak is demonstrated with the following ROOT macro:

{
  {
    RooExpensiveObjectCache::instance(); // force the standard instance construction (otherwise created in factory method call)
    cout << "make ws" << endl;
    RooWorkspace w("combined", "combined");
    cout << "factory method:" << endl;
    w.factory("RooGaussian::gaus(x[-5,5],mean[0,-5,5],sigma[1,0.1,3])");
    w.writeToFile("/tmp/test.root");
    cout << "reading back" << endl;
    {
      TFile f("/tmp/test.root");
      RooWorkspace *w2 = f.Get<RooWorkspace>("combined");
      std::cout << "deleting w2" << endl;
      delete w2;
    }
    std::cout << "deleting w" << endl;
  }
}

along with a modification to RooExpensiveObjectCache to printout when an instance is being constructed or destructed. Before this fix the above then prints out (I annotated the output a bit):

Processing test.C...
Created 0x12cb8cc68 <--- this is the static instance
make ws
Created 0x7ffee2baaab0 <--- the workspace's cache
factory method:
reading back
Created 0x7fcbc7b39008 <--- the read-back workspace's cache
Created 0x7fcbd45a0b70 <--- memory leaking cache
deleting w2
Destroyed 0x7fcbc7b39008
deleting w
Destroyed 0x7ffee2baaab0
root [1] .q
Destroyed 0x12cb8cc68

After the fix caches are created and destroyed as expected:

Processing test.C...
Created 0x1290a5c68
make ws
Created 0x7ffee623eab0
factory method:
reading back
Created 0x7f9bd8437408
deleting w2
Destroyed 0x7f9bd8437408
deleting w
Destroyed 0x7ffee623eab0
root [1] .q
Destroyed 0x1290a5c68

@phsft-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@guitargeek
Copy link
Contributor

@phsft-bot build

@phsft-bot
Copy link
Collaborator

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac12/noimt, mac11/cxx14, windows10/cxx14
How to customize builds

@phsft-bot
Copy link
Collaborator

Build failed on mac12/noimt.
Running on macphsft17.dyndns.cern.ch:/Users/sftnight/build/jenkins/workspace/root-pullrequests-build
See console output.

Warnings:

  • [2023-01-13T12:35:18.342Z] /Users/sftnight/build/jenkins/workspace/root-pullrequests-build/root/core/base/src/TDirectory.cxx:1245:7: warning: 'sprintf' is deprecated: This function is provided for compatibility reasons only. Due to security concerns inherent in the design of sprintf(3), it is highly recommended that you use snprintf(3) instead. [-Wdeprecated-declarations]

Failing tests:

And 63 more

@guitargeek guitargeek changed the title do not stream RooAbsArg eocache to avoid memory leak when reading back workspaces [RF] Do not stream RooAbsArg eocache to avoid memory leak when reading back workspaces Jan 13, 2023
Copy link
Contributor

@guitargeek guitargeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you very much for this important fix!

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.

3 participants