[RF] Refactor ChangeOperModeRAII to work with groups of RooAbsArgs

Several places needed to record a set of operation-mode changes and
restore them later as a group, so it's better to have the
ChangeOperModeRAII act on groups of RooAbsArg to not have to create one
RAII object per arg.

(cherry picked from commit d42a27c)

Author: guitargeek

roofit/roofitcore/inc/RooEvaluatorWrapper.h

@@ -21,8 +21,10 @@
 #include <RooRealProxy.h>
 #include <RooSetProxy.h>
 
+#include <memory>
 #include <stack>
 
+class ChangeOperModeRAII;
 class RooAbsArg;
 class RooAbsCategory;
 class RooAbsPdf;
@@ -73,7 +75,7 @@ class RooEvaluatorWrapper final : public RooAbsReal {
    void setUseGeneratedFunctionCode(bool);
    void writeDebugMacro(std::string const &) const;
 
-   std::stack<std::unique_ptr<ChangeOperModeRAII>> setOperModes(RooAbsArg::OperMode opMode);
+   std::unique_ptr<ChangeOperModeRAII> setOperModes(RooAbsArg::OperMode opMode);
 
 protected:
    double evaluate() const override;

roofit/roofitcore/inc/RooFit/Evaluator.h

@@ -20,7 +20,6 @@
 #include <RConfig.h>
 
 #include <memory>
-#include <stack>
 
 class ChangeOperModeRAII;
 class RooAbsArg;
@@ -45,7 +44,7 @@ class Evaluator {
 
    void setOffsetMode(RooFit::EvalContext::OffsetMode);
 
-   std::stack<std::unique_ptr<ChangeOperModeRAII>> setOperModes(RooAbsArg::OperMode opMode);
+   std::unique_ptr<ChangeOperModeRAII> setOperModes(RooAbsArg::OperMode opMode);
 
 private:
    void processVariable(NodeInfo &nodeInfo);
@@ -68,7 +67,7 @@ class Evaluator {
    RooFit::EvalContext _evalContextCUDA;
    std::vector<NodeInfo> _nodes;                             // the ordered computation graph
    std::unordered_map<TNamed const *, NodeInfo *> _nodesMap; // for quick lookup of nodes
-   std::stack<std::unique_ptr<ChangeOperModeRAII>> _changeOperModeRAIIs;
+   std::unique_ptr<ChangeOperModeRAII> _operModeChanges;
 };
 
 } // end namespace RooFit

roofit/roofitcore/res/RooFitImplHelpers.h

@@ -46,25 +46,51 @@ class DisableCachingRAII {
    bool _oldState;
 };
 
-/// Struct to temporarily change the operation mode of a RooAbsArg until it
-/// goes out of scope.
+/// Scope guard that temporarily changes the operation mode of one or more
+/// RooAbsArg instances. Each call to change() records the arg's current
+/// operMode before flipping it to the requested mode (non-recursively, i.e.
+/// value clients are not touched). Destruction (or an explicit clear())
+/// restores every recorded mode in LIFO order.
+///
+/// The class is movable but not copyable, so it can be returned from
+/// functions that build up a batch of changes to hand to the caller.
 class ChangeOperModeRAII {
 public:
-   ChangeOperModeRAII(RooAbsArg *arg, RooAbsArg::OperMode opMode) : _arg{arg}, _oldOpMode(arg->operMode())
+   ChangeOperModeRAII() = default;
+
+   /// Convenience ctor: behaves like a scope guard for a single arg.
+   ChangeOperModeRAII(RooAbsArg *arg, RooAbsArg::OperMode opMode) { change(arg, opMode); }
+
+   ~ChangeOperModeRAII() { clear(); }
+
+   ChangeOperModeRAII(ChangeOperModeRAII &&) = default;
+   ChangeOperModeRAII &operator=(ChangeOperModeRAII &&) = default;
+   ChangeOperModeRAII(ChangeOperModeRAII const &) = delete;
+   ChangeOperModeRAII &operator=(ChangeOperModeRAII const &) = delete;
+
+   /// Record arg's current operMode and flip it to opMode. If the current
+   /// mode already equals opMode, this is a no-op (nothing to restore).
+   void change(RooAbsArg *arg, RooAbsArg::OperMode opMode)
    {
-      arg->setOperMode(opMode, /*recurse=*/false);
+      if (opMode == arg->operMode())
+         return;
+      _entries.emplace_back(arg, arg->operMode());
+      arg->setOperMode(opMode, /*recurseADirty=*/false);
    }
 
-   ChangeOperModeRAII(ChangeOperModeRAII const &other) = delete;
-   ChangeOperModeRAII &operator=(ChangeOperModeRAII const &other) = delete;
-   ChangeOperModeRAII(ChangeOperModeRAII &&other) = delete;
-   ChangeOperModeRAII &operator=(ChangeOperModeRAII &&other) = delete;
+   /// Restore every recorded change right away, emptying this guard.
+   void clear()
+   {
+      for (auto it = _entries.rbegin(); it != _entries.rend(); ++it) {
+         it->first->setOperMode(it->second, /*recurseADirty=*/false);
+      }
+      _entries.clear();
+   }
 
-   ~ChangeOperModeRAII() { _arg->setOperMode(_oldOpMode, /*recurse=*/false); }
+   bool empty() const { return _entries.empty(); }
 
 private:
-   RooAbsArg *_arg = nullptr;
-   RooAbsArg::OperMode _oldOpMode;
+   std::vector<std::pair<RooAbsArg *, RooAbsArg::OperMode>> _entries;
 };
 
 namespace RooHelpers {

roofit/roofitcore/src/RooEvaluatorWrapper.cxx

@@ -741,7 +741,7 @@ void RooEvaluatorWrapper::writeDebugMacro(std::string const &filename) const
       return _funcWrapper->writeDebugMacro(filename);
 }
 
-std::stack<std::unique_ptr<ChangeOperModeRAII>> RooEvaluatorWrapper::setOperModes(RooAbsArg::OperMode opMode)
+std::unique_ptr<ChangeOperModeRAII> RooEvaluatorWrapper::setOperModes(RooAbsArg::OperMode opMode)
 {
    return _evaluator->setOperModes(opMode);
 }

roofit/roofitcore/src/RooFit/Evaluator.cxx

@@ -628,9 +628,9 @@ void Evaluator::markGPUNodes()
 /// Evaluator gets deleted.
 void Evaluator::setOperMode(RooAbsArg *arg, RooAbsArg::OperMode opMode)
 {
-   if (opMode != arg->operMode()) {
-      _changeOperModeRAIIs.emplace(std::make_unique<ChangeOperModeRAII>(arg, opMode));
-   }
+   if (!_operModeChanges)
+      _operModeChanges = std::make_unique<ChangeOperModeRAII>();
+   _operModeChanges->change(arg, opMode);
 }
 
 // Change the operation modes of all RooAbsArgs in the computation graph.
@@ -646,9 +646,9 @@ void Evaluator::setOperMode(RooAbsArg *arg, RooAbsArg::OperMode opMode)
 // like a RooRealVar) would be left permanently in ADirty after the first
 // minimization, dramatically slowing down later scalar evaluations (for
 // example on pdfs held by the legacy test statistics' internal cache).
-std::stack<std::unique_ptr<ChangeOperModeRAII>> Evaluator::setOperModes(RooAbsArg::OperMode opMode)
+std::unique_ptr<ChangeOperModeRAII> Evaluator::setOperModes(RooAbsArg::OperMode opMode)
 {
-   std::stack<std::unique_ptr<ChangeOperModeRAII>> out{};
+   auto out = std::make_unique<ChangeOperModeRAII>();
    std::unordered_set<RooAbsArg *> visited;
 
    std::vector<RooAbsArg *> queue;
@@ -663,9 +663,7 @@ std::stack<std::unique_ptr<ChangeOperModeRAII>> Evaluator::setOperModes(RooAbsAr
       if (!visited.insert(node).second)
          continue;
 
-      if (opMode != node->operMode()) {
-         out.emplace(std::make_unique<ChangeOperModeRAII>(node, opMode));
-      }
+      out->change(node, opMode);
 
       // Only follow value-client links: that is exactly the propagation path
       // used by RooAbsArg::setOperMode with mode==ADirty.

roofit/roofitcore/src/RooMinimizer.cxx

@@ -131,7 +131,7 @@ void reorderCombinations(std::vector<std::vector<int>> &combos, const std::vecto
 //
 // This should be called before running any routine via the _minimizer data
 // member. The RAII object should only be destructed after the routine is done.
-std::stack<std::unique_ptr<ChangeOperModeRAII>> setOperModesDirty(RooAbsReal &function)
+std::unique_ptr<ChangeOperModeRAII> setOperModesDirty(RooAbsReal &function)
 {
    if (auto *wrapper = dynamic_cast<RooFit::Experimental::RooEvaluatorWrapper *>(&function)) {
       return wrapper->setOperModes(RooAbsArg::ADirty);

roofit/roofitcore/src/RooRealIntegral.cxx

@@ -841,12 +841,12 @@ double RooRealIntegral::evaluate() const
         // from caching subgraph results (e.g. for nested numeric integrals).
         RooArgList serverList;
         _function->treeNodeServerList(&serverList, nullptr, true, true, false, true);
-        std::list<ChangeOperModeRAII> operModeRAII;
+        ChangeOperModeRAII operModeRAII;
 
         for (auto *arg : serverList) {
            arg->syncCache();
            if (arg->operMode() == RooAbsArg::AClean) {
-              operModeRAII.emplace_back(arg, RooAbsArg::Auto);
+              operModeRAII.change(arg, RooAbsArg::Auto);
            }
         }