[RF][HS3] Allow registering JSONIO exporter by class name

When instantiating an Exporter like this:

```
STATIC_EXECUTE([]() {
  using namespace RooFit::JSONIO;
  
  registerImporter<MYCLASSFactory>("key", false);
  registerExporter<MYCLASSStreamer>(MYCLASS::Class(), false);
 });
```

it is possible that this will segfault, as `MYCLASS` might not be sufficiently loaded yet to permit `MYCLASS::Class()` to be called.

In order to circumvent this problem, a new method has been added:

With `registerExporter<MYCLASSStreamer>("MYCLASS", false);`, it is now possible to add an exporter in a "delayed" fashion, where it will only be actually added to the exporter list when to RooJSONFactoryWSTool is invoked - before that, it will linger in a temprorary "Loader" helper object.

Author: cburgard

roofit/hs3/inc/RooFitHS3/JSONIO.h

@@ -76,7 +76,6 @@ using ExportMap = std::map<TClass const *, std::vector<std::unique_ptr<const Exp
 using ExportKeysMap = std::map<TClass const *, ExportKeys>;
 using ImportExpressionMap = std::map<const std::string, ImportExpression>;
 
-void setupKeys();
 ImportMap &importers();
 ExportMap &exporters();
 ImportExpressionMap &importExpressions();
@@ -92,9 +91,15 @@ static bool registerExporter(const TClass *key, bool topPriority = true)
 {
    return registerExporter(key, std::make_unique<T>(), topPriority);
 }
+template <class T>
+static bool registerExporter(const std::string &key, bool topPriority = true)
+{
+   return registerExporter(key, std::make_unique<T>(), topPriority);
+}
 
 bool registerImporter(const std::string &key, std::unique_ptr<const Importer> f, bool topPriority = true);
 bool registerExporter(const TClass *key, std::unique_ptr<const Exporter> f, bool topPriority = true);
+bool registerExporter(const std::string &key, std::unique_ptr<const Exporter> f, bool topPriority = true);
 int removeImporters(const std::string &needle);
 int removeExporters(const std::string &needle);
 void printImporters();

roofit/hs3/src/JSONIO.cxx

@@ -18,18 +18,18 @@
 
 #include <TClass.h>
 
-#include <iostream>
 #include <fstream>
+#include <iostream>
+#include <mutex>
 #include <sstream>
 
 // Include raw strings with initial export and import keys in JSON
 #include "RooFitHS3_wsexportkeys.cxx"
 #include "RooFitHS3_wsfactoryexpressions.cxx"
 
-namespace RooFit {
-namespace JSONIO {
+namespace RooFit::JSONIO {
 
-void setupKeys()
+void setupExportKeys()
 {
    static bool isAlreadySetup = false;
    if (isAlreadySetup) {
@@ -38,16 +38,23 @@ void setupKeys()
 
    isAlreadySetup = true;
 
-   {
-      std::stringstream exportkeys;
-      exportkeys << RooFitHS3_wsexportkeys;
-      loadExportKeys(exportkeys);
-   }
-   {
-      std::stringstream factoryexpressions;
-      factoryexpressions << RooFitHS3_wsfactoryexpressions;
-      loadFactoryExpressions(factoryexpressions);
+   std::stringstream exportkeys;
+   exportkeys << RooFitHS3_wsexportkeys;
+   loadExportKeys(exportkeys);
+}
+
+void setupFactoryExpressions()
+{
+   static bool isAlreadySetup = false;
+   if (isAlreadySetup) {
+      return;
    }
+
+   isAlreadySetup = true;
+
+   std::stringstream factoryexpressions;
+   factoryexpressions << RooFitHS3_wsfactoryexpressions;
+   loadFactoryExpressions(factoryexpressions);
 }
 
 ImportMap &importers()
@@ -56,22 +63,59 @@ ImportMap &importers()
    return _importers;
 }
 
-ExportMap &exporters()
+namespace {
+
+auto &exportersToAdd()
+{
+   static std::map<const std::string, std::vector<std::unique_ptr<const Exporter>>> toAdd;
+   return toAdd;
+}
+
+ExportMap &exportersImpl()
 {
    static ExportMap _exporters;
    return _exporters;
 }
 
+} // namespace
+
+ExportMap &exporters()
+{
+   // If there are exporters to be added, do this now.
+   for (auto &item : exportersToAdd()) {
+      TClass *klass = TClass::GetClass(item.first.c_str());
+      auto &exporters = exportersImpl()[klass]; // registered exporters so far
+      auto &toAdd = item.second;                // exporters to add
+
+      // Find the nullptr separator
+      auto nullIt = std::find(toAdd.begin(), toAdd.end(), nullptr);
+
+      if (nullIt == toAdd.end()) {
+         throw std::runtime_error("toAdd does not contain nullptr separator");
+      }
+
+      // Move elements after nullptr to the back
+      exporters.insert(exporters.end(), std::make_move_iterator(nullIt + 1), std::make_move_iterator(toAdd.end()));
+
+      // Move elements before nullptr to the front
+      exporters.insert(exporters.begin(), std::make_move_iterator(toAdd.begin()), std::make_move_iterator(nullIt));
+
+      toAdd.clear();
+   }
+   exportersToAdd().clear();
+   return exportersImpl();
+}
+
 ImportExpressionMap &importExpressions()
 {
-   setupKeys();
+   setupFactoryExpressions();
    static ImportExpressionMap _factoryExpressions;
    return _factoryExpressions;
 }
 
 ExportKeysMap &exportKeys()
 {
-   setupKeys();
+   setupExportKeys();
    static ExportKeysMap _exportKeys;
    return _exportKeys;
 }
@@ -90,6 +134,19 @@ bool registerExporter(const TClass *key, std::unique_ptr<const Exporter> f, bool
    return true;
 }
 
+bool registerExporter(const std::string &key, std::unique_ptr<const Exporter> f, bool topPriority)
+{
+   auto &vec = exportersToAdd()[key];
+   // The vector starts out with just a nullptr separator. Elements before it
+   // will be added to the top of the exporter queue, and the elements after
+   // get appended to the end.
+   if (vec.empty()) {
+      vec.emplace_back(nullptr);
+   }
+   vec.insert(topPriority ? vec.begin() : vec.end(), std::move(f));
+   return true;
+}
+
 int removeImporters(const std::string &needle)
 {
    int n = 0;
@@ -269,5 +326,4 @@ void printExportKeys()
    }
 }
 
-} // namespace JSONIO
-} // namespace RooFit
+} // namespace RooFit::JSONIO

roofit/hs3/test/testRooFitHS3.cxx

@@ -44,8 +44,6 @@ int validate(RooWorkspace &ws1, std::string const &argName, bool exact = true)
 {
    RooWorkspace ws2;
 
-   ws1.Print();
-
    const std::string json1 = RooJSONFactoryWSTool{ws1}.exportJSONtoString();
 
    if (writeJsonFiles) {
@@ -118,8 +116,7 @@ int validate(std::vector<std::string> const &expressions, bool exact = true)
 {
    RooWorkspace ws;
    for (std::size_t iExpr = 0; iExpr < expressions.size() - 1; ++iExpr) {
-      std::cout << expressions[iExpr] << std::endl;
-      ws.factory(expressions[iExpr])->Print();
+      ws.factory(expressions[iExpr]);
    }
    const std::string argName = ws.factory(expressions.back())->GetName();
    return validate(ws, argName, exact);
@@ -433,8 +430,8 @@ TEST(RooFitHS3, SimultaneousGaussians)
 // https://github.com/root-project/root/issues/14637
 TEST(RooFitHS3, ScientificNotation)
 {
-   RooRealVar v1("v1","v1",1.0);
-   RooRealVar v2("v2","v2",1.0);
+   RooRealVar v1("v1", "v1", 1.0);
+   RooRealVar v2("v2", "v2", 1.0);
 
    // make a formula that is some parameters times some numbers
    auto thestring = "@0*0.2e-6 + @1*0.1";
@@ -445,18 +442,19 @@ TEST(RooFitHS3, ScientificNotation)
    RooFormulaVar fvBad("fvBad", "fvBad", thestring, arglist);
 
    // make gaussian with mean as that formula
-   RooRealVar x("x","x",0.0,-5.0,5.0);
-   RooGaussian g("g","g",x, fvBad, 1.0);
+   RooRealVar x("x", "x", 0.0, -5.0, 5.0);
+   RooGaussian g("g", "g", x, fvBad, 1.0);
 
    RooWorkspace ws("ws");
    ws.import(g);
-   //std::cout << (fvBad.expression()) << std::endl;
+   // std::cout << (fvBad.expression()) << std::endl;
 
    // export to json
    RooJSONFactoryWSTool t(ws);
    auto jsonStr = t.exportJSONtoString();
 
-   // try to import, before the fix, it threw RooJSONFactoryWSTool::DependencyMissingError because of problem reading the exponential char
+   // try to import, before the fix, it threw RooJSONFactoryWSTool::DependencyMissingError because of problem reading
+   // the exponential char
    RooWorkspace newws("newws");
    RooJSONFactoryWSTool t2(newws);
    ASSERT_TRUE(t2.importJSONfromString(jsonStr));
@@ -557,3 +555,87 @@ TEST(RooFitHS3, RooSpline)
    const int status = validate(ws, "spline", /*exact=*/true);
    EXPECT_EQ(status, 0);
 }
+
+namespace {
+
+class TestExporterA final : public RooFit::JSONIO::Exporter {
+public:
+   std::string const &key() const override
+   {
+      static const std::string k{"unit_test_exporter_A"};
+      return k;
+   }
+   bool exportObject(RooJSONFactoryWSTool *, const RooAbsArg *, RooFit::Detail::JSONNode &) const override
+   {
+      callCounter()++;
+      return true; // do nothing, just for test
+   }
+   static int &callCounter()
+   {
+      static int counter = 0;
+      return counter;
+   }
+};
+
+template <int N>
+class TestExporter final : public RooFit::JSONIO::Exporter {
+public:
+   std::string const &key() const override
+   {
+      static const std::string k{"unit_test_exporter"};
+      return k;
+   }
+   bool exportObject(RooJSONFactoryWSTool *, const RooAbsArg *, RooFit::Detail::JSONNode &) const override
+   {
+      callCounter()++;
+      return true; // do nothing, just for test
+   }
+   static int &callCounter()
+   {
+      static int counter = 0;
+      return counter;
+   }
+};
+
+} // namespace
+
+// Test the custom exporter registration mechanism.
+TEST(RooFitHS3, RegisterExporterByClassName)
+{
+   using RooFit::JSONIO::registerExporter;
+
+   constexpr const char *className = "RooGaussian";
+   TClass *klass = TClass::GetClass(className);
+   ASSERT_NE(klass, nullptr);
+
+   RooWorkspace ws{"ws"};
+   ws.factory("RooGaussian::model(x[-10, 10], mu[-10, 10], sigma[2., 0.01, 10])");
+
+   // 1. Add new exporter by class pointer with top priority.
+   //    We expect this to get used.
+   registerExporter<TestExporter<1>>(klass, /*topPriotiry=*/true);
+   RooJSONFactoryWSTool{ws}.exportJSONtoString();
+   EXPECT_EQ(TestExporter<1>::callCounter()--, 1);
+
+   // 2. Add new exporter by class pointer with bottom priority.
+   //    We expect the previous TestExporter<1> to still be used.
+   registerExporter<TestExporter<2>>(klass, /*topPriotiry=*/false);
+   RooJSONFactoryWSTool{ws}.exportJSONtoString();
+   EXPECT_EQ(TestExporter<1>::callCounter()--, 1);
+
+   // 3. Add new exporter by name with top priority.
+   //    We expect this to get used.
+   registerExporter<TestExporter<3>>(std::string{className}, /*topPriotiry=*/true);
+   RooJSONFactoryWSTool{ws}.exportJSONtoString();
+   EXPECT_EQ(TestExporter<3>::callCounter()--, 1);
+
+   // 4. Add new exporter by name with bottom priority.
+   //    We expect the previous TestExporter<3> to still be used.
+   registerExporter<TestExporter<4>>(std::string{className}, /*topPriotiry=*/false);
+   RooJSONFactoryWSTool{ws}.exportJSONtoString();
+   EXPECT_EQ(TestExporter<3>::callCounter()--, 1);
+
+   // Cleanup for other tests, also making sure the expected number of
+   // exporters is removed.
+   EXPECT_EQ(RooFit::JSONIO::removeExporters("TestExporter"), 4);
+}