iox-eclipse-iceoryx#1640 Optimize memory order

Signed-off-by: Matthias Killat <[email protected]>

iceoryx_hoofs/include/iceoryx_hoofs/design_pattern/polymorphic_handler.hpp

@@ -48,18 +48,19 @@ class Activatable
     bool isActive() const noexcept;
 
   private:
-    bool m_active{true};
+    std::atomic<bool> m_active{true};
 };
 
 namespace detail
 {
 
-///@brief default hooks for the PolymorphicHandler
-///@note template hooks to avoid forced virtual inheritance (STL approach),
+/// @brief default hooks for the PolymorphicHandler
+/// @tparam Interface the handler interface
+/// @note template hooks to avoid forced virtual inheritance
 template <typename Interface>
 struct DefaultHooks
 {
-    /// @brief called after if the polymorphic handler is set or reset after finalize
+    /// @brief called if the polymorphic handler is set or reset after finalize
     /// @param currentInstance the current instance of the handler singleton
     /// @param newInstance the instance of the handler singleton to be set
     static void onSetAfterFinalize(Interface& currentInstance, Interface& newInstance) noexcept;
@@ -78,7 +79,7 @@ struct DefaultHooks
 /// @note The lifetime of external non-default instances must exceed the lifetime of the PolymorphicHandler.
 /// @note The PolymorphicHandler is guaranteed to provide a valid handler during the whole program lifetime (static).
 ///       It is hence not advisable to have other static variables depend on the PolymorphicHandler.
-///       It must be ensured that the are destroyed before the PolymorphicHandler.
+///       It must be ensured that they are destroyed before the PolymorphicHandler.
 /// @note Hooks must implement
 ///       static void onSetAfterFinalize(Interface& /*currentInstance*/, Interface& /*newInstance*/).
 template <typename Interface, typename Default, typename Hooks = detail::DefaultHooks<Interface>>

iceoryx_hoofs/include/iceoryx_hoofs/internal/design_pattern/polymorphic_handler.inl

@@ -42,17 +42,17 @@ void DefaultHooks<Interface>::onSetAfterFinalize(Interface&, Interface&) noexcep
 
 void Activatable::activate() noexcept
 {
-    m_active = true;
+    m_active.store(true, std::memory_order_relaxed);
 }
 
 void Activatable::deactivate() noexcept
 {
-    m_active = false;
+    m_active.store(false, std::memory_order_relaxed);
 }
 
 bool Activatable::isActive() const noexcept
 {
-    return m_active;
+    return m_active.load(std::memory_order_relaxed);
 }
 
 // The get method is considered to be on the hot path and works as follows:
@@ -72,20 +72,18 @@ bool Activatable::isActive() const noexcept
 //    - deactivate the old handler (can still be used as it still needs to exist)
 //
 
-// on any call after the handler was changes in another thread
+// on any call after the handler was changed in another thread
 // 1. we check whether the handler is active
 // (can be outdated information but will eventually be false once the atomic value is updated)
-// 2. if it was changed it is now inactive and we wait to obtain the mutex lock
-// under lock, we update the local handler to the new one (note that it cannot change
-// while this happens as we hold the lock)
-
+// 2. if it was changed it is now inactive and we update the local handler
+// Note that it may change again hence we loop (usually it acts like an if, i.e. checks once)
 template <typename Interface, typename Default, typename Hooks>
 Interface& PolymorphicHandler<Interface, Default, Hooks>::get() noexcept
 {
     // NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) false positive, thread_local
     thread_local Interface* localHandler = getCurrent(); // initialized once per thread on first call
 
-    if (!localHandler->isActive())
+    while (!localHandler->isActive())
     {
         localHandler = getCurrent();
     }
@@ -108,10 +106,11 @@ Interface* PolymorphicHandler<Interface, Default, Hooks>::setHandler(Interface&
     auto& ins = instance();
     // m_current is now guaranteed to be set
 
-    if (ins.m_isFinal.load())
+    if (ins.m_isFinal.load(std::memory_order_acquire))
     {
         // it must be ensured that the handlers still exist and are thread-safe,
-        // this is ensured for the default handler
+        // this is ensured for the default handler by m_defaultGuard
+        // (the primary guard that is constructed with the instance alone is not sufficient)
         Hooks::onSetAfterFinalize(*getCurrent(), handler);
         return nullptr;
     }
@@ -135,7 +134,7 @@ template <typename Interface, typename Default, typename Hooks>
 void PolymorphicHandler<Interface, Default, Hooks>::finalize() noexcept
 {
     auto& ins = instance();
-    ins.m_isFinal.store(true);
+    ins.m_isFinal.store(true, std::memory_order_release);
 }
 
 template <typename Interface, typename Default, typename Hooks>

iceoryx_hoofs/test/moduletests/test_static_lifetime_guard.cpp

@@ -202,6 +202,15 @@ TEST_F(StaticLifetimeGuard_test, destructionAtZeroCountWorks)
 TEST_F(StaticLifetimeGuard_test, constructionAfterDestructionWorks)
 {
     ::testing::Test::RecordProperty("TEST_ID", "0077e73d-ddf5-47e7-a7c6-93819f376175");
+
+    // ensure that the old instance is destroyed if it exists
+    if (Guard::count() > 0)
+    {
+        Guard guard;
+        Guard::setCount(1);
+        // now the instance will be destroyed once the guard is destroyed
+    }
+
     Foo::reset();
     EXPECT_EQ(Guard::count(), 0);
     {