From e95e823b67c0a06ef18bda4e1e27b6e15fb5b85d Mon Sep 17 00:00:00 2001 From: Maximilian Goblirsch-Kolb Date: Wed, 26 Jan 2022 16:15:33 +0100 Subject: [PATCH 1/4] push next attempt --- roofit/roofit/src/RooLagrangianMorphFunc.cxx | 145 +++++++++---------- 1 file changed, 70 insertions(+), 75 deletions(-) diff --git a/roofit/roofit/src/RooLagrangianMorphFunc.cxx b/roofit/roofit/src/RooLagrangianMorphFunc.cxx index 1b9bccce3112c..49f30764a26dd 100644 --- a/roofit/roofit/src/RooLagrangianMorphFunc.cxx +++ b/roofit/roofit/src/RooLagrangianMorphFunc.cxx @@ -448,27 +448,67 @@ void readValues(std::map &myMap, TH1 *h_pc) } } -/////////////////////////////////////////////////////////////////////////////// -/// retrieve a param_hist from a certain subfolder 'name' of the file -inline TH1F *getParamHist(TDirectory *file, const std::string &name, const std::string &objkey = "param_card", - bool notFoundError = true) -{ - auto f_tmp = file->Get(name.c_str()); - if (!f_tmp) { - std::cerr << "unable to retrieve folder '" << name << "' from file '" << file->GetName() << "'!" << std::endl; +/// Reading file-resident TFolders results in memory leakage, addressing this +/// by assigning ownership to the loaded folder over its content +/// and then deleting +/// @param folder TFolder to clean up +/// @param deleteIt if set, enable deletion of the folder after +/// setting ownership +void cleanUpFolder(TFolder* folder, bool deleteIt=true){ + // start by assigning ownership to the folder itself + folder->SetOwner(); + // And we need to do the same for all nested sub-folders. + auto subdirs = folder->GetListOfFolders(); + for (auto* subdir : *subdirs){ + auto thisfolder = dynamic_cast(subdir); + if (thisfolder){ + // recursively clean up the subfolders - no explicit deletion here, + // will be handled by owning parent + cleanUpFolder(thisfolder, false); + } + } + // now ownership is set up - can delete if requested. + if (deleteIt) delete folder; +} + +/// Helper to load an object from a file-resident TFolder, while +/// avoiding memory leaks. +/// @tparam objType Type of object to load. +/// @param inFile input file to load from. Expected to be a valid pointer +/// @param folderName Name of the TFolder to load from the file +/// @param objName Name of the object to load +/// @param notFoundError If set, print a detailed error if we didn't find something +/// @return Returns a pointer to a clone of the loaded object. Ownership assigned to the caller. +template objType* loadFromFileResidentFolder(TDirectory* inFile, const std::string & folderName, const std::string & objName, + bool notFoundError = true){ + auto folder = inFile->Get(folderName.c_str()); + if (!folder) { + std::cerr << "Error: unable to access data from folder '" << folderName << "'!" << std::endl; return nullptr; } - // retrieve the histogram param_card which should live directly in the folder - TH1F *h_pc = dynamic_cast(f_tmp->FindObject(objkey.c_str())); - if (h_pc) { - return h_pc; - } - if (notFoundError) { - std::cerr << "unable to retrieve " << objkey << " histogram from folder '" << name << "'" << std::endl; + objType *loadedObject = dynamic_cast(folder->FindObject(objName.c_str())); + if (!loadedObject) { + if (notFoundError){ + std::stringstream errstr; + errstr << "Error: unable to retrieve object '" << objName << "' from folder '" << folderName + << "'. contents are:"; + TIter next(folder->GetListOfFolders()->begin()); + TFolder *f; + while ((f = (TFolder *)next())) { + errstr << " " << f->GetName(); + } + std::cerr << errstr.str() << std::endl; + } + cleanUpFolder(folder); + return nullptr; } - return nullptr; -} + // replace the loaded object by a clone to be able to clean the loaded folder + objType* output = static_cast(loadedObject->Clone()); // can use a static_cast - confirmed validity by initial cast above. + cleanUpFolder(folder); + return output; +} + /////////////////////////////////////////////////////////////////////////////// /// retrieve a ParamSet from a certain subfolder 'name' of the file @@ -477,7 +517,7 @@ template void readValues(std::map &myMap, TDirectory *file, const std::string &name, const std::string &key = "param_card", bool notFoundError = true) { - TH1F *h_pc = getParamHist(file, name, key, notFoundError); + TH1F *h_pc = loadFromFileResidentFolder(file, name, key, notFoundError); readValues(myMap, h_pc); } @@ -752,24 +792,8 @@ void collectHistograms(const char *name, TDirectory *file, std::mapGet(sample.c_str()); - if (!folder) { - std::cerr << "Error: unable to access data from folder '" << sample << "'!" << std::endl; - continue; - } - TH1 *hist = dynamic_cast(folder->FindObject(varname.c_str())); - if (!hist) { - std::stringstream errstr; - errstr << "Error: unable to retrieve histogram '" << varname << "' from folder '" << sample - << "'. contents are:"; - TIter next(folder->GetListOfFolders()->begin()); - TFolder *f; - while ((f = (TFolder *)next())) { - errstr << " " << f->GetName(); - } - std::cerr << errstr.str() << std::endl; - return; - } + TH1* hist = loadFromFileResidentFolder(file, sample, varname, true); + if (!hist) return; auto it = list_hf.find(sample); if (it != list_hf.end()) { @@ -820,25 +844,8 @@ void collectRooAbsReal(const char * /*name*/, TDirectory *file, std::mapGet(sample.c_str()); - if (!folder) { - std::cerr << "Error: unable to access data from folder '" << sample << "'!" << std::endl; - continue; - } - - RooAbsReal *obj = dynamic_cast(folder->FindObject(varname.c_str())); - if (!obj) { - std::stringstream errstr; - std::cerr << "Error: unable to retrieve RooAbsArg '" << varname << "' from folder '" << sample - << "'. contents are:" << std::endl; - TIter next(folder->GetListOfFolders()->begin()); - TFolder *f; - while ((f = (TFolder *)next())) { - errstr << " " << f->GetName(); - } - std::cerr << errstr.str() << std::endl; - return; - } + RooAbsReal* obj = loadFromFileResidentFolder(file,sample,varname,true); + if (!obj) return; auto it = list_hf.find(sample); if (it == list_hf.end()) { int idx = physics.getSize(); @@ -859,12 +866,7 @@ void collectCrosssections(const char *name, TDirectory *file, std::mapGet(sample.c_str()); - if (!folder) { - std::cerr << "unable to access data from folder '" << sample << "'!" << std::endl; - return; - } - TObject *obj = folder->FindObject(varname.c_str()); + TObject* obj = loadFromFileResidentFolder(file, sample, varname,false); TParameter *xsection = nullptr; TParameter *error = nullptr; TParameter *p = dynamic_cast *>(obj); @@ -878,14 +880,7 @@ void collectCrosssections(const char *name, TDirectory *file, std::mapGetListOfFolders()->begin()); - TFolder *f; - while ((f = (TFolder *)next())) { - errstr << " " << f->GetName(); - } - std::cerr << errstr.str() << std::endl; + errstr << "Error: unable to retrieve cross section '" << varname << "' from folder '" << sample; return; } @@ -916,8 +911,8 @@ void collectCrosssectionsTPair(const char *name, TDirectory *file, std::mapGet(basefolder.c_str()); - TPair *pair = dynamic_cast(folder->FindObject(varname.c_str())); + TPair* pair = loadFromFileResidentFolder(file,basefolder,varname,false); + if (!pair) return; TParameter *xsec_double = dynamic_cast *>(pair->Key()); if (xsec_double) { collectCrosssections(name, file, list_xs, physics, varname, inputParameters); @@ -1755,10 +1750,9 @@ void RooLagrangianMorphFunc::collectInputs(TDirectory *file) cxcoutP(InputArguments) << "initializing physics inputs from file " << file->GetName() << " with object name(s) '" << obsName << "'" << std::endl; auto folderNames = this->_config.folderNames; - auto base = file->Get(folderNames[0].c_str()); - TObject *obj = base->FindObject(obsName.c_str()); + TObject* obj = loadFromFileResidentFolder(file, folderNames.front(), obsName, true); if (!obj) { - std::cerr << "unable to locate object '" << obsName << "' in folder '" << base << "'!" << std::endl; + std::cerr << "unable to locate object '" << obsName << "' in folder '" << folderNames.front() << "'!" << std::endl; return; } std::string classname = obj->ClassName(); @@ -1807,6 +1801,7 @@ void RooLagrangianMorphFunc::addFolders(const RooArgList &folders) if (!f) continue; std::string name(f->GetName()); + cleanUpFolder(f); if (name.empty()) continue; this->_config.folderNames.push_back(name); @@ -2557,7 +2552,7 @@ void RooLagrangianMorphFunc::setParameters(const char *foldername) { std::string filename = this->_config.fileName; TDirectory *file = openFile(filename.c_str()); - TH1 *paramhist = getParamHist(file, foldername); + TH1 *paramhist = loadFromFileResidentFolder(file, foldername,"param_card"); setParams(paramhist, this->_operators, false); closeFile(file); } From 0e26524e96df8e2652476685d673ea28e910aac7 Mon Sep 17 00:00:00 2001 From: Maximilian Goblirsch-Kolb Date: Wed, 26 Jan 2022 17:58:29 +0100 Subject: [PATCH 2/4] doc --- roofit/roofit/src/RooLagrangianMorphFunc.cxx | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/roofit/roofit/src/RooLagrangianMorphFunc.cxx b/roofit/roofit/src/RooLagrangianMorphFunc.cxx index 49f30764a26dd..d0e9a3aeaa131 100644 --- a/roofit/roofit/src/RooLagrangianMorphFunc.cxx +++ b/roofit/roofit/src/RooLagrangianMorphFunc.cxx @@ -448,10 +448,10 @@ void readValues(std::map &myMap, TH1 *h_pc) } } - -/// Reading file-resident TFolders results in memory leakage, addressing this -/// by assigning ownership to the loaded folder over its content -/// and then deleting +/////////////////////////////////////////////////////////////////////////////// +/// Set up a TFolder loaded from a file to own its content. +/// Also recursively updates nested subfolders accordingly +/// Optionally, delete the TFolder at the end /// @param folder TFolder to clean up /// @param deleteIt if set, enable deletion of the folder after /// setting ownership @@ -463,8 +463,7 @@ void cleanUpFolder(TFolder* folder, bool deleteIt=true){ for (auto* subdir : *subdirs){ auto thisfolder = dynamic_cast(subdir); if (thisfolder){ - // recursively clean up the subfolders - no explicit deletion here, - // will be handled by owning parent + // no explicit deletion here, will be handled by parent cleanUpFolder(thisfolder, false); } } @@ -472,7 +471,8 @@ void cleanUpFolder(TFolder* folder, bool deleteIt=true){ if (deleteIt) delete folder; } -/// Helper to load an object from a file-resident TFolder, while +/////////////////////////////////////////////////////////////////////////////// +/// Helper to load a single object from a file-resident TFolder, while /// avoiding memory leaks. /// @tparam objType Type of object to load. /// @param inFile input file to load from. Expected to be a valid pointer From 2b495352222e8e2d22735139639cd6ee43ae18db Mon Sep 17 00:00:00 2001 From: Maximilian Goblirsch-Kolb Date: Wed, 26 Jan 2022 18:03:10 +0100 Subject: [PATCH 3/4] align with coding conventions --- roofit/roofit/src/RooLagrangianMorphFunc.cxx | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/roofit/roofit/src/RooLagrangianMorphFunc.cxx b/roofit/roofit/src/RooLagrangianMorphFunc.cxx index d0e9a3aeaa131..8bfa17f5a3a4a 100644 --- a/roofit/roofit/src/RooLagrangianMorphFunc.cxx +++ b/roofit/roofit/src/RooLagrangianMorphFunc.cxx @@ -455,7 +455,8 @@ void readValues(std::map &myMap, TH1 *h_pc) /// @param folder TFolder to clean up /// @param deleteIt if set, enable deletion of the folder after /// setting ownership -void cleanUpFolder(TFolder* folder, bool deleteIt=true){ +void cleanUpFolder(TFolder* folder, bool deleteIt=true) +{ // start by assigning ownership to the folder itself folder->SetOwner(); // And we need to do the same for all nested sub-folders. @@ -474,20 +475,23 @@ void cleanUpFolder(TFolder* folder, bool deleteIt=true){ /////////////////////////////////////////////////////////////////////////////// /// Helper to load a single object from a file-resident TFolder, while /// avoiding memory leaks. -/// @tparam objType Type of object to load. +/// @tparam AObjType Type of object to load. /// @param inFile input file to load from. Expected to be a valid pointer /// @param folderName Name of the TFolder to load from the file /// @param objName Name of the object to load /// @param notFoundError If set, print a detailed error if we didn't find something /// @return Returns a pointer to a clone of the loaded object. Ownership assigned to the caller. -template objType* loadFromFileResidentFolder(TDirectory* inFile, const std::string & folderName, const std::string & objName, - bool notFoundError = true){ +template AObjType* loadFromFileResidentFolder(TDirectory* inFile, + const std::string & folderName, + const std::string & objName, + bool notFoundError = true) +{ auto folder = inFile->Get(folderName.c_str()); if (!folder) { std::cerr << "Error: unable to access data from folder '" << folderName << "'!" << std::endl; return nullptr; } - objType *loadedObject = dynamic_cast(folder->FindObject(objName.c_str())); + AObjType *loadedObject = dynamic_cast(folder->FindObject(objName.c_str())); if (!loadedObject) { if (notFoundError){ std::stringstream errstr; @@ -504,7 +508,7 @@ template objType* loadFromFileResidentFolder(TDirectory* inFile, return nullptr; } // replace the loaded object by a clone to be able to clean the loaded folder - objType* output = static_cast(loadedObject->Clone()); // can use a static_cast - confirmed validity by initial cast above. + AObjType* output = static_cast(loadedObject->Clone()); // can use a static_cast - confirmed validity by initial cast above. cleanUpFolder(folder); return output; } From b967f878121683dd8920db72233c75f69f835609 Mon Sep 17 00:00:00 2001 From: Maximilian Goblirsch-Kolb Date: Thu, 27 Jan 2022 14:07:06 +0100 Subject: [PATCH 4/4] remove need for a cleanup method --- roofit/roofit/src/RooLagrangianMorphFunc.cxx | 66 +++++++++++--------- 1 file changed, 36 insertions(+), 30 deletions(-) diff --git a/roofit/roofit/src/RooLagrangianMorphFunc.cxx b/roofit/roofit/src/RooLagrangianMorphFunc.cxx index 8bfa17f5a3a4a..257c9836ac104 100644 --- a/roofit/roofit/src/RooLagrangianMorphFunc.cxx +++ b/roofit/roofit/src/RooLagrangianMorphFunc.cxx @@ -449,27 +449,38 @@ void readValues(std::map &myMap, TH1 *h_pc) } /////////////////////////////////////////////////////////////////////////////// -/// Set up a TFolder loaded from a file to own its content. +/// Set up folder ownership over its children, and treat likewise any subfolders. +/// @param theFolder: folder to update. Assumed to be a valid pointer +void setOwnerRecursive(TFolder* theFolder){ + theFolder->SetOwner(); + // And also need to set up ownership for nested folders + auto subdirs = theFolder->GetListOfFolders(); + for (auto* subdir : *subdirs){ + auto thisfolder = dynamic_cast(subdir); + if (thisfolder){ + // no explicit deletion here, will be handled by parent + setOwnerRecursive(thisfolder); + } + } +} + + +/////////////////////////////////////////////////////////////////////////////// +/// Load a TFolder from a file while ensuring it owns its content. +/// This avoids memory leaks. Note that when fetching objects +/// from this folder, you need to clone them to prevent deletion. /// Also recursively updates nested subfolders accordingly -/// Optionally, delete the TFolder at the end -/// @param folder TFolder to clean up -/// @param deleteIt if set, enable deletion of the folder after -/// setting ownership -void cleanUpFolder(TFolder* folder, bool deleteIt=true) -{ - // start by assigning ownership to the folder itself - folder->SetOwner(); - // And we need to do the same for all nested sub-folders. - auto subdirs = folder->GetListOfFolders(); - for (auto* subdir : *subdirs){ - auto thisfolder = dynamic_cast(subdir); - if (thisfolder){ - // no explicit deletion here, will be handled by parent - cleanUpFolder(thisfolder, false); - } - } - // now ownership is set up - can delete if requested. - if (deleteIt) delete folder; +/// @param inFile: Input file to read - assumed to be a valid pointer +/// @param folderName: Name of the folder to read from the file +/// @return a unique_ptr to the folder. Nullptr if not found. +std::unique_ptr readOwningFolderFromFile(TDirectory* inFile, const std::string & folderName){ + std::unique_ptr theFolder(inFile->Get(folderName.c_str())); + if (!theFolder) { + std::cerr << "Error: unable to access data from folder '" << folderName << "' from file '"<GetName()<<"'!" << std::endl; + return nullptr; + } + setOwnerRecursive(theFolder.get()); + return theFolder; } /////////////////////////////////////////////////////////////////////////////// @@ -486,9 +497,8 @@ template AObjType* loadFromFileResidentFolder(TDirectory* inFil const std::string & objName, bool notFoundError = true) { - auto folder = inFile->Get(folderName.c_str()); + auto folder = readOwningFolderFromFile(inFile, folderName); if (!folder) { - std::cerr << "Error: unable to access data from folder '" << folderName << "'!" << std::endl; return nullptr; } AObjType *loadedObject = dynamic_cast(folder->FindObject(objName.c_str())); @@ -504,13 +514,10 @@ template AObjType* loadFromFileResidentFolder(TDirectory* inFil } std::cerr << errstr.str() << std::endl; } - cleanUpFolder(folder); return nullptr; } - // replace the loaded object by a clone to be able to clean the loaded folder - AObjType* output = static_cast(loadedObject->Clone()); // can use a static_cast - confirmed validity by initial cast above. - cleanUpFolder(folder); - return output; + // replace the loaded object by a clone, as the loaded folder will delete the original + return static_cast(loadedObject->Clone()); // can use a static_cast - confirmed validity by initial cast above. } @@ -1801,11 +1808,10 @@ void RooLagrangianMorphFunc::addFolders(const RooArgList &folders) TIter next(file->GetList()); TObject *obj = nullptr; while ((obj = (TObject *)next())) { - auto f = file->Get(obj->GetName()); + auto f = readOwningFolderFromFile(file,obj->GetName()); if (!f) continue; - std::string name(f->GetName()); - cleanUpFolder(f); + std::string name(f->GetName()); if (name.empty()) continue; this->_config.folderNames.push_back(name);