diff --git a/roofit/roofit/test/testRooMomentMorphFuncND.cxx b/roofit/roofit/test/testRooMomentMorphFuncND.cxx index 8033df2170dd9..963e1e84e943f 100644 --- a/roofit/roofit/test/testRooMomentMorphFuncND.cxx +++ b/roofit/roofit/test/testRooMomentMorphFuncND.cxx @@ -3,6 +3,7 @@ #include #include +#include #include #include #include @@ -15,27 +16,17 @@ #include +#include #include namespace { -// For each morph configuration we expect: a normal `getVal(normSet)` on the -// uncompiled pdf and an Evaluator run on the same pdf compiled for that -// normSet must agree. Whether the morph is in pdf mode (sum is a RooAddPdf -// whose components must be normalized for the value to be a proper density) -// or in func mode (sum is a RooRealSumFunc, the value is the raw weighted -// sum of bin contents) is internal to compileForNormSet -- both paths must -// produce the same final value. -void checkBackendConsistency(bool pdfMode) +/// Workspace with the observable "x", the morphing parameter "mu", and the +/// morph pdf "morph" with two RooHistPdf templates. +std::unique_ptr makeMorphWorkspace(bool pdfMode) { - // FATAL silences a transient logEvalError that the morph emits from its - // slope/offset RooFormulaVars at cache-construction time (in func mode the - // initial rms-fractions are zero, so slope = inf for one self-evaluation - // before calculateFractions runs). It does not affect correctness. - // TODO: its not elegant that this happens and should be fixed. - RooHelpers::LocalChangeMsgLevel changeMsgLevel{RooFit::FATAL}; - - RooWorkspace ws; + auto wsPtr = std::make_unique(); + RooWorkspace &ws = *wsPtr; ws.factory("x[-5,15]"); ws.factory("mu[0.5,0.0,1.0]"); RooRealVar &x = *ws.var("x"); @@ -63,7 +54,30 @@ void checkBackendConsistency(bool pdfMode) RooWrapperPdf morph("morph", "morph", morph_func, /*selfNormalized=*/true); ws.import(morph, RooFit::Silence(true)); - RooAbsPdf &morphPdf = *ws.pdf("morph"); + return wsPtr; +} + +// For each morph configuration we expect: a normal `getVal(normSet)` on the +// uncompiled pdf and an Evaluator run on the same pdf compiled for that +// normSet must agree. Whether the morph is in pdf mode (sum is a RooAddPdf +// whose components must be normalized for the value to be a proper density) +// or in func mode (sum is a RooRealSumFunc, the value is the raw weighted +// sum of bin contents) is internal to compileForNormSet -- both paths must +// produce the same final value. +void checkBackendConsistency(bool pdfMode) +{ + // FATAL silences a transient logEvalError that the morph emits from its + // slope/offset RooFormulaVars at cache-construction time (in func mode the + // initial rms-fractions are zero, so slope = inf for one self-evaluation + // before calculateFractions runs). It does not affect correctness. + // TODO: its not elegant that this happens and should be fixed. + RooHelpers::LocalChangeMsgLevel changeMsgLevel{RooFit::FATAL}; + + std::unique_ptr ws = makeMorphWorkspace(pdfMode); + RooRealVar &x = *ws->var("x"); + RooRealVar &mu = *ws->var("mu"); + + RooAbsPdf &morphPdf = *ws->pdf("morph"); RooArgSet normSet(x); mu.setVal(0.5); @@ -89,3 +103,23 @@ TEST(RooMomentMorphFuncND, BackendConsistencyFuncMode) { checkBackendConsistency(/*pdfMode=*/false); } + +/// The compiled computation graph of the morph pdf references the original +/// morph function and the RooHistPdf templates owned by the workspace. If the +/// workspace is deleted before an NLL object, the destructor of the NLL must +/// not touch the deleted nodes anymore. This happened at the exit of the +/// rf616_morphing.py tutorial, where the Python garbage collector deletes the +/// workspace first, and was observed as a `free(): invalid pointer` abort. +TEST(RooMomentMorphFuncND, WorkspaceDeletedBeforeNLL) +{ + RooHelpers::LocalChangeMsgLevel changeMsgLevel{RooFit::FATAL}; + + std::unique_ptr ws = makeMorphWorkspace(/*pdfMode=*/true); + RooAbsPdf &morphPdf = *ws->pdf("morph"); + std::unique_ptr data{ws->pdf("hp0")->generate(*ws->var("x"), 100)}; + std::unique_ptr nll{morphPdf.createNLL(*data)}; + EXPECT_TRUE(std::isfinite(nll->getVal())); + + ws.reset(); + nll.reset(); +} diff --git a/roofit/roofitcore/inc/RooAbsArg.h b/roofit/roofitcore/inc/RooAbsArg.h index b53f27a8185b6..bdc29c720b296 100644 --- a/roofit/roofitcore/inc/RooAbsArg.h +++ b/roofit/roofitcore/inc/RooAbsArg.h @@ -52,6 +52,7 @@ using RooListProxy = RooCollectionProxy; class RooExpensiveObjectCache; class RooWorkspace; namespace RooFit { +class Evaluator; namespace Experimental { class CodegenContext; } @@ -612,8 +613,12 @@ class RooAbsArg : public TNamed, public RooPrintable { /// Returns the token for retrieving results in the BatchMode. For internal use only. std::size_t dataToken() const { return _dataToken; } bool hasDataToken() const { return _dataToken != std::numeric_limits::max(); } - void setDataToken(std::size_t index); - void resetDataToken() { _dataToken = std::numeric_limits::max(); } + void setDataToken(std::size_t index, RooFit::Evaluator *evaluator = nullptr); + void resetDataToken() + { + _dataToken = std::numeric_limits::max(); + _evaluator = nullptr; + } protected: mutable bool _valueDirty = true; // Flag set if value needs recalculating because input values modified @@ -638,6 +643,7 @@ class RooAbsArg : public TNamed, public RooPrintable { std::size_t _dataToken = std::numeric_limits::max(); /// setOperModes(RooAbsArg::OperMode opMode); + void unregisterNode(RooAbsArg const &arg); + private: void processVariable(NodeInfo &nodeInfo); void processCategory(NodeInfo &nodeInfo); diff --git a/roofit/roofitcore/src/RooAbsArg.cxx b/roofit/roofitcore/src/RooAbsArg.cxx index 4f790ee178d7f..7fd7484a10bc5 100644 --- a/roofit/roofitcore/src/RooAbsArg.cxx +++ b/roofit/roofitcore/src/RooAbsArg.cxx @@ -74,6 +74,7 @@ for single nodes. #include #include #include +#include #include #include "RooFitImplHelpers.h" #include @@ -168,6 +169,12 @@ RooAbsArg::RooAbsArg(const RooAbsArg &other, const char *name) RooAbsArg::~RooAbsArg() { + // If a RooFit::Evaluator still holds a data token for this arg, tell it that + // the arg is gone so that it doesn't reset the token in its destructor. + if (_evaluator) { + _evaluator->unregisterNode(*this); + } + // Notify all servers that they no longer need to serve us while (!_serverList.empty()) { removeServer(*_serverList.containedObjects().back(), true); @@ -2164,17 +2171,22 @@ RooAbsArg::compileForNormSet(RooArgSet const &normSet, RooFit::Detail::CompileCo } /// Sets the token for retrieving results in the BatchMode. For internal use only. -void RooAbsArg::setDataToken(std::size_t index) -{ - if (_dataToken == index) { - return; +/// If an `evaluator` is passed, it is the RooFit::Evaluator that resets the +/// token in its destructor, and this arg will notify it if it gets deleted +/// before the Evaluator. +void RooAbsArg::setDataToken(std::size_t index, RooFit::Evaluator *evaluator) +{ + if (_dataToken != index) { + if (hasDataToken()) { + std::stringstream errMsg; + errMsg << "The data token for \"" << GetName() << "\" is already set!" + << " Are you trying to evaluate the same object by multiple RooFit::Evaluator instances?" + << " This is not allowed."; + throw std::runtime_error(errMsg.str()); + } + _dataToken = index; } - if (_dataToken != std::numeric_limits::max()) { - std::stringstream errMsg; - errMsg << "The data token for \"" << GetName() << "\" is already set!" - << " Are you trying to evaluate the same object by multiple RooFit::Evaluator instances?" - << " This is not allowed."; - throw std::runtime_error(errMsg.str()); + if (evaluator) { + _evaluator = evaluator; } - _dataToken = index; } diff --git a/roofit/roofitcore/src/RooFit/Evaluator.cxx b/roofit/roofitcore/src/RooFit/Evaluator.cxx index 6a64e94aa9016..ccd423fe188e0 100644 --- a/roofit/roofitcore/src/RooFit/Evaluator.cxx +++ b/roofit/roofitcore/src/RooFit/Evaluator.cxx @@ -190,7 +190,7 @@ Evaluator::Evaluator(const RooAbsReal &absReal, bool useGPU) if (dynamic_cast(arg)) { nodeInfo.isVariable = true; } else { - arg->setDataToken(iNode); + arg->setDataToken(iNode, this); } if (dynamic_cast(arg)) { nodeInfo.isCategory = true; @@ -351,7 +351,8 @@ void Evaluator::updateOutputSizes() Evaluator::~Evaluator() { for (auto &info : _nodes) { - if (!info.isVariable) { + // Nodes that were deleted before the Evaluator were unregistered + if (!info.isVariable && info.absArg) { info.absArg->resetDataToken(); } } @@ -360,6 +361,18 @@ Evaluator::~Evaluator() } } +/// Called from RooAbsArg::~RooAbsArg() when a node of the computation graph is +/// deleted while this Evaluator is still alive. The Evaluator can't be used +/// anymore after that, but its destructor must not touch the deleted node. +/// For internal use only. +void Evaluator::unregisterNode(RooAbsArg const &arg) +{ + auto found = _nodesMap.find(arg.namePtr()); + if (found != _nodesMap.end() && found->second->absArg == &arg) { + found->second->absArg = nullptr; + } +} + void Evaluator::computeCPUNode(const RooAbsArg *node, NodeInfo &info) { using namespace Detail; diff --git a/roofit/roofitcore/test/CMakeLists.txt b/roofit/roofitcore/test/CMakeLists.txt index 21204155cf6e8..0a1268a4d05aa 100644 --- a/roofit/roofitcore/test/CMakeLists.txt +++ b/roofit/roofitcore/test/CMakeLists.txt @@ -84,6 +84,7 @@ endif() ROOT_ADD_GTEST(testNaNPacker testNaNPacker.cxx LIBRARIES RooFitCore) ROOT_ADD_GTEST(testRooBatchCompute testRooBatchCompute.cxx LIBRARIES RooBatchCompute) ROOT_ADD_GTEST(testRooEfficiency testRooEfficiency.cxx LIBRARIES RooFitCore RooFit) +ROOT_ADD_GTEST(testRooEvaluator testRooEvaluator.cxx LIBRARIES RooFitCore RooFit) ROOT_ADD_GTEST(testRooExtendedBinding testRooExtendedBinding.cxx LIBRARIES RooFitCore RooFit) ROOT_ADD_GTEST(testRooMCStudy testRooMCStudy.cxx LIBRARIES RooFitCore RooFit) ROOT_ADD_GTEST(testRooMinimizer testRooMinimizer.cxx LIBRARIES RooFitCore RooFit) diff --git a/roofit/roofitcore/test/testRooEvaluator.cxx b/roofit/roofitcore/test/testRooEvaluator.cxx new file mode 100644 index 0000000000000..84fa3ff904209 --- /dev/null +++ b/roofit/roofitcore/test/testRooEvaluator.cxx @@ -0,0 +1,43 @@ +// Tests for RooFit::Evaluator +// Author: Jonas Rembser, CERN 10/2026 + +#include +#include +#include + +#include + +#include + +/// Deleting nodes of the computation graph before the Evaluator must not +/// crash: the Evaluator used to reset the data tokens of its nodes in the +/// destructor without checking whether the nodes still exist (use after free +/// that was observed as a `free(): invalid pointer` abort at the exit of +/// Python scripts, where the garbage collector can delete the RooWorkspace +/// owning the model before the NLL object). +TEST(RooEvaluator, NodesDeletedBeforeEvaluator) +{ + auto x = std::make_unique("x", "x", 0.0, -5.0, 5.0); + auto mu = std::make_unique("mu", "mu", 0.0, -1.0, 1.0); + auto sigma = std::make_unique("sigma", "sigma", 1.0, 0.1, 10.0); + auto gauss = std::make_unique("gauss", "gauss", *x, *mu, *sigma); + + auto evaluator = std::make_unique(*gauss); + EXPECT_FLOAT_EQ(evaluator->run()[0], gauss->getVal()); + + // Delete the pdf before the Evaluator that holds a data token for it. + gauss.reset(); + evaluator.reset(); + + // A new model with the same node name must get a fresh data token. + auto gauss2 = std::make_unique("gauss", "gauss", *x, *mu, *sigma); + { + RooFit::Evaluator evaluator2{*gauss2}; + EXPECT_FLOAT_EQ(evaluator2.run()[0], gauss2->getVal()); + } + + // The other order: the node outlives the Evaluator. Its data token must + // have been reset such that it can be used by a new Evaluator. + RooFit::Evaluator evaluator3{*gauss2}; + EXPECT_FLOAT_EQ(evaluator3.run()[0], gauss2->getVal()); +} diff --git a/tutorials/roofit/roofit/rf619_discrete_profiling.py b/tutorials/roofit/roofit/rf619_discrete_profiling.py index 1f7c3a5738e55..9a68a351e641e 100644 --- a/tutorials/roofit/roofit/rf619_discrete_profiling.py +++ b/tutorials/roofit/roofit/rf619_discrete_profiling.py @@ -166,11 +166,6 @@ minim.minimize("Minuit2", "Migrad") profileGraph.SetPoint(i, meanVal, nll.getVal()) -# The fit objects are not needed anymore. Delete them already before the -# plotting phase, so that the NLL evaluation machinery is not torn down at -# interpreter shutdown, where the order of cleanups is less controlled. -del minim -del nll c = ROOT.TCanvas("c_rf619", "NLL vs Mean for Different Discrete Combinations", 1200, 400) c.Divide(3, 1) diff --git a/tutorials/roofit/roostats/rs401d_FeldmanCousins.py b/tutorials/roofit/roostats/rs401d_FeldmanCousins.py index 9db04dacfebe5..55a21e81a9b6f 100644 --- a/tutorials/roofit/roostats/rs401d_FeldmanCousins.py +++ b/tutorials/roofit/roostats/rs401d_FeldmanCousins.py @@ -164,12 +164,6 @@ def rs401d_FeldmanCousins(doFeldmanCousins=False, doMCMC=True): dataCanvas.Draw() dataCanvas.SaveAs("3.png") - # The profile likelihood and the NLL are not needed anymore. Delete them - # already here, so that the NLL evaluation machinery is not torn down at - # interpreter shutdown, where the order of cleanups is less controlled. - del pll - del nll - # -------------------------------------------------------------- # show use of Feldman-Cousins utility in RooStats # set the distribution creator, which encodes the test statistic