[RF] Forbid calling getVal() with r-value references to norm set

bindings/pyroot/pythonizations/python/ROOT/_pythonization/_roofit/_rooabsreal.py

@@ -116,22 +116,6 @@ def chi2FitTo(self, *args, **kwargs):
         args, kwargs = _kwargs_to_roocmdargs(*args, **kwargs)
         return self._chi2FitTo(*args, **kwargs)
 
-    def getVal(self, normalizationSet=None):
-        # We do the conversion to RooArgSet now, such that we can keep alive
-        # the normalization set by setting it as an attribute of this
-        # RooAbsReal.
-        if isinstance(normalizationSet, (set, list, tuple)):
-            import ROOT
-
-            normalizationSet = ROOT.RooArgSet(normalizationSet)
-        # With the pythonizations, we have the opportunity to use the Python
-        # reference counting to make sure the last normalization set doesn't
-        # get deleted under our feet (RooFit tries to use it by pointer when
-        # you call getVal() without any normalization set the next time).
-        if normalizationSet:
-            self._getVal_normSet = normalizationSet
-        return self._getVal(normalizationSet) if normalizationSet else self._getVal()
-
     def setEvalErrorLoggingMode(m):
 
         import ROOT

roofit/histfactory/test/testHistFactory.cxx

@@ -40,13 +40,15 @@ const bool writeJsonFiles = false;
 
 std::vector<double> getValues(RooAbsReal const &real, RooRealVar &obs, bool normalize, bool useBatchMode)
 {
+   RooArgSet normSet{obs};
+
    std::vector<double> out;
    // We want to evaluate the function at the bin centers
    std::vector<double> binCenters(obs.numBins());
    for (int iBin = 0; iBin < obs.numBins(); ++iBin) {
       obs.setBin(iBin);
       binCenters[iBin] = obs.getVal();
-      out.push_back(normalize ? real.getVal(obs) : real.getVal());
+      out.push_back(normalize ? real.getVal(normSet) : real.getVal());
    }
 
    if (useBatchMode == false) {

roofit/roofit/test/testRooPoisson.cxx

@@ -15,11 +15,13 @@ TEST(RooPoisson, Bare) {
   RooRealVar lambda("lambda", "lambda", 1);
   RooPoisson pois("pois", "pois", x, lambda);
 
+  RooArgSet normSet{x};
+
   auto Poisson = [&](int val, double lambdaVal, double target) {
     x = val;
     lambda = lambdaVal;
 
-    EXPECT_NEAR(pois.getVal(x), target, 5.E-16)
+    EXPECT_NEAR(pois.getVal(normSet), target, 5.E-16)
       << "Test was Pois(" << val << " | " << lambdaVal << ")";
   };
 

roofit/roofitcore/inc/RooAbsReal.h

@@ -131,6 +131,8 @@ class RooAbsReal : public RooAbsArg {
     return _fast ? _value : getValV(normalisationSet.empty() ? nullptr : &normalisationSet) ;
   }
 
+  double getVal(RooArgSet &&) const;
+
   virtual double getValV(const RooArgSet* normalisationSet = nullptr) const ;
 
   double getPropagatedError(const RooFitResult &fr, const RooArgSet &nset = {}) const;

roofit/roofitcore/src/RooAbsReal.cxx

@@ -4475,3 +4475,29 @@ void RooAbsReal::enableOffsetting(bool flag)
 
 
 RooAbsReal::Ref::Ref(double val) : _ref{RooFit::RooConst(val)} {}
+
+////////////////////////////////////////////////////////////////////////////////
+/// Calling RooAbsReal::getVal() with an r-value reference is a common
+/// performance trap, because this usually happens when implicitly constructing
+/// the RooArgSet to be used as the parameter (for example, in calls like
+/// `pdf.getVal(x)`).
+///
+/// Creating the RooArgSet can cause quite some overhead, especially when the
+/// evaluated object is just a simple variable. Even worse, many RooFit objects
+/// internally cache information using the uniqueId() of the normalization set
+/// as the key. So by constructing normalization sets in place, RooFits caching
+/// logic is broken.
+///
+/// To avoid these kind of problems, getVal() will just throw an error when
+/// it's called with an r-value reference. This also catches the cases where
+/// one uses it in Python, implicitly creating the normalization set from a
+/// Python list or set.
+double RooAbsReal::getVal(RooArgSet &&) const
+{
+   std::stringstream errMsg;
+   errMsg << "calling RooAbsReal::getVal() with r-value references to the normalization set is not allowed, because "
+             "it breaks RooFits caching logic and potentially introduces significant overhead. Please explicitly "
+             "create the RooArgSet outside the call to getVal().";
+   coutF(Eval) << errMsg.str() << std::endl;
+   throw std::runtime_error(errMsg.str());
+}

roofit/roofitcore/test/testRooAbsPdf.cxx

@@ -389,8 +389,10 @@ TEST(RooAbsPdf, NormSetChange)
 
    RooAddition add("add", "add", {gauss});
 
+   RooArgSet normSet{x};
+
    double v1 = add.getVal();
-   double v2 = add.getVal(x);
+   double v2 = add.getVal(normSet);
 
    // The change of normalization set should trigger a recomputation of the
    // value, so val2 should be different from val1. }

roofit/roofitcore/test/testRooDataHist.cxx

@@ -740,9 +740,10 @@ TEST_P(WeightsTest, VectorizedWeights)
    }
 
    std::vector<double> weightsGetVal(nVals);
+   RooArgSet normSet{x};
    for (std::size_t i = 0; i < nVals; ++i) {
       x.setVal(xVals[i]);
-      weightsGetVal[i] = absReal->getVal(x);
+      weightsGetVal[i] = absReal->getVal(normSet);
    }
    x.setVal(0.0);
 

roofit/roofitcore/test/testRooProdPdf.cxx

@@ -99,14 +99,23 @@ TEST(RooProdPdf, TestGetPartIntList)
    // Product of all the pdfs.
    auto &prod = static_cast<RooProdPdf &>(*ws.factory("PROD::prod(pdf1, pdf2)"));
 
-   EXPECT_DOUBLE_EQ(prod.getVal({}), 1.0);
-   EXPECT_DOUBLE_EQ(prod.getVal({x}), 1. / a);
-   EXPECT_DOUBLE_EQ(prod.getVal({y}), 1. / b);
-   EXPECT_DOUBLE_EQ(prod.getVal({z}), 1. / c);
-   EXPECT_DOUBLE_EQ(prod.getVal({x, y}), 1. / a / b);
-   EXPECT_DOUBLE_EQ(prod.getVal({x, z}), 1. / a / c);
-   EXPECT_DOUBLE_EQ(prod.getVal({y, z}), 1. / b / c);
-   EXPECT_DOUBLE_EQ(prod.getVal({x, y, z}), 1. / a / b / c);
+   RooArgSet normSetNada{};
+   RooArgSet normSetX{x};
+   RooArgSet normSetY{y};
+   RooArgSet normSetZ{z};
+   RooArgSet normSetXY{x, y};
+   RooArgSet normSetXZ{x, z};
+   RooArgSet normSetYZ{y, z};
+   RooArgSet normSetXYZ{x, y, z};
+
+   EXPECT_DOUBLE_EQ(prod.getVal(normSetNada), 1.0);
+   EXPECT_DOUBLE_EQ(prod.getVal(normSetX), 1. / a);
+   EXPECT_DOUBLE_EQ(prod.getVal(normSetY), 1. / b);
+   EXPECT_DOUBLE_EQ(prod.getVal(normSetZ), 1. / c);
+   EXPECT_DOUBLE_EQ(prod.getVal(normSetXY), 1. / a / b);
+   EXPECT_DOUBLE_EQ(prod.getVal(normSetXZ), 1. / a / c);
+   EXPECT_DOUBLE_EQ(prod.getVal(normSetYZ), 1. / b / c);
+   EXPECT_DOUBLE_EQ(prod.getVal(normSetXYZ), 1. / a / b / c);
 }
 
 TEST(RooProdPdf, TestDepsAreCond)

roofit/roofitcore/test/testRooWrapperPdf.cxx

@@ -29,7 +29,8 @@ TEST(RooWrapperPdf, Basics)
 
   RooWrapperPdf polPdf("polPdf", "polynomial PDF", pol);
 
-  EXPECT_GT(pol.getVal(x)*1.05, polPdf.getVal(x)) << "Wrapper pdf normalises.";
+  RooArgSet normSet{x};
+  EXPECT_GT(pol.getVal(normSet)*1.05, polPdf.getVal(normSet)) << "Wrapper pdf normalises.";
 
   RooArgSet intSet(x);
   RooArgSet numSet;