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] Address memory leaks in RooLagrangianMorphFunc #9717

Merged

Conversation

goblirsc
Copy link
Contributor

This Pull request:

The RooLagrangianMorphFunc loads its input objects from a nested set of TFolders stored in a TFile.
This MR adds protection (inspired by ROOT-9275 to avoid memory leaks in the initialisation of the RooLagrangianMorphFunc, which for large RooFit workspaces with many morphing functions can otherwise become unfeasible to use.

Changes or fixes:

  • add a cleanUpFolder helper method to an existing anoymous namespace to RooLagrangianMorphFunc.cxx, which ensures ownership is enabled for TFolders loaded from TFiles and optionally deletes the folders after setting them up
  • add a templated loadFromFileResidentFolder method to the same anonymous namespace in RooLagrangianMorphFunc.cxx, refactoring the procedure of loading an object from a nested TFolder structure that was previously performed in various locations in the class. The method exploits the cleanUpFolder method to prevent memory leaks in the access procedure.

Checklist:

  • [ x ] tested changes locally - confirmed leak rate massively reduced in local testing. Draft test script (not yet merged) provided by @rahulgrit retains same output as before the changes
  • updated the docs (if necessary) - no changes to user-accessible code. Inline doxygen documentation of new methods.

Tagging @rahulgrit @guitargeek

Sorry, something went wrong.

@phsft-bot
Copy link

Can one of the admins verify this patch?

@goblirsc goblirsc changed the title [RF] Address memory leaks in RooLagrangianMorphPdf [RF] Address memory leaks in RooLagrangianMorphFunc Jan 26, 2022
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.

Hi @goblirsc, thanks a ton for spotting the memory leaks and opening a PR to fix them!

I have a few suggestions to improve your fix. There are two anti-patterns in your fix:

  • manual memory management with delete instead of using smart pointer
  • relying on a "cleanup" function that needs to be called at the end of an objects lifetime. This is dangerous, because if one adds new code branches with if-else, one might forget to call the cleanup function and create a leak.

It would be better to do all of this at the initialization of the folder object (see RAII on Wikipedia). What about introducing these functions instead of cleanUpFolder?

TFolder * flagOwningFolder(TFolder* folder)
{                                                                                       
  // start by assigning ownership to the folder itself
  folder->SetOwner();
  // And we need to do the same for all nested sub-folders.
  for (auto* subdir : *folder->GetListOfFolders()){
    if (auto thisfolder = dynamic_cast<TFolder*>(subdir)){
      // no explicit deletion here, will be handled by parent
      flagOwningFolder(thisfolder);      
    }                                    
  }       
  return folder;
}
      
std::unique_ptr<TFolder> getOwningFolder(TDirectory * inFile, std::string const& folderName) {
   return std::unique_ptr<TFolder>{flagOwningFolder(inFile->Get<TFolder>(folderName.c_str()))};

And then in the code, you can just use it like auto folder = getOwningFolder(inFile, folderName) and you don't need to remember calling any cleanup function: the SetOwner recursion is done in the initialization, and delete is called by the std::unique_ptr that owns the parent folder.

Would this work for you?

@guitargeek
Copy link
Contributor

@phsft-bot build

@phsft-bot
Copy link

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

@goblirsc
Copy link
Contributor Author

Hi @goblirsc, thanks a ton for spotting the memory leaks and opening a PR to fix them!

I have a few suggestions to improve your fix. There are two anti-patterns in your fix:

  • manual memory management with delete instead of using smart pointer
  • relying on a "cleanup" function that needs to be called at the end of an objects lifetime. This is dangerous, because if one adds new code branches with if-else, one might forget to call the cleanup function and create a leak.

It would be better to do all of this at the initialization of the folder object (see RAII on Wikipedia). What about introducing these functions instead of cleanUpFolder?

TFolder * flagOwningFolder(TFolder* folder)
{                                                                                       
  // start by assigning ownership to the folder itself
  folder->SetOwner();
  // And we need to do the same for all nested sub-folders.
  for (auto* subdir : *folder->GetListOfFolders()){
    if (auto thisfolder = dynamic_cast<TFolder*>(subdir)){
      // no explicit deletion here, will be handled by parent
      flagOwningFolder(thisfolder);      
    }                                    
  }       
  return folder;
}
      
std::unique_ptr<TFolder> getOwningFolder(TDirectory * inFile, std::string const& folderName) {
   return std::unique_ptr<TFolder>{flagOwningFolder(inFile->Get<TFolder>(folderName.c_str()))};

And then in the code, you can just use it like auto folder = getOwningFolder(inFile, folderName) and you don't need to remember calling any cleanup function: the SetOwner recursion is done in the initialization, and delete is called by the std::unique_ptr that owns the parent folder.

Would this work for you?

Hi @guitargeek,

thanks a lot, I agree this is a much nicer approach! Will update the PR in a little bit.

Cheers, Max

@guitargeek
Copy link
Contributor

Great, thanks for the prompt update!

@guitargeek
Copy link
Contributor

@phsft-bot build

@phsft-bot
Copy link

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

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, thanks a lot again for the fixes and the quick follow-up to the review!

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

3 participants