iox-eclipse-iceoryx#1640 Optimize PolymorphicHandler

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

iceoryx_hoofs/include/iceoryx_hoofs/design_pattern/polymorphic_handler.hpp

@@ -69,11 +69,11 @@ class PolymorphicHandler
     friend class StaticLifetimeGuard<Self>;
 
   public:
-    /// @brief set the current singleton instance
+    /// @brief obtain the current singleton instance
     /// @return the current instance
     /// @note we cannot be sure to use the current handler unless we call get,
     /// i.e. a reference obtained from get may reference a previous handler
-    /// (that is still functional but inactive)
+    /// (that is still functional)
     static Interface& get() noexcept;
 
     /// @brief set the current singleton instance
@@ -82,6 +82,9 @@ class PolymorphicHandler
     /// @note the handler cannot be replaced if it was finalized
     /// @note using a guard in the interface prevents the handler to be destroyed while it is used,
     ///       passing the guard by value is necessary (it has no state anyway)
+    /// @note If finalization takes effect here, setHandler will still change the handler
+    /// This is still correct concurrent behavior in the sense that it maps
+    /// to a sequential execution where the handler is set before finalization.
     template <typename Handler>
     static Interface* set(StaticLifetimeGuard<Handler> handlerGuard) noexcept;
 
@@ -102,20 +105,22 @@ class PolymorphicHandler
     static StaticLifetimeGuard<Self> guard() noexcept;
 
   private:
-    // should a defaultHandler be created, this delays its destruction
-    StaticLifetimeGuard<Default> m_defaultGuard;
-    std::atomic_bool m_isFinal{false};
-    std::atomic<Interface*> m_current;
-
     PolymorphicHandler() noexcept;
 
-    static PolymorphicHandler& instance() noexcept;
+    static PolymorphicHandler& self() noexcept;
 
     static Default& getDefault() noexcept;
 
-    static Interface* getCurrent() noexcept;
+    static Interface* getCurrentRelaxed() noexcept;
+
+    static Interface* getCurrentSync() noexcept;
 
     static Interface* setHandler(Interface& handler) noexcept;
+
+    // should a defaultHandler be created, the guard prevents its destruction
+    StaticLifetimeGuard<Default> m_defaultGuard;
+    std::atomic_bool m_isFinal{false};
+    std::atomic<Interface*> m_current{nullptr};
 };
 
 } // namespace design_pattern

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

@@ -64,18 +64,18 @@ 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
+    thread_local Interface* localHandler = getCurrentSync(); // initialized once per thread on first call
 
-    // weak load, required since it could be set in another thread concurrently
-    auto* currentHandler = instance().m_current.load(std::memory_order_relaxed);
+    // required since it could be set in another thread concurrently
+    auto* currentHandler = getCurrentRelaxed();
 
     // is the known localHandler outdated?
     // NB: this is a variation of double checked locking but with atomics:
     // we avoid the more costly operation on the hot path unless there was a change (rare case)
     if (localHandler != currentHandler)
     {
         // stronger sync of memory
-        localHandler = getCurrent();
+        localHandler = getCurrentSync();
         // note that it may concurrently change again but we do not loop to check as there is no
         // point in such a lock-free code (it may also change while returning)
     }
@@ -96,19 +96,26 @@ Interface* PolymorphicHandler<Interface, Default, Hooks>::set(StaticLifetimeGuar
 template <typename Interface, typename Default, typename Hooks>
 Interface* PolymorphicHandler<Interface, Default, Hooks>::setHandler(Interface& handler) noexcept
 {
-    auto& ins = instance();
+    auto& s = self();
     // m_current is now guaranteed to be set
 
-    if (ins.m_isFinal.load(std::memory_order_acquire))
+    // ensure we cannot miss that it was set to true concurrently
+    // OK, since it will never change back from true to false
+    bool exp{true};
+    if (s.m_isFinal.compare_exchange_strong(exp, true, std::memory_order_relaxed))
     {
         // it must be ensured that the handlers still exist and are thread-safe,
         // 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);
+        Hooks::onSetAfterFinalize(*getCurrentSync(), handler);
         return nullptr;
     }
 
-    auto prev = ins.m_current.exchange(&handler, std::memory_order_acq_rel);
+    // Note that if finalization takes effect here, setHandler will still change the handler
+    // This is still correct concurrent behavior in the sense that it maps
+    // to a sequential execution where the handler is set before finalization.
+
+    auto prev = s.m_current.exchange(&handler, std::memory_order_acq_rel);
 
     return prev;
 }
@@ -122,7 +129,7 @@ Interface* PolymorphicHandler<Interface, Default, Hooks>::reset() noexcept
 template <typename Interface, typename Default, typename Hooks>
 void PolymorphicHandler<Interface, Default, Hooks>::finalize() noexcept
 {
-    instance().m_isFinal.store(true, std::memory_order_release);
+    self().m_isFinal.store(true, std::memory_order_relaxed);
 }
 
 template <typename Interface, typename Default, typename Hooks>
@@ -132,22 +139,33 @@ PolymorphicHandler<Interface, Default, Hooks>::PolymorphicHandler() noexcept
 }
 
 template <typename Interface, typename Default, typename Hooks>
-PolymorphicHandler<Interface, Default, Hooks>& PolymorphicHandler<Interface, Default, Hooks>::instance() noexcept
+PolymorphicHandler<Interface, Default, Hooks>& PolymorphicHandler<Interface, Default, Hooks>::self() noexcept
 {
-    return StaticLifetimeGuard<Self>::instance();
+    // NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) singleton pattern
+    static auto& ins = StaticLifetimeGuard<Self>::instance();
+    return ins;
 }
 
 template <typename Interface, typename Default, typename Hooks>
 Default& PolymorphicHandler<Interface, Default, Hooks>::getDefault() noexcept
 {
-    return StaticLifetimeGuard<Default>::instance();
+    // NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) singleton pattern
+    static auto& ins = StaticLifetimeGuard<Default>::instance();
+    return ins;
+}
+
+template <typename Interface, typename Default, typename Hooks>
+Interface* PolymorphicHandler<Interface, Default, Hooks>::getCurrentRelaxed() noexcept
+{
+    // only load the pointer atomically
+    return self().m_current.load(std::memory_order_relaxed);
 }
 
 template <typename Interface, typename Default, typename Hooks>
-Interface* PolymorphicHandler<Interface, Default, Hooks>::getCurrent() noexcept
+Interface* PolymorphicHandler<Interface, Default, Hooks>::getCurrentSync() noexcept
 {
     // must be strong enough to sync memory of the object pointed to
-    return instance().m_current.load(std::memory_order_acquire);
+    return self().m_current.load(std::memory_order_acquire);
 }
 
 template <typename Interface, typename Default, typename Hooks>

iceoryx_hoofs/test/moduletests/test_polymorphic_handler.cpp

@@ -18,7 +18,10 @@
 #include "iceoryx_hoofs/design_pattern/static_lifetime_guard.hpp"
 
 #include "test.hpp"
+#include <atomic>
+#include <chrono>
 #include <iostream>
+#include <thread>
 
 namespace
 {
@@ -180,6 +183,48 @@ TEST_F(PolymorphicHandler_test, setToCurrentHandlerWorks)
     EXPECT_EQ(prevHandler, &alternateHandler);
 }
 
+TEST_F(PolymorphicHandler_test, defaultHandlerIsVisibleInAllThreads)
+{
+    ::testing::Test::RecordProperty("TEST_ID", "caa1e507-7cc1-4233-8c9c-5c4e56be9fb3");
+
+    Handler::set(defaultGuard);
+
+    std::atomic<uint32_t> count{0};
+
+    auto checkHandler = [&]() {
+        auto& h = Handler::get();
+
+        if (h.id() == DEFAULT_ID)
+        {
+            ++count;
+        }
+    };
+
+    constexpr uint32_t NUM_THREADS{2}; // including main thread
+
+    std::thread t(checkHandler);
+    t.join();
+
+    checkHandler();
+
+    EXPECT_EQ(count, NUM_THREADS);
+}
+
+TEST_F(PolymorphicHandler_test, handlerChangePropagatesBetweenThreads)
+{
+    ::testing::Test::RecordProperty("TEST_ID", "f0a8e941-e064-4889-a6db-425b35a3b7b0");
+
+    Handler::set(defaultGuard);
+    EXPECT_EQ(Handler::get().id(), DEFAULT_ID);
+
+    std::thread t([]() { Handler::set(alternateGuard); });
+
+    t.join();
+
+    // the handler should now be visible in the main thread
+    EXPECT_EQ(Handler::get().id(), ALTERNATE_ID);
+}
+
 TEST_F(PolymorphicHandler_test, settingAfterFinalizeCallsHook)
 {
     ::testing::Test::RecordProperty("TEST_ID", "171ac802-01b9-4e08-80a6-6f2defecaf6d");

iceoryx_hoofs/test/moduletests/test_static_lifetime_guard.cpp

@@ -25,15 +25,15 @@ using namespace ::testing;
 
 // allows to create different types for independent tests
 template <uint64_t N>
-struct Foo
+struct Fou
 {
-    Foo()
+    Fou()
         : id(++instancesCreated)
     {
         ++ctorCalled;
     }
 
-    ~Foo()
+    ~Fou()
     {
         ++dtorCalled;
     }
@@ -55,16 +55,16 @@ constexpr uint32_t FIRST_INSTANCE_ID{1};
 constexpr uint32_t SECOND_INSTANCE_ID{2};
 
 template <uint64_t N>
-uint32_t Foo<N>::ctorCalled{0};
+uint32_t Fou<N>::ctorCalled{0};
 
 template <uint64_t N>
-uint32_t Foo<N>::dtorCalled{0};
+uint32_t Fou<N>::dtorCalled{0};
 
 template <uint64_t N>
-uint32_t Foo<N>::instancesCreated{0};
+uint32_t Fou<N>::instancesCreated{0};
 
 template <uint64_t N>
-using TestGuard = iox::design_pattern::StaticLifetimeGuard<Foo<N>>;
+using TestGuard = iox::design_pattern::StaticLifetimeGuard<Fou<N>>;
 
 // create a bundle of types and functions that are relevant for the tests,
 // since we need a different static type for each test
@@ -75,7 +75,7 @@ struct TestTypes : public TestGuard<N>
     // NB: using the base class methods would admit to argue that we test another type,
     // hence we use this alias of the Guard
     using Guard = TestGuard<N>;
-    using Foo = Foo<N>;
+    using Foo = Fou<N>;
 
     using TestGuard<N>::setCount;