diff --git a/Source/Core/Common/CMakeLists.txt b/Source/Core/Common/CMakeLists.txt index 69ab3baf6ad..e8bd9171750 100644 --- a/Source/Core/Common/CMakeLists.txt +++ b/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 diff --git a/Source/Core/Common/Config/Config.h b/Source/Core/Common/Config/Config.h index ea6058a8d35..e3e3d3df3b5 100644 --- a/Source/Core/Common/Config/Config.h +++ b/Source/Core/Common/Config/Config.h @@ -72,10 +72,10 @@ T Get(const Info& 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 diff --git a/Source/Core/Common/Config/ConfigInfo.h b/Source/Core/Common/Config/ConfigInfo.h index e8d904fae27..32f5db069f5 100644 --- a/Source/Core/Common/Config/ConfigInfo.h +++ b/Source/Core/Common/Config/ConfigInfo.h @@ -4,12 +4,12 @@ #pragma once #include -#include #include #include #include "Common/CommonTypes.h" #include "Common/Config/Enums.h" +#include "Common/Mutex.h" #include "Common/TypeUtils.h" namespace Config @@ -35,73 +35,58 @@ template 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& other) { *this = other; } - - // Not thread-safe - Info(Info&& other) { *this = std::move(other); } + Info(const Info& other) + : m_location{other.m_location}, m_default_value{other.m_default_value}, + m_cached_value(other.GetCachedValue()) + { + } // Make it easy to convert Info into Info> // so that enum settings can still easily work with code that doesn't care about the enum values. template Enum> Info(const Info& other) + : m_location{other.GetLocation()}, m_default_value{static_cast(other.GetDefaultValue())}, + m_cached_value(other.template GetCachedValueCasted()) { - *this = other; } - Info& operator=(const Info& other) - { - m_location = other.GetLocation(); - m_default_value = other.GetDefaultValue(); - m_cached_value = other.GetCachedValue(); - return *this; - } + ~Info() = default; - // Not thread-safe - Info& operator=(Info&& 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 into Info> - // so that enum settings can still easily work with code that doesn't care about the enum values. - template Enum> - Info& operator=(const Info& other) - { - m_location = other.GetLocation(); - m_default_value = static_cast(other.GetDefaultValue()); - m_cached_value = other.template GetCachedValueCasted(); - 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 GetCachedValue() const { - std::shared_lock lock(m_cached_value_mutex); + std::lock_guard lk{m_cached_value_mutex}; return m_cached_value; } template CachedValue GetCachedValueCasted() const { - std::shared_lock lock(m_cached_value_mutex); - return CachedValue{static_cast(m_cached_value.value), m_cached_value.config_version}; + std::lock_guard lk{m_cached_value_mutex}; + return {static_cast(m_cached_value.value), m_cached_value.config_version}; } - void SetCachedValue(const CachedValue& cached_value) const + // Only updates if the provided config_version is newer. + void TryToSetCachedValue(CachedValue 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 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 diff --git a/Source/Core/Common/Mutex.h b/Source/Core/Common/Mutex.h new file mode 100644 index 00000000000..22aebd08b1b --- /dev/null +++ b/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 + +namespace Common +{ +namespace detail +{ +template +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; + +// Very fast to lock and unlock when uncontested (~3x faster than std::mutex). +using SpinMutex = detail::AtomicMutexBase; + +} // namespace Common diff --git a/Source/Core/DolphinLib.props b/Source/Core/DolphinLib.props index 5049ac785f6..32f76c8d014 100644 --- a/Source/Core/DolphinLib.props +++ b/Source/Core/DolphinLib.props @@ -145,6 +145,7 @@ + diff --git a/Source/UnitTests/Common/CMakeLists.txt b/Source/UnitTests/Common/CMakeLists.txt index 3b0ffe79e61..a2e21d7a802 100644 --- a/Source/UnitTests/Common/CMakeLists.txt +++ b/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) diff --git a/Source/UnitTests/Common/MutexTest.cpp b/Source/UnitTests/Common/MutexTest.cpp new file mode 100644 index 00000000000..222724ad502 --- /dev/null +++ b/Source/UnitTests/Common/MutexTest.cpp @@ -0,0 +1,102 @@ +// Copyright 2025 Dolphin Emulator Project +// SPDX-License-Identifier: GPL-2.0-or-later + +#include + +#include +#include +#include +#include + +#include "Common/Mutex.h" + +template +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 try_lock_fail_count{}; + std::vector 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("AtomicMutex"); + DoAtomicMutexTests("SpinMutex"); +} diff --git a/Source/UnitTests/UnitTests.vcxproj b/Source/UnitTests/UnitTests.vcxproj index f35f6a691a9..698e33bc37a 100644 --- a/Source/UnitTests/UnitTests.vcxproj +++ b/Source/UnitTests/UnitTests.vcxproj @@ -53,6 +53,7 @@ +