From bb0b94a72f2d58a82aef5bfb7e6ded9f7ec4c443 Mon Sep 17 00:00:00 2001 From: Nathan Brei Date: Wed, 17 Apr 2024 15:16:20 -0400 Subject: [PATCH 1/4] Improve JAutoActivator One of the use cases for AutoActivator is debugging factories that might be excepting, but the exceptions are being swallowed by the caller. --- src/libraries/JANA/Services/JComponentManager.cc | 5 +++-- src/libraries/JANA/Services/JComponentManager.h | 1 + src/libraries/JANA/Utils/JAutoActivator.cc | 10 ++++------ src/libraries/JANA/Utils/JAutoActivator.h | 1 - 4 files changed, 8 insertions(+), 9 deletions(-) diff --git a/src/libraries/JANA/Services/JComponentManager.cc b/src/libraries/JANA/Services/JComponentManager.cc index d9b34939c..b4c724b7c 100644 --- a/src/libraries/JANA/Services/JComponentManager.cc +++ b/src/libraries/JANA/Services/JComponentManager.cc @@ -36,12 +36,13 @@ void JComponentManager::Init() { m_params->SetDefaultParameter("record_call_stack", m_enable_call_graph_recording, "Records a trace of who called each factory. Reduces performance but necessary for plugins such as janadot."); m_params->SetDefaultParameter("jana:nevents", m_nevents, "Max number of events that sources can emit"); m_params->SetDefaultParameter("jana:nskip", m_nskip, "Number of events that sources should skip before starting emitting"); + m_params->SetDefaultParameter("autoactivate", m_autoactivate, "List of factories to activate regardless of what the event processors request. Format is typename:tag,typename:tag"); m_params->FilterParameters(m_default_tags, "DEFTAG:"); // Look for factories to auto-activate - // Right now AutoActivator parameter won't show up in parameters list. Reconsider this. - if (JAutoActivator::IsRequested(m_params())) { + if (!m_autoactivate.empty()) { add(new JAutoActivator); + // JAutoActivator will re-parse the autoactivate list by itself } } diff --git a/src/libraries/JANA/Services/JComponentManager.h b/src/libraries/JANA/Services/JComponentManager.h index bd42b94ad..ac00f2e48 100644 --- a/src/libraries/JANA/Services/JComponentManager.h +++ b/src/libraries/JANA/Services/JComponentManager.h @@ -65,6 +65,7 @@ class JComponentManager : public JService { std::map m_default_tags; bool m_enable_call_graph_recording = false; + std::string m_autoactivate; uint64_t m_nskip=0; uint64_t m_nevents=0; diff --git a/src/libraries/JANA/Utils/JAutoActivator.cc b/src/libraries/JANA/Utils/JAutoActivator.cc index c4497d2ac..a2d199204 100644 --- a/src/libraries/JANA/Utils/JAutoActivator.cc +++ b/src/libraries/JANA/Utils/JAutoActivator.cc @@ -9,10 +9,6 @@ JAutoActivator::JAutoActivator() { SetTypeName("JAutoActivator"); } -bool JAutoActivator::IsRequested(JParameterManager& params) { - return params.Exists("autoactivate") && (!params.GetParameterValue("autoactivate").empty()); -} - void JAutoActivator::AddAutoActivatedFactory(string factory_name, string factory_tag) { m_auto_activated_factories.push_back({std::move(factory_name), std::move(factory_tag)}); } @@ -67,7 +63,8 @@ void JAutoActivator::Init() { } } catch (...) { - LOG << "Error parsing AUTOACTIVATE=" << autoactivate_conf << LOG_END; + LOG_ERROR(GetLogger()) << "Error parsing parameter 'autoactivate'. Found: " << autoactivate_conf << LOG_END; + throw JException("AutoActivator could not parse parameter 'autoactivate'"); } } } @@ -81,7 +78,8 @@ void JAutoActivator::Process(const std::shared_ptr &event) { factory->Create(event); // This will do nothing if factory is already created } else { - LOG << "Warning: Could not find factory with name=" << name << ", tag=" << tag << LOG_END; + LOG_ERROR(GetLogger()) << "Could not find factory with typename=" << name << ", tag=" << tag << LOG_END; + throw JException("AutoActivator could not find factory with typename=%s, tag=%s", name.c_str(), tag.c_str()); } } } diff --git a/src/libraries/JANA/Utils/JAutoActivator.h b/src/libraries/JANA/Utils/JAutoActivator.h index 695a6d958..35d7eb410 100644 --- a/src/libraries/JANA/Utils/JAutoActivator.h +++ b/src/libraries/JANA/Utils/JAutoActivator.h @@ -16,7 +16,6 @@ class JAutoActivator : public JEventProcessor { public: JAutoActivator(); - static bool IsRequested(JParameterManager& params); static std::pair Split(std::string factory_name); void AddAutoActivatedFactory(string factory_name, string factory_tag); void Init() override; From 8d81c369e9ceb57ea252d1a26f2152e6aa1c5491 Mon Sep 17 00:00:00 2001 From: Nathan Brei Date: Wed, 17 Apr 2024 15:30:50 -0400 Subject: [PATCH 2/4] Reduce the amount of logging devoted to NUMA config --- src/libraries/JANA/Utils/JProcessorMapping.cc | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/libraries/JANA/Utils/JProcessorMapping.cc b/src/libraries/JANA/Utils/JProcessorMapping.cc index b4cf7c044..553e5913b 100644 --- a/src/libraries/JANA/Utils/JProcessorMapping.cc +++ b/src/libraries/JANA/Utils/JProcessorMapping.cc @@ -165,12 +165,11 @@ std::ostream& operator<<(std::ostream& os, const JProcessorMapping::LocalityStra std::ostream& operator<<(std::ostream& os, const JProcessorMapping& m) { - os << "NUMA Configuration" << std::endl; - os << " Affinity strategy: " << m.m_affinity_strategy << std::endl; - os << " Locality strategy: " << m.m_locality_strategy << std::endl; + os << "NUMA Configuration: " << "affinity=" << m.m_affinity_strategy << ", locality=" << m.m_locality_strategy; if (m.m_locality_strategy != JProcessorMapping::LocalityStrategy::Global) { - os << " Location count: " << m.m_loc_count << std::endl; + os << " (" << m.m_loc_count << " locations)"; } + os << std::endl; if (m.m_initialized) { JTablePrinter table; From df65224e5a50b464527d037449751a17d74645ef Mon Sep 17 00:00:00 2001 From: Nathan Brei Date: Wed, 17 Apr 2024 21:37:09 -0400 Subject: [PATCH 3/4] Add test case covering exceptions thrown from JFactory --- src/programs/unit_tests/JFactoryTests.cc | 22 ++++++++++++++++++++++ src/programs/unit_tests/JFactoryTests.h | 16 ++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/src/programs/unit_tests/JFactoryTests.cc b/src/programs/unit_tests/JFactoryTests.cc index 6cb6cba9a..159ee8ae7 100644 --- a/src/programs/unit_tests/JFactoryTests.cc +++ b/src/programs/unit_tests/JFactoryTests.cc @@ -197,5 +197,27 @@ TEST_CASE("JFactoryTests") { REQUIRE((*it)->data == 49); REQUIRE(sut.GetNumObjects() == 1); } + + SECTION("Exception in JFactory::Process") { + LOG << "JFactoryTests: Exception in JFactory::Process" << LOG_END; + auto event = std::make_shared(); + JFactoryTestExceptingFactory fac; + REQUIRE(fac.GetStatus() == JFactory::Status::Uninitialized); + REQUIRE_THROWS(fac.CreateAndGetData(event)); + + REQUIRE(fac.GetStatus() == JFactory::Status::Unprocessed); + REQUIRE_THROWS(fac.CreateAndGetData(event)); + } + + SECTION("Exception in JFactory::Init") { + LOG << "JFactoryTests: Exception in JFactory::Init" << LOG_END; + auto event = std::make_shared(); + JFactoryTestExceptingInInitFactory fac; + REQUIRE(fac.GetStatus() == JFactory::Status::Uninitialized); + REQUIRE_THROWS(fac.CreateAndGetData(event)); + + REQUIRE(fac.GetStatus() == JFactory::Status::Uninitialized); + REQUIRE_THROWS(fac.CreateAndGetData(event)); + } } diff --git a/src/programs/unit_tests/JFactoryTests.h b/src/programs/unit_tests/JFactoryTests.h index 6bc54a201..fdb9362c5 100644 --- a/src/programs/unit_tests/JFactoryTests.h +++ b/src/programs/unit_tests/JFactoryTests.h @@ -50,6 +50,22 @@ struct JFactoryTestDummyFactory : public JFactoryT { } }; +struct JFactoryTestExceptingFactory : public JFactoryT { + + void Process(const std::shared_ptr&) override { + throw JException("This was never going to work!"); + } +}; + +struct JFactoryTestExceptingInInitFactory : public JFactoryT { + + void Init() override { + throw JException("This was never going to initialize even"); + } + void Process(const std::shared_ptr&) override { + } +}; + struct JFactoryTestDummySource: public JEventSource { From e745f43838f77b81a289336f40bd9db99f97f241 Mon Sep 17 00:00:00 2001 From: Nathan Brei Date: Thu, 18 Apr 2024 00:30:44 -0400 Subject: [PATCH 4/4] Remove std::call_once wrapper around JFactory::Init This doesn't make our factories safer or more correct, and may be responsible for the very simple test case that mysteriously hangs, but only during the Docker CI job. --- src/libraries/JANA/JFactory.cc | 2 +- src/libraries/JANA/JFactory.h | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/src/libraries/JANA/JFactory.cc b/src/libraries/JANA/JFactory.cc index f5ffae1a2..d19980f6e 100644 --- a/src/libraries/JANA/JFactory.cc +++ b/src/libraries/JANA/JFactory.cc @@ -14,7 +14,7 @@ void JFactory::Create(const std::shared_ptr& event) { if (mStatus == Status::Uninitialized) { try { - std::call_once(mInitFlag, &JFactory::Init, this); + Init(); mStatus = Status::Unprocessed; } catch (JException& ex) { diff --git a/src/libraries/JANA/JFactory.h b/src/libraries/JANA/JFactory.h index cabaa9930..af360e4d7 100644 --- a/src/libraries/JANA/JFactory.h +++ b/src/libraries/JANA/JFactory.h @@ -202,9 +202,6 @@ class JFactory { CreationStatus mCreationStatus = CreationStatus::NotCreatedYet; - // Used to make sure Init is called only once - std::once_flag mInitFlag; - // Common to components JEventLevel mLevel = JEventLevel::Event; std::string mPluginName;