Merge pull request #13574 from Swiftb0y/refactor/control-value

Modernize `ControlValueAtomic`

src/control/controlvalue.h

@@ -1,20 +1,20 @@
 #pragma once
 
-#include <QAtomicInt>
-#include <QObject>
+#include <atomic>
+#include <bit>
+#include <cstddef>
 #include <limits>
 
-#include "util/assert.h"
-#include "util/compatibility/qatomic.h"
+namespace {
 
 // for lock free access, this value has to be >= the number of value using threads
 // value must be a fraction of an integer
-constexpr int kDefaultRingSize = 8;
+constexpr std::size_t kDefaultRingSize = 8;
 // there are basically unlimited readers allowed at each ring element
 // but we have to count them so max() is just fine.
-// NOTE(rryan): Wrapping max with parentheses avoids conflict with the max macro
-// defined in windows.h.
-constexpr int kMaxReaderSlots = (std::numeric_limits<int>::max)();
+constexpr std::size_t kMaxReaderSlots = std::numeric_limits<std::size_t>::max();
+
+} // namespace
 
 // A single instance of a value of type T along with an atomic integer which
 // tracks the current number of readers or writers of the slot. The value
@@ -35,34 +35,37 @@ class ControlRingValue {
     // slot, because the stored value is preserved.
     bool tryGet(T* value) const {
         // Read while consuming one readerSlot
-        if (m_readerSlots.fetchAndAddAcquire(-1) > 0) {
+        if (m_readerSlots.fetch_sub(1, std::memory_order_acquire) > 0) {
             // Reader slot has been acquired, no writer is active
             *value = m_value;
-            m_readerSlots.fetchAndAddRelease(1);
+            m_readerSlots.fetch_add(1, std::memory_order_release);
             // We need the early return here to make the compiler
             // aware that *value is initialised in the true case.
             return true;
         }
-        m_readerSlots.fetchAndAddRelease(1);
+        m_readerSlots.fetch_add(1, std::memory_order_release);
         return false;
     }
 
     bool trySet(const T& value) {
         // try to lock this element entirely for reading
-        if (m_readerSlots.testAndSetAcquire(kMaxReaderSlots, 0)) {
+        std::size_t expected = kMaxReaderSlots;
+        if (m_readerSlots.compare_exchange_strong(expected, 0, std::memory_order_acquire)) {
             m_value = value;
             // We need to re-add kMaxReaderSlots instead of storing it
             // to keep the balance if readers have decreased the number
             // of slots in the meantime!
-            m_readerSlots.fetchAndAddRelease(kMaxReaderSlots);
+            m_readerSlots.fetch_add(kMaxReaderSlots, std::memory_order_release);
             return true;
         }
         return false;
     }
 
   private:
     T m_value;
-    mutable QAtomicInt m_readerSlots;
+    mutable std::atomic<std::size_t> m_readerSlots;
+    static_assert(std::atomic<std::size_t>::is_always_lock_free,
+            "atomics used for lock-free data storage are not lock-free");
 };
 
 // Ring buffer based implementation for all Types sizeof(T) > sizeof(void*)
@@ -71,12 +74,15 @@ class ControlRingValue {
 // ring-buffer of ControlRingValues and a read pointer and write pointer to
 // provide getValue()/setValue() methods which *sacrifice perfect consistency*
 // for the benefit of wait-free read/write access to a value.
-template <typename T, int cRingSize, bool ATOMIC = false>
+template<typename T, std::size_t cRingSize, bool ATOMIC = false>
 class ControlValueAtomicBase {
+    static_assert(std::has_single_bit(cRingSize),
+            "cRingSize is not a power of two; required for optimal alignment");
+
   public:
-    inline T getValue() const {
+    T getValue() const {
         T value;
-        unsigned int index = static_cast<unsigned int>(atomicLoadRelaxed(m_readIndex)) % cRingSize;
+        std::size_t index = m_readIndex.load(std::memory_order_relaxed) % cRingSize;
         while (!m_ring[index].tryGet(&value)) {
             // We are here if
             // 1) there are more then kMaxReaderSlots reader (get) reading the same value or
@@ -91,12 +97,12 @@ class ControlValueAtomicBase {
         return value;
     }
 
-    inline void setValue(const T& value) {
+    void setValue(const T& value) {
         // Test if we can read atomic
         // This test is const and will be mad only at compile time
-        unsigned int index;
+        std::size_t index;
         do {
-            index = static_cast<unsigned int>(m_writeIndex.fetchAndAddAcquire(1)) % cRingSize;
+            index = m_writeIndex.fetch_add(1, std::memory_order_acquire) % cRingSize;
             // This will be repeated if the value is locked
             // 1) by another writer writing at the same time or
             // 2) a delayed reader is still blocking the formerly current value
@@ -107,58 +113,62 @@ class ControlValueAtomicBase {
 
   protected:
     ControlValueAtomicBase() : m_readIndex(0), m_writeIndex(1) {
-        // NOTE(rryan): Wrapping max with parentheses avoids conflict with the
-        // max macro defined in windows.h.
-        DEBUG_ASSERT(((std::numeric_limits<unsigned int>::max)() % cRingSize) == (cRingSize - 1));
+    }
+    ControlValueAtomicBase(const T& value)
+            : ControlValueAtomicBase() {
+        setValue(value);
     }
 
   private:
     // In worst case, each reader can consume a reader slot from a different ring element.
     // In this case there is still one ring element available for writing.
     ControlRingValue<T> m_ring[cRingSize];
-    QAtomicInt m_readIndex;
-    QAtomicInt m_writeIndex;
+    std::atomic<std::size_t> m_readIndex;
+    std::atomic<std::size_t> m_writeIndex;
+    static_assert(std::atomic<std::size_t>::is_always_lock_free,
+            "atomics used for lock-free data storage are not lock-free");
 };
 
 // Specialized template for types that are deemed to be atomic on the target
 // architecture. Instead of using a read/write ring to guarantee atomicity,
 // direct assignment/read of an aligned member variable is used.
-template<typename T, int cRingSize>
+template<typename T, std::size_t cRingSize>
 class ControlValueAtomicBase<T, cRingSize, true> {
   public:
-    inline T getValue() const {
+    T getValue() const {
         return m_value;
     }
 
-
-    inline void setValue(const T& value) {
+    void setValue(const T& value) {
         m_value = value;
     }
 
   protected:
     ControlValueAtomicBase() = default;
+    ControlValueAtomicBase(const T& value)
+            : m_value(value){};
 
   private:
-#if defined(__GNUC__)
-    T m_value __attribute__((aligned(sizeof(void*))));
-#elif defined(_MSC_VER)
-#ifdef _WIN64
-    T __declspec(align(8)) m_value;
-#else
-    T __declspec(align(4)) m_value;
-#endif
-#else
-    T m_value;
-#endif
+    std::atomic<T> m_value;
+    static_assert(std::atomic<T>::is_always_lock_free);
 };
 
-// ControlValueAtomic is a wrapper around ControlValueAtomicBase which uses the
-// sizeof(T) to determine which underlying implementation of
-// ControlValueAtomicBase to use. For types where sizeof(T) <= sizeof(void*),
-// the specialized implementation of ControlValueAtomicBase for types that are
-// atomic on the architecture is used.
-template <typename T, int cRingSize = kDefaultRingSize>
-class ControlValueAtomic : public ControlValueAtomicBase<T, cRingSize, sizeof(T) <= sizeof(void*)> {
+template<typename T, std::size_t cRingSize = kDefaultRingSize>
+class ControlValueAtomic : public ControlValueAtomicBase<T,
+                                   cRingSize,
+                                   std::atomic<T>::is_always_lock_free> {
+    // naming the parent class is tedious because all template parameters have to be specified,
+    // so this alias makes it a little more manageable.
+    using ParentT = ControlValueAtomicBase<T, cRingSize, std::atomic<T>::is_always_lock_free>;
+
+    static_assert(!(!std::atomic<T>::is_always_lock_free &&
+                          sizeof(T) <= sizeof(void*)),
+            "T is not lock free even though it is smaller than void*! Consider "
+            "using `std::atomic<T>::is_lock_free()` to only fallback to the "
+            "eventually-consistent ControlValueAtomicBase when necessary");
+
   public:
     ControlValueAtomic() = default;
+    ControlValueAtomic(const T& value)
+            : ParentT(value){};
 };