Merge pull request #266 from JeffersonLab/nbrei_eicrecon_fixes

Bugfix: Rough edges in JFactoryPodioT and JMultifactory

src/libraries/JANA/JFactoryGenerator.h

@@ -9,9 +9,12 @@
 #ifndef _JFactoryGenerator_h_
 #define _JFactoryGenerator_h_
 
+class JApplication;
+
 class JFactoryGenerator {
 
     std::string m_plugin_name;
+    JApplication* m_app;
 
 public:
 
@@ -26,6 +29,14 @@ class JFactoryGenerator {
     inline void SetPluginName(std::string plugin_name) {
         m_plugin_name = std::move(plugin_name);
     }
+
+    inline void SetApplication(JApplication* app) {
+        m_app = app;
+    }
+
+    inline JApplication* GetApplication() {
+        return m_app;
+    }
 };
 
 /// JFactoryGeneratorT works for both JFactories and JMultifactories

src/libraries/JANA/JMultifactory.cc

@@ -16,23 +16,60 @@ void JMultifactory::Execute(const std::shared_ptr<const JEvent>& event) {
 #endif
 
     auto run_number = event->GetRunNumber();
-    std::call_once(m_is_initialized, &JMultifactory::Init, this);
+    try {
+        std::call_once(m_is_initialized, &JMultifactory::Init, this);
+    }
+    catch(std::exception &e) {
+        // Rethrow as a JException so that we can add context information
+        throw JException(e.what());
+    }
+
     if (m_last_run_number == -1) {
         // This is the very first run
-        BeginRun(event);
+        try {
+            BeginRun(event);
+        }
+        catch(std::exception &e) {
+            // Rethrow as a JException so that we can add context information
+            throw JException(e.what());
+        }
         m_last_run_number = run_number;
     }
     else if (m_last_run_number != run_number) {
         // This is a later run, and it has changed
-        EndRun();
-        BeginRun(event);
+        try {
+            EndRun();
+        }
+        catch(std::exception &e) {
+            // Rethrow as a JException so that we can add context information
+            throw JException(e.what());
+        }
+        try {
+            BeginRun(event);
+        }
+        catch(std::exception &e) {
+            // Rethrow as a JException so that we can add context information
+            throw JException(e.what());
+        }
         m_last_run_number = run_number;
     }
-    Process(event);
+    try {
+        Process(event);
+    }
+    catch(std::exception &e) {
+        // Rethrow as a JException so that we can add context information
+        throw JException(e.what());
+    }
 }
 
 void JMultifactory::Release() {
-    std::call_once(m_is_finished, &JMultifactory::Finish, this);
+    try {
+        std::call_once(m_is_finished, &JMultifactory::Finish, this);
+    }
+    catch(std::exception &e) {
+        // Rethrow as a JException so that we can add context information
+        throw JException(e.what());
+    }
 }
 
 JFactorySet* JMultifactory::GetHelpers() {

src/libraries/JANA/JMultifactory.h

@@ -68,8 +68,9 @@ class JMultifactory {
     // However, don't worry about a Status variable. Every time Execute() gets called, so does Process().
     // The JMultifactoryHelpers will control calls to Execute().
 
-    std::string mTagSuffix;  // In order to have multiple (differently configured) instances in the same factorySet
-    std::string mPluginName; // So we can propagate this to the JMultifactoryHelpers, so we can have useful error messages
+    std::string mTag;         // JMultifactories each get their own name
+                              // This can be used for parameter and collection name prefixing, though at a higher level
+    std::string mPluginName;  // So we can propagate this to the JMultifactoryHelpers, so we can have useful error messages
     std::string mFactoryName; // So we can propagate this to the JMultifactoryHelpers, so we can have useful error messages
     JApplication* mApp;
 
@@ -131,7 +132,7 @@ class JMultifactory {
     JApplication* GetApplication() { return mApp; }
 
     // These are set by JFactoryGeneratorT (just like JFactories) and get propagated to each of the JMultifactoryHelpers
-    void SetTag(std::string tagSuffix) { mTagSuffix = std::move(tagSuffix); }
+    void SetTag(std::string tag) { mTag = std::move(tag); }
     void SetFactoryName(std::string factoryName) { mFactoryName = std::move(factoryName); }
     void SetPluginName(std::string pluginName) { mPluginName = std::move(pluginName); }
 };
@@ -142,7 +143,6 @@ template <typename T>
 void JMultifactory::DeclareOutput(std::string tag, bool owns_data) {
     JFactory* helper = new JMultifactoryHelper<T>(this);
     if (!owns_data) helper->SetFactoryFlag(JFactory::JFactory_Flags_t::NOT_OBJECT_OWNER);
-    tag += mTagSuffix;
     helper->SetPluginName(mPluginName);
     helper->SetFactoryName(mFactoryName);
     helper->SetTag(std::move(tag));
@@ -174,7 +174,6 @@ void JMultifactory::DeclarePodioOutput(std::string tag, bool owns_data) {
     auto* helper = new JMultifactoryHelperPodio<T>(this);
     if (!owns_data) helper->SetSubsetCollection(true);
 
-    tag += mTagSuffix;
     helper->SetTag(std::move(tag));
     helper->SetPluginName(mPluginName);
     helper->SetFactoryName(mFactoryName);

src/libraries/JANA/Podio/JFactoryPodioT.h

@@ -193,6 +193,7 @@ void JFactoryPodioT<T>::Set(const std::vector<T*>& aData) {
     if (mIsSubsetCollection) collection.setSubsetCollection(true);
     for (T* item : aData) {
         collection.push_back(*item);
+        delete item;
     }
     SetCollection(std::move(collection));
 }
@@ -203,6 +204,7 @@ void JFactoryPodioT<T>::Set(std::vector<T*>&& aData) {
     if (mIsSubsetCollection) collection.setSubsetCollection(true);
     for (T* item : aData) {
         collection.push_back(*item);
+        delete item;
     }
     SetCollection(std::move(collection));
 }
@@ -212,6 +214,7 @@ void JFactoryPodioT<T>::Insert(T* aDatum) {
     CollectionT collection;
     if (mIsSubsetCollection) collection->setSubsetCollection(true);
     collection->push_back(*aDatum);
+    delete aDatum;
     SetCollection(std::move(collection));
 }
 

src/libraries/JANA/Services/JComponentManager.cc

@@ -43,6 +43,7 @@ void JComponentManager::add(JEventSourceGenerator *source_generator) {
 
 void JComponentManager::add(JFactoryGenerator *factory_generator) {
     factory_generator->SetPluginName(m_current_plugin_name);
+    factory_generator->SetApplication(m_app);
     m_fac_gens.push_back(factory_generator);
 }
 
@@ -52,7 +53,9 @@ void JComponentManager::add(JEventSource *event_source) {
     m_evt_srces.push_back(event_source);
     auto fac_gen = event_source->GetFactoryGenerator();
     if (fac_gen != nullptr) {
-        m_fac_gens.push_back(event_source->GetFactoryGenerator());
+        fac_gen->SetPluginName(m_current_plugin_name);
+        fac_gen->SetApplication(m_app);
+        m_fac_gens.push_back(fac_gen);
     }
 }
 
@@ -94,6 +97,8 @@ void JComponentManager::resolve_event_sources() {
         source->SetApplication(m_app);
         auto fac_gen = source->GetFactoryGenerator();
         if (fac_gen != nullptr) {
+            fac_gen->SetPluginName(m_current_plugin_name);
+            fac_gen->SetApplication(m_app);
             m_fac_gens.push_back(fac_gen);
         }
         m_evt_srces.push_back(source);

src/libraries/JANA/Services/JPluginLoader.cc

@@ -100,7 +100,7 @@ void JPluginLoader::attach_plugins(JComponentManager* jcm) {
     // be attached. To accommodate this we wrap the following chunk of code in
     // a lambda function so we can run it over the additional plugins recursively
     // until all are attached. (see below)
-    auto add_plugins_lamda = [=](std::vector<std::string> &plugins) {
+    auto add_plugins_lamda = [=, this](std::vector<std::string> &plugins) {
         std::stringstream paths_checked;
         for (const std::string& plugin : plugins) {
             // The user might provide a short name like "JTest", or a long name like "JTest.so".

src/programs/perf_tests/PerfTests.cc

@@ -98,7 +98,8 @@ int main() {
         auto logger = app.GetService<JLoggingService>()->get_logger("PerfTests");
         // TODO: Add Podio sources, processors, and factories just like JTest
         LOG_INFO(logger) << "Running PODIO stress test" << LOG_END;
-        benchmark(app);
+        JBenchmarker benchmarker(&app);
+        benchmarker.RunUntilFinished();
     }
 #endif
 

src/programs/unit_tests/PodioTests.cc

@@ -202,29 +202,46 @@ struct MyWrapper {
     }
 };
 
+#if podio_VERSION < PODIO_VERSION(0, 17, 0)
+template <typename T>
+struct MyWrapper<T, std::void_t<typename PodioTypeMap<T>::collection_t>> {
+    int x = 2;
+    bool have_podio() {
+        return true;
+    }
+};
+#else
 template <typename T>
 struct MyWrapper<T, std::void_t<typename T::collection_type>> {
     int x = 2;
     bool have_podio() {
         return true;
     }
 };
+#endif
 
 TEST_CASE("SFINAE for JFactoryT || JFactoryPodioT") {
+
     MyWrapper<int> w;
     REQUIRE(w.have_podio() == false);
 
     MyWrapper<ExampleCluster> ww;
     REQUIRE(ww.have_podio() == true);
 
     ww.x = 22;
+
 }
 
 template <typename, typename=void>
 struct is_podio : std::false_type {};
 
+#if podio_VERSION < PODIO_VERSION(0, 17, 0)
+template <typename T>
+struct is_podio<T, std::void_t<typename PodioTypeMap<T>::collection_t>> : std::true_type {};
+#else
 template <typename T>
 struct is_podio<T, std::void_t<typename T::collection_type>> : std::true_type {};
+#endif
 
 template <typename T>
 static constexpr bool is_podio_v = is_podio<T>::value;
@@ -315,4 +332,37 @@ TEST_CASE("JFactoryPodioT::Init gets called") {
     REQUIRE(fac->init_called == true);
 }
 
+namespace jana2_tests_podiotests_insert {
+
+struct TestFac : public JFactoryPodioT<ExampleCluster> {
+    TestFac() {
+        SetTag("clusters");
+    }
+    void Process(const std::shared_ptr<const JEvent>& event) override {
+        Insert(new ExampleCluster(16.0));
+    }
+};
+}
+
+TEST_CASE("JFactoryPodioT::Insert() and retrieval") {
+
+    JApplication app;
+    auto event = std::make_shared<JEvent>(&app);
+    auto fs = new JFactorySet;
+    fs->Add(new jana2_tests_podiotests_insert::TestFac);
+    event->SetFactorySet(fs);
+    event->GetFactorySet()->Release();  // Simulate a trip to the JEventPool
+
+    // Retrieve as vector<ExampleCluster*> (Goes through mData)
+    auto vcp = event->Get<ExampleCluster>("clusters");
+    REQUIRE(vcp.size() == 1);
+    REQUIRE(vcp[0]->energy() == 16.0);
+
+    // Retrieve as ExampleClusterCollection (Goes through Frame)
+    auto r = event->GetCollection<ExampleCluster>("clusters");
+    REQUIRE(r != nullptr);
+    REQUIRE(r->size() == 1);
+    REQUIRE((*r)[0].energy() == 16.0);
+}
+
 } // namespace podiotests