From ba1de47b2ee954cd9e615f67603cd8e3996d1f87 Mon Sep 17 00:00:00 2001 From: Nathan Brei Date: Tue, 21 Nov 2023 11:54:48 -0500 Subject: [PATCH 1/8] Fix PODIO perf test --- src/programs/perf_tests/PerfTests.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/programs/perf_tests/PerfTests.cc b/src/programs/perf_tests/PerfTests.cc index 47037a3d9..b1f9f12a7 100644 --- a/src/programs/perf_tests/PerfTests.cc +++ b/src/programs/perf_tests/PerfTests.cc @@ -98,7 +98,8 @@ int main() { auto logger = app.GetService()->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 From c725d4e907ac7323ec05b14c82328edd07ff9c3f Mon Sep 17 00:00:00 2001 From: Nathan Brei Date: Tue, 21 Nov 2023 12:52:55 -0500 Subject: [PATCH 2/8] Fix PODIO unit test --- src/programs/unit_tests/PodioTests.cc | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/programs/unit_tests/PodioTests.cc b/src/programs/unit_tests/PodioTests.cc index c176c8868..2081433c6 100644 --- a/src/programs/unit_tests/PodioTests.cc +++ b/src/programs/unit_tests/PodioTests.cc @@ -202,6 +202,15 @@ struct MyWrapper { } }; +#if podio_VERSION < PODIO_VERSION(0, 17, 0) +template +struct MyWrapper::collection_t>> { + int x = 2; + bool have_podio() { + return true; + } +}; +#else template struct MyWrapper> { int x = 2; @@ -209,8 +218,10 @@ struct MyWrapper> { return true; } }; +#endif TEST_CASE("SFINAE for JFactoryT || JFactoryPodioT") { + MyWrapper w; REQUIRE(w.have_podio() == false); @@ -218,13 +229,19 @@ TEST_CASE("SFINAE for JFactoryT || JFactoryPodioT") { REQUIRE(ww.have_podio() == true); ww.x = 22; + } template struct is_podio : std::false_type {}; +#if podio_VERSION < PODIO_VERSION(0, 17, 0) +template +struct is_podio::collection_t>> : std::true_type {}; +#else template struct is_podio> : std::true_type {}; +#endif template static constexpr bool is_podio_v = is_podio::value; From 2a49b305c0652a95e469a97c46f7a01f6bbbf0c1 Mon Sep 17 00:00:00 2001 From: Nathan Brei Date: Tue, 21 Nov 2023 13:08:59 -0500 Subject: [PATCH 3/8] JMultifactory no longer suffixes output collection names This feature never worked right, and the functionality has been moved to a higher level. See EICrecon's JOmnifactory. --- src/libraries/JANA/JMultifactory.h | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/libraries/JANA/JMultifactory.h b/src/libraries/JANA/JMultifactory.h index 920953097..a3fbe6c0d 100644 --- a/src/libraries/JANA/JMultifactory.h +++ b/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 void JMultifactory::DeclareOutput(std::string tag, bool owns_data) { JFactory* helper = new JMultifactoryHelper(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(this); if (!owns_data) helper->SetSubsetCollection(true); - tag += mTagSuffix; helper->SetTag(std::move(tag)); helper->SetPluginName(mPluginName); helper->SetFactoryName(mFactoryName); From 20a6276f6962d3809df7fc1a8e35a7bae4a80b95 Mon Sep 17 00:00:00 2001 From: Nathan Brei Date: Tue, 21 Nov 2023 13:56:09 -0500 Subject: [PATCH 4/8] Verify correct behavior of JFactoryPodioT::Insert --- src/programs/unit_tests/PodioTests.cc | 33 +++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/src/programs/unit_tests/PodioTests.cc b/src/programs/unit_tests/PodioTests.cc index 2081433c6..a1de8bde6 100644 --- a/src/programs/unit_tests/PodioTests.cc +++ b/src/programs/unit_tests/PodioTests.cc @@ -332,4 +332,37 @@ TEST_CASE("JFactoryPodioT::Init gets called") { REQUIRE(fac->init_called == true); } +namespace jana2_tests_podiotests_insert { + +struct TestFac : public JFactoryPodioT { + TestFac() { + SetTag("clusters"); + } + void Process(const std::shared_ptr& event) override { + Insert(new ExampleCluster(16.0)); + } +}; +} + +TEST_CASE("JFactoryPodioT::Insert() and retrieval") { + + JApplication app; + auto event = std::make_shared(&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 (Goes through mData) + auto vcp = event->Get("clusters"); + REQUIRE(vcp.size() == 1); + REQUIRE(vcp[0]->energy() == 16.0); + + // Retrieve as ExampleClusterCollection (Goes through Frame) + auto r = event->GetCollection("clusters"); + REQUIRE(r != nullptr); + REQUIRE(r->size() == 1); + REQUIRE((*r)[0].energy() == 16.0); +} + } // namespace podiotests From 5ae2275238bcca1d65760dd678bdc14506db723b Mon Sep 17 00:00:00 2001 From: Nathan Brei Date: Tue, 21 Nov 2023 14:01:44 -0500 Subject: [PATCH 5/8] JMultifactory catches std::exception and rethrows as a JException --- src/libraries/JANA/JMultifactory.cc | 49 +++++++++++++++++++++++++---- 1 file changed, 43 insertions(+), 6 deletions(-) diff --git a/src/libraries/JANA/JMultifactory.cc b/src/libraries/JANA/JMultifactory.cc index 745663fc5..2a215a834 100644 --- a/src/libraries/JANA/JMultifactory.cc +++ b/src/libraries/JANA/JMultifactory.cc @@ -16,23 +16,60 @@ void JMultifactory::Execute(const std::shared_ptr& 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() { From a229aa7f13febbc591ba4b907365a329102caf77 Mon Sep 17 00:00:00 2001 From: Nathan Brei Date: Tue, 21 Nov 2023 14:15:18 -0500 Subject: [PATCH 6/8] JFactoryGenerators now have a pointer to JApplication This means we can get rid of the extra 'app' argument when declaring OmniFactories and ChainFactories --- src/libraries/JANA/JFactoryGenerator.h | 11 +++++++++++ src/libraries/JANA/Services/JComponentManager.cc | 7 ++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/libraries/JANA/JFactoryGenerator.h b/src/libraries/JANA/JFactoryGenerator.h index c5fb9c267..739465681 100644 --- a/src/libraries/JANA/JFactoryGenerator.h +++ b/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 diff --git a/src/libraries/JANA/Services/JComponentManager.cc b/src/libraries/JANA/Services/JComponentManager.cc index ea8ab15d7..e57fdacb3 100644 --- a/src/libraries/JANA/Services/JComponentManager.cc +++ b/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); From 160bcdb0afcb1b335351c5f36b417c3b6acbdd28 Mon Sep 17 00:00:00 2001 From: Nathan Brei Date: Tue, 21 Nov 2023 14:22:13 -0500 Subject: [PATCH 7/8] Fix memory leak in JFactoryPodioT::Set This resolves issue #235. --- src/libraries/JANA/Podio/JFactoryPodioT.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/libraries/JANA/Podio/JFactoryPodioT.h b/src/libraries/JANA/Podio/JFactoryPodioT.h index 279c0b709..c762e844f 100644 --- a/src/libraries/JANA/Podio/JFactoryPodioT.h +++ b/src/libraries/JANA/Podio/JFactoryPodioT.h @@ -193,6 +193,7 @@ void JFactoryPodioT::Set(const std::vector& 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::Set(std::vector&& 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::Insert(T* aDatum) { CollectionT collection; if (mIsSubsetCollection) collection->setSubsetCollection(true); collection->push_back(*aDatum); + delete aDatum; SetCollection(std::move(collection)); } From 47f7f41b899db58cc1f3bae0ec585c8810c08f76 Mon Sep 17 00:00:00 2001 From: Nathan Brei Date: Tue, 21 Nov 2023 14:29:18 -0500 Subject: [PATCH 8/8] Fix C++20 warning about implicit capture of 'this' --- src/libraries/JANA/Services/JPluginLoader.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/JANA/Services/JPluginLoader.cc b/src/libraries/JANA/Services/JPluginLoader.cc index 3c36e971c..f37997293 100644 --- a/src/libraries/JANA/Services/JPluginLoader.cc +++ b/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 &plugins) { + auto add_plugins_lamda = [=, this](std::vector &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".