Merge pull request #14061 from jordan-woyak/config-info-spin-mutex

ConfigInfo: Cleanups and change mutex to a spin lock.

Source/Core/Common/CMakeLists.txt

@@ -111,6 +111,7 @@ add_library(common
   MinizipUtil.h
   MsgHandler.cpp
   MsgHandler.h
+  Mutex.h
   NandPaths.cpp
   NandPaths.h
   Network.cpp

Source/Core/Common/Config/Config.h

@@ -72,10 +72,10 @@ T Get(const Info<T>& info)
     cached.value = GetUncached(info);
     cached.config_version = config_version;
 
-    info.SetCachedValue(cached);
+    info.TryToSetCachedValue(cached);
   }
 
-  return cached.value;
+  return std::move(cached.value);
 }
 
 template <typename T>

Source/Core/Common/Config/ConfigInfo.h

@@ -4,12 +4,12 @@
 #pragma once
 
 #include <mutex>
-#include <shared_mutex>
 #include <string>
 #include <utility>
 
 #include "Common/CommonTypes.h"
 #include "Common/Config/Enums.h"
+#include "Common/Mutex.h"
 #include "Common/TypeUtils.h"
 
 namespace Config
@@ -35,73 +35,58 @@ template <typename T>
 class Info
 {
 public:
-  constexpr Info(const Location& location, const T& default_value)
-      : m_location{location}, m_default_value{default_value}, m_cached_value{default_value, 0}
+  constexpr Info(Location location, T default_value)
+      : m_location{std::move(location)}, m_default_value{default_value},
+        m_cached_value{std::move(default_value), 0}
   {
   }
 
-  Info(const Info<T>& other) { *this = other; }
-
-  // Not thread-safe
-  Info(Info<T>&& other) { *this = std::move(other); }
+  Info(const Info<T>& other)
+      : m_location{other.m_location}, m_default_value{other.m_default_value},
+        m_cached_value(other.GetCachedValue())
+  {
+  }
 
   // Make it easy to convert Info<Enum> into Info<UnderlyingType<Enum>>
   // so that enum settings can still easily work with code that doesn't care about the enum values.
   template <Common::TypedEnum<T> Enum>
   Info(const Info<Enum>& other)
+      : m_location{other.GetLocation()}, m_default_value{static_cast<T>(other.GetDefaultValue())},
+        m_cached_value(other.template GetCachedValueCasted<T>())
   {
-    *this = other;
   }
 
-  Info<T>& operator=(const Info<T>& other)
-  {
-    m_location = other.GetLocation();
-    m_default_value = other.GetDefaultValue();
-    m_cached_value = other.GetCachedValue();
-    return *this;
-  }
+  ~Info() = default;
 
-  // Not thread-safe
-  Info<T>& operator=(Info<T>&& other)
-  {
-    m_location = std::move(other.m_location);
-    m_default_value = std::move(other.m_default_value);
-    m_cached_value = std::move(other.m_cached_value);
-    return *this;
-  }
-
-  // Make it easy to convert Info<Enum> into Info<UnderlyingType<Enum>>
-  // so that enum settings can still easily work with code that doesn't care about the enum values.
-  template <Common::TypedEnum<T> Enum>
-  Info<T>& operator=(const Info<Enum>& other)
-  {
-    m_location = other.GetLocation();
-    m_default_value = static_cast<T>(other.GetDefaultValue());
-    m_cached_value = other.template GetCachedValueCasted<T>();
-    return *this;
-  }
+  // Assignments after construction would require more locking to be thread safe.
+  // It seems unnecessary to have this functionality anyways.
+  Info& operator=(const Info&) = delete;
+  Info& operator=(Info&&) = delete;
+  // Moves are also unnecessary and would be thread unsafe without additional locking.
+  Info(Info&&) = delete;
 
   constexpr const Location& GetLocation() const { return m_location; }
   constexpr const T& GetDefaultValue() const { return m_default_value; }
 
   CachedValue<T> GetCachedValue() const
   {
-    std::shared_lock lock(m_cached_value_mutex);
+    std::lock_guard lk{m_cached_value_mutex};
     return m_cached_value;
   }
 
   template <typename U>
   CachedValue<U> GetCachedValueCasted() const
   {
-    std::shared_lock lock(m_cached_value_mutex);
-    return CachedValue<U>{static_cast<U>(m_cached_value.value), m_cached_value.config_version};
+    std::lock_guard lk{m_cached_value_mutex};
+    return {static_cast<U>(m_cached_value.value), m_cached_value.config_version};
   }
 
-  void SetCachedValue(const CachedValue<T>& cached_value) const
+  // Only updates if the provided config_version is newer.
+  void TryToSetCachedValue(CachedValue<T> new_value) const
   {
-    std::unique_lock lock(m_cached_value_mutex);
-    if (m_cached_value.config_version < cached_value.config_version)
-      m_cached_value = cached_value;
+    std::lock_guard lk{m_cached_value_mutex};
+    if (new_value.config_version > m_cached_value.config_version)
+      m_cached_value = std::move(new_value);
   }
 
 private:
@@ -109,6 +94,10 @@ private:
   T m_default_value;
 
   mutable CachedValue<T> m_cached_value;
-  mutable std::shared_mutex m_cached_value_mutex;
+
+  // In testing, this mutex is effectively never contested.
+  // The lock durations are brief and each `Info` object is mostly relevant to one thread.
+  // Common::SpinMutex is ~3x faster than std::shared_mutex when uncontested.
+  mutable Common::SpinMutex m_cached_value_mutex;
 };
 }  // namespace Config

Source/Core/Common/Mutex.h

@@ -0,0 +1,51 @@
+// Copyright 2025 Dolphin Emulator Project
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#pragma once
+
+#include <atomic>
+
+namespace Common
+{
+namespace detail
+{
+template <bool UseAtomicWait>
+class AtomicMutexBase
+{
+public:
+  void lock()
+  {
+    while (m_lock.exchange(true, std::memory_order_acquire))
+    {
+      if constexpr (UseAtomicWait)
+        m_lock.wait(true, std::memory_order_relaxed);
+    }
+  }
+
+  bool try_lock()
+  {
+    bool expected = false;
+    return m_lock.compare_exchange_weak(expected, true, std::memory_order_acquire,
+                                        std::memory_order_relaxed);
+  }
+
+  // Unlike with std::mutex, this call may come from any thread.
+  void unlock()
+  {
+    m_lock.store(false, std::memory_order_release);
+    if constexpr (UseAtomicWait)
+      m_lock.notify_one();
+  }
+
+private:
+  std::atomic_bool m_lock{};
+};
+}  // namespace detail
+
+// Sometimes faster than std::mutex.
+using AtomicMutex = detail::AtomicMutexBase<true>;
+
+// Very fast to lock and unlock when uncontested (~3x faster than std::mutex).
+using SpinMutex = detail::AtomicMutexBase<false>;
+
+}  // namespace Common

Source/Core/DolphinLib.props

@@ -145,6 +145,7 @@
     <ClInclude Include="Common\MemoryUtil.h" />
     <ClInclude Include="Common\MinizipUtil.h" />
     <ClInclude Include="Common\MsgHandler.h" />
+    <ClInclude Include="Common\Mutex.h" />
     <ClInclude Include="Common\NandPaths.h" />
     <ClInclude Include="Common\Network.h" />
     <ClInclude Include="Common\PcapFile.h" />

Source/UnitTests/Common/CMakeLists.txt

@@ -14,6 +14,7 @@ add_dolphin_test(FixedSizeQueueTest FixedSizeQueueTest.cpp)
 add_dolphin_test(FlagTest FlagTest.cpp)
 add_dolphin_test(FloatUtilsTest FloatUtilsTest.cpp)
 add_dolphin_test(MathUtilTest MathUtilTest.cpp)
+add_dolphin_test(MutexTest MutexTest.cpp)
 add_dolphin_test(NandPathsTest NandPathsTest.cpp)
 add_dolphin_test(SettingsHandlerTest SettingsHandlerTest.cpp)
 add_dolphin_test(SPSCQueueTest SPSCQueueTest.cpp)

Source/UnitTests/Common/MutexTest.cpp

@@ -0,0 +1,102 @@
+// Copyright 2025 Dolphin Emulator Project
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include <gtest/gtest.h>
+
+#include <algorithm>
+#include <chrono>
+#include <mutex>
+#include <thread>
+
+#include "Common/Mutex.h"
+
+template <typename MutexType>
+static void DoAtomicMutexTests(const char mutex_name[])
+{
+  MutexType work_mutex;
+
+  bool worker_done = false;
+
+  static constexpr auto SLEEP_TIME = std::chrono::microseconds{1};
+
+  // lock() on main thread, unlock() on worker thread.
+  std::thread thread{[&, lk = std::unique_lock{work_mutex}] {
+    std::this_thread::sleep_for(SLEEP_TIME);
+    worker_done = true;
+  }};
+
+  // lock() waits for the thread to unlock().
+  {
+    std::lock_guard lk{work_mutex};
+    EXPECT_TRUE(worker_done);
+  }
+
+  thread.join();
+
+  // Prevent below workers from incrementing `done_count`.
+  MutexType done_mutex;
+  std::unique_lock done_lk{done_mutex};
+
+  // try_lock() fails when holding a lock.
+  EXPECT_FALSE(done_mutex.try_lock());
+
+  static constexpr int THREAD_COUNT = 4;
+  static constexpr int REPEAT_COUNT = 100;
+  static constexpr int TOTAL_ITERATIONS = THREAD_COUNT * REPEAT_COUNT;
+
+  int done_count = 0;
+  int work_count = 0;
+  std::atomic<int> try_lock_fail_count{};
+  std::vector<std::thread> threads(THREAD_COUNT);
+  for (auto& t : threads)
+  {
+    t = std::thread{[&] {
+      // lock() blocks until main thread unlock()s.
+      {
+        std::lock_guard lk{done_mutex};
+        ++done_count;
+      }
+
+      // Contesting lock() and try_lock() doesn't explode.
+      for (int i = 0; i != REPEAT_COUNT; ++i)
+      {
+        {
+          std::lock_guard lk{work_mutex};
+          ++work_count;
+        }
+
+        // Try lock in a loop.
+        while (!work_mutex.try_lock())
+        {
+          try_lock_fail_count.fetch_add(1, std::memory_order_relaxed);
+        }
+        std::lock_guard lk{work_mutex, std::adopt_lock};
+        ++work_count;
+      }
+    }};
+  }
+
+  std::this_thread::sleep_for(SLEEP_TIME);
+
+  // The threads are still blocking on done_mutex.
+  EXPECT_EQ(done_count, 0);
+
+  done_lk.unlock();
+  std::ranges::for_each(threads, &std::thread::join);
+
+  // The threads finished.
+  EXPECT_EQ(done_count, THREAD_COUNT);
+  EXPECT_EQ(work_count, TOTAL_ITERATIONS * 2);
+
+  GTEST_LOG_(INFO) << mutex_name << "::try_lock() failure %: "
+                   << (try_lock_fail_count * 100.0 / (TOTAL_ITERATIONS + try_lock_fail_count));
+
+  // Things are still sane after contesting in worker threads.
+  std::lock_guard lk{work_mutex};
+}
+
+TEST(Mutex, AtomicMutex)
+{
+  DoAtomicMutexTests<Common::AtomicMutex>("AtomicMutex");
+  DoAtomicMutexTests<Common::SpinMutex>("SpinMutex");
+}

Source/UnitTests/UnitTests.vcxproj

@@ -53,6 +53,7 @@
     <ClCompile Include="Common\FlagTest.cpp" />
     <ClCompile Include="Common\FloatUtilsTest.cpp" />
     <ClCompile Include="Common\MathUtilTest.cpp" />
+    <ClCompile Include="Common\MutexTest.cpp" />
     <ClCompile Include="Common\NandPathsTest.cpp" />
     <ClCompile Include="Common\SettingsHandlerTest.cpp" />
     <ClCompile Include="Common\SPSCQueueTest.cpp" />