Merge 8b937187bb into 8ee64a84c7

Source/Core/Common/CMakeLists.txt

@@ -70,6 +70,7 @@ add_library(common
   Flag.h
   FloatUtils.cpp
   FloatUtils.h
+  FlushThread.h
   FormatUtil.h
   FPURoundMode.h
   GekkoDisassembler.cpp

Source/Core/Common/FlushThread.h

@@ -0,0 +1,143 @@
+// Copyright 2025 Dolphin Emulator Project
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#pragma once
+
+#include <atomic>
+#include <functional>
+#include <semaphore>
+#include <thread>
+
+#include "Common/CommonTypes.h"
+#include "Common/Event.h"
+#include "Common/Thread.h"
+
+// This class allows flushing data writes in a delayed manner.
+// When SetDirty is called the provided function will be invoked on thread with configured delay.
+// Multiple SetDirty calls may produce just one flush, delay based on the last call.
+
+namespace Common
+{
+
+class FlushThread final
+{
+public:
+  FlushThread() = default;
+  explicit FlushThread(std::string name, std::function<void()> func)
+  {
+    Reset(std::move(name), std::move(func));
+  }
+
+  ~FlushThread() { Shutdown(); }
+
+  FlushThread(const FlushThread&) = delete;
+  FlushThread& operator=(const FlushThread&) = delete;
+
+  FlushThread(FlushThread&&) = delete;
+  FlushThread& operator=(FlushThread&&) = delete;
+
+  // May not take effect until clean.
+  void SetFlushDelay(DT delay) { m_flush_delay.store(delay, std::memory_order_relaxed); }
+
+  // (Re)Starts the thread with the provided flush function.
+  // Other state is unchanged.
+  void Reset(std::string name, std::function<void()> func)
+  {
+    Shutdown();
+
+    m_want_shutdown.store(false, std::memory_order_relaxed);
+    m_thread = std::thread{std::bind_front(&FlushThread::ThreadFunc, this), std::move(name),
+                           std::move(func)};
+  }
+
+  // Graceful immediate shutdown. Waits for final flush if necessary.
+  // Does nothing if thread isn't running.
+  void Shutdown()
+  {
+    if (!m_thread.joinable())
+      return;
+
+    WaitForCompletion();
+    m_want_shutdown.store(true, std::memory_order_relaxed);
+    m_event.Set();
+    m_thread.join();
+  }
+
+  void SetDirty()
+  {
+    m_dirty_count.fetch_add(1, std::memory_order_relaxed);
+    m_flush_deadline.store((Clock::now() + m_flush_delay.load(std::memory_order_relaxed)));
+    m_event.Set();
+  }
+
+  // Lets the worker immediately flush if necessary.
+  // Does nothing if thread isn't running.
+  void WaitForCompletion()
+  {
+    if (!m_thread.joinable())
+      return;
+
+    m_run_freely.release();
+
+    m_event.Set();
+    // Wait for m_dirty_count == 0.
+    while (auto old_count = m_dirty_count.load(std::memory_order_acquire))
+      m_dirty_count.wait(old_count, std::memory_order_acquire);
+
+    m_run_freely.acquire();
+  }
+
+private:
+  auto GetDeadline() const { return m_flush_deadline.load(std::memory_order_relaxed); }
+
+  void WaitUntilFlushIsWanted()
+  {
+    while (!m_run_freely.try_acquire_until(GetDeadline()))
+    {
+      if (Clock::now() >= GetDeadline())
+        return;
+    }
+    m_run_freely.release();
+  }
+
+  void ThreadFunc(const std::string& name, const std::function<void()>& flush_func)
+  {
+    Common::SetCurrentThreadName(name.c_str());
+
+    while (true)
+    {
+      m_event.Wait();
+
+      if (m_want_shutdown.load(std::memory_order_relaxed))
+        break;
+
+      WaitUntilFlushIsWanted();
+
+      const auto cleaning_count = m_dirty_count.load(std::memory_order_relaxed);
+      if (cleaning_count != 0)
+      {
+        flush_func();
+        m_dirty_count.fetch_sub(cleaning_count, std::memory_order_release);
+        m_dirty_count.notify_all();
+      }
+    }
+  }
+
+  // Incremented when a flush needs to happen.
+  // Decremented by worker-thread to signal completion.
+  std::atomic<u32> m_dirty_count{};
+
+  std::atomic<DT> m_flush_delay{};
+  std::atomic<TimePoint> m_flush_deadline{};
+
+  // Worker tries to acquire this for the flush delay.
+  // Releasing it lets the worker run without waiting.
+  std::counting_semaphore<> m_run_freely{0};
+
+  std::thread m_thread;
+  Common::Event m_event;
+
+  std::atomic_bool m_want_shutdown{};
+};
+
+}  // namespace Common

Source/Core/Core/HW/GCMemcard/GCMemcardDirectory.cpp

@@ -4,9 +4,7 @@
 #include "Core/HW/GCMemcard/GCMemcardDirectory.h"
 
 #include <algorithm>
-#include <chrono>
 #include <cstring>
-#include <memory>
 #include <mutex>
 #include <string>
 #include <string_view>
@@ -25,8 +23,6 @@
 #include "Common/Logging/Log.h"
 #include "Common/MsgHandler.h"
 #include "Common/StringUtil.h"
-#include "Common/Thread.h"
-#include "Common/Timer.h"
 
 #include "Core/Config/MainSettings.h"
 #include "Core/Config/SessionSettings.h"
@@ -181,8 +177,7 @@ std::vector<std::string> GCMemcardDirectory::GetFileNamesForGameID(const std::st
 GCMemcardDirectory::GCMemcardDirectory(const std::string& directory, ExpansionInterface::Slot slot,
                                        const Memcard::HeaderData& header_data, u32 game_id)
     : MemoryCardBase(slot, header_data.m_size_mb), m_game_id(game_id), m_last_block(-1),
-      m_hdr(header_data), m_bat1(header_data.m_size_mb), m_saves(0), m_save_directory(directory),
-      m_exiting(false)
+      m_hdr(header_data), m_bat1(header_data.m_size_mb), m_saves(0), m_save_directory(directory)
 {
   // Use existing header data if available
   {
@@ -256,44 +251,19 @@ GCMemcardDirectory::GCMemcardDirectory(const std::string& directory, ExpansionIn
   m_dir2 = m_dir1;
   m_bat2 = m_bat1;
 
-  m_flush_thread = std::thread(&GCMemcardDirectory::FlushThread, this);
-}
-
-void GCMemcardDirectory::FlushThread()
+  if (Config::Get(Config::SESSION_SAVE_DATA_WRITABLE))
   {
-  if (!Config::Get(Config::SESSION_SAVE_DATA_WRITABLE))
-  {
-    return;
-  }
+    m_flush_thread.Reset(fmt::format("Memcard {} flushing thread", m_card_slot),
+                         std::bind_front(&GCMemcardDirectory::FlushToFile, this));
 
-  Common::SetCurrentThreadName(fmt::format("Memcard {} flushing thread", m_card_slot).c_str());
-
-  constexpr std::chrono::seconds flush_interval{1};
-  while (true)
-  {
-    // no-op until signalled
-    m_flush_trigger.Wait();
-
-    if (m_exiting.TestAndClear())
-      return;
-    // no-op as long as signalled within flush_interval
-    while (m_flush_trigger.WaitFor(flush_interval))
-    {
-      if (m_exiting.TestAndClear())
-        return;
-    }
-
-    FlushToFile();
+    m_flush_thread.SetFlushDelay(std::chrono::seconds{1});
   }
 }
 
 GCMemcardDirectory::~GCMemcardDirectory()
 {
-  m_exiting.Set();
-  m_flush_trigger.Set();
-  m_flush_thread.join();
-
-  FlushToFile();
+  // Trigger one more flush on Shutdown since Write doesn't always SetDirty.
+  m_flush_thread.SetDirty();
 }
 
 s32 GCMemcardDirectory::Read(u32 src_address, s32 length, u8* dest_address)
@@ -417,7 +387,7 @@ s32 GCMemcardDirectory::Write(u32 dest_address, s32 length, const u8* src_addres
   if (extra)
     extra = Write(dest_address + length, extra, src_address + length);
   if (offset + length == Memcard::BLOCK_SIZE)
-    m_flush_trigger.Set();
+    m_flush_thread.SetDirty();
   return length + extra;
 }
 
@@ -620,7 +590,7 @@ bool GCMemcardDirectory::SetUsedBlocks(int save_index)
 
 void GCMemcardDirectory::FlushToFile()
 {
-  std::unique_lock l(m_write_mutex);
+  std::lock_guard lk{m_write_mutex};
   Memcard::DEntry invalid;
   for (Memcard::GCIFile& save : m_saves)
   {
@@ -719,7 +689,7 @@ void GCMemcardDirectory::FlushToFile()
 
 void GCMemcardDirectory::DoState(PointerWrap& p)
 {
-  std::unique_lock l(m_write_mutex);
+  std::lock_guard lk{m_write_mutex};
   m_last_block = -1;
   m_last_block_address = nullptr;
   p.Do(m_save_directory);
@@ -735,10 +705,10 @@ void MigrateFromMemcardFile(const std::string& directory_name, ExpansionInterfac
                             DiscIO::Region region)
 {
   File::CreateFullPath(directory_name);
-  const std::string ini_memcard = Config::GetMemcardPath(card_slot, region);
+  std::string ini_memcard = Config::GetMemcardPath(card_slot, region);
   if (File::Exists(ini_memcard))
   {
-    auto [error_code, memcard] = Memcard::GCMemcard::Open(ini_memcard.c_str());
+    auto [error_code, memcard] = Memcard::GCMemcard::Open(std::move(ini_memcard));
     if (!error_code.HasCriticalErrors() && memcard && memcard->IsValid())
     {
       for (u8 i = 0; i < Memcard::DIRLEN; i++)

Source/Core/Core/HW/GCMemcard/GCMemcardDirectory.h

@@ -5,10 +5,9 @@
 
 #include <mutex>
 #include <string>
-#include <thread>
 #include <vector>
 
-#include "Common/Event.h"
+#include "Common/FlushThread.h"
 #include "Core/HW/GCMemcard/GCIFile.h"
 #include "Core/HW/GCMemcard/GCMemcard.h"
 #include "Core/HW/GCMemcard/GCMemcardBase.h"
@@ -19,12 +18,12 @@
 void MigrateFromMemcardFile(const std::string& directory_name, ExpansionInterface::Slot card_slot,
                             DiscIO::Region region);
 
-class GCMemcardDirectory : public MemoryCardBase
+class GCMemcardDirectory final : public MemoryCardBase
 {
 public:
   GCMemcardDirectory(const std::string& directory, ExpansionInterface::Slot slot,
                      const Memcard::HeaderData& header_data, u32 game_id);
-  ~GCMemcardDirectory();
+  ~GCMemcardDirectory() override;
 
   GCMemcardDirectory(const GCMemcardDirectory&) = delete;
   GCMemcardDirectory& operator=(const GCMemcardDirectory&) = delete;
@@ -33,8 +32,6 @@ public:
 
   static std::vector<std::string> GetFileNamesForGameID(const std::string& directory,
                                                         const std::string& game_id);
-  void FlushToFile();
-  void FlushThread();
   s32 Read(u32 src_address, s32 length, u8* dest_address) override;
   s32 Write(u32 dest_address, s32 length, const u8* src_address) override;
   void ClearBlock(u32 address) override;
@@ -42,6 +39,7 @@ public:
   void DoState(PointerWrap& p) override;
 
 private:
+  void FlushToFile();
   bool LoadGCI(Memcard::GCIFile gci);
   inline s32 SaveAreaRW(u32 block, bool writing = false);
   // s32 DirectoryRead(u32 offset, u32 length, u8* dest_address);
@@ -61,8 +59,6 @@ private:
   std::vector<Memcard::GCIFile> m_saves;
 
   std::string m_save_directory;
-  Common::Event m_flush_trigger;
   std::mutex m_write_mutex;
-  Common::Flag m_exiting;
-  std::thread m_flush_thread;
+  Common::FlushThread m_flush_thread;
 };

Source/Core/Core/HW/GCMemcard/GCMemcardRaw.cpp

@@ -8,19 +8,16 @@
 #include <memory>
 #include <mutex>
 #include <string>
-#include <thread>
 
 #include <fmt/format.h>
 
 #include "Common/ChunkFile.h"
-#include "Common/CommonPaths.h"
 #include "Common/CommonTypes.h"
 #include "Common/FileUtil.h"
 #include "Common/IOFile.h"
 #include "Common/Logging/Log.h"
 #include "Common/MsgHandler.h"
 #include "Common/StringUtil.h"
-#include "Common/Thread.h"
 #include "Common/Timer.h"
 
 #include "Core/Config/SessionSettings.h"
@@ -35,11 +32,10 @@
 #define SIZE_TO_Mb (1024 * 8 * 16)
 #define MC_HDR_SIZE 0xA000
 
-MemoryCard::MemoryCard(const std::string& filename, ExpansionInterface::Slot card_slot,
-                       u16 size_mbits)
-    : MemoryCardBase(card_slot, size_mbits), m_filename(filename)
+MemoryCard::MemoryCard(std::string filename, ExpansionInterface::Slot card_slot, u16 size_mbits)
+    : MemoryCardBase(card_slot, size_mbits)
 {
-  File::IOFile file(m_filename, "rb");
+  File::IOFile file(filename, "rb");
   if (file)
   {
     // Measure size of the existing memcard file.
@@ -48,7 +44,7 @@ MemoryCard::MemoryCard(const std::string& filename, ExpansionInterface::Slot car
     m_memcard_data = std::make_unique<u8[]>(m_memory_card_size);
     memset(&m_memcard_data[0], 0xFF, m_memory_card_size);
 
-    INFO_LOG_FMT(EXPANSIONINTERFACE, "Reading memory card {}", m_filename);
+    INFO_LOG_FMT(EXPANSIONINTERFACE, "Reading memory card {}", filename);
     file.ReadBytes(&m_memcard_data[0], m_memory_card_size);
   }
   else
@@ -62,7 +58,7 @@ MemoryCard::MemoryCard(const std::string& filename, ExpansionInterface::Slot car
     // Fills in the first 5 blocks (MC_HDR_SIZE bytes)
     auto& sram = Core::System::GetInstance().GetSRAM();
     const CardFlashId& flash_id = sram.settings_ex.flash_id[Memcard::SLOT_A];
-    const bool shift_jis = m_filename.find(".JAP.raw") != std::string::npos;
+    const bool shift_jis = filename.find(".JAP.raw") != std::string::npos;
     const u32 rtc_bias = sram.settings.rtc_bias;
     const u32 sram_language = static_cast<u32>(sram.settings.language);
     const u64 format_time =
@@ -76,60 +72,32 @@ MemoryCard::MemoryCard(const std::string& filename, ExpansionInterface::Slot car
     INFO_LOG_FMT(EXPANSIONINTERFACE, "No memory card found. A new one was created instead.");
   }
 
-  // Class members (including inherited ones) have now been initialized, so
-  // it's safe to startup the flush thread (which reads them).
   m_flush_buffer = std::make_unique<u8[]>(m_memory_card_size);
-  m_flush_thread = std::thread(&MemoryCard::FlushThread, this);
-}
 
-MemoryCard::~MemoryCard()
+  if (Config::Get(Config::SESSION_SAVE_DATA_WRITABLE))
   {
-  if (m_flush_thread.joinable())
-  {
-    m_flush_trigger.Set();
+    m_flush_thread.Reset(fmt::format("Memcard {} flushing thread", m_card_slot),
+                         std::bind_front(&MemoryCard::FlushToFile, this, std::move(filename)));
 
-    m_flush_thread.join();
+    m_flush_thread.SetFlushDelay(std::chrono::seconds{1});
   }
 }
 
-void MemoryCard::FlushThread()
-{
-  if (!Config::Get(Config::SESSION_SAVE_DATA_WRITABLE))
-  {
-    return;
-  }
+MemoryCard::~MemoryCard() = default;
 
-  Common::SetCurrentThreadName(fmt::format("Memcard {} flushing thread", m_card_slot).c_str());
-
-  const auto flush_interval = std::chrono::seconds(15);
-
-  while (true)
+void MemoryCard::FlushToFile(const std::string& filename)
 {
-    // If triggered, we're exiting.
-    // If timed out, check if we need to flush.
-    bool do_exit = m_flush_trigger.WaitFor(flush_interval);
-    if (!do_exit)
-    {
-      bool is_dirty = m_dirty.TestAndClear();
-      if (!is_dirty)
-      {
-        continue;
-      }
-    }
-
-    // Opening the file is purposefully done each iteration to ensure the
-    // file doesn't disappear out from under us after the first check.
-    File::IOFile file(m_filename, "r+b");
+  File::IOFile file(filename, "r+b");
 
   if (!file)
   {
     std::string dir;
-      SplitPath(m_filename, &dir, nullptr, nullptr);
+    SplitPath(filename, &dir, nullptr, nullptr);
     if (!File::IsDirectory(dir))
     {
       File::CreateFullPath(dir);
     }
-      file.Open(m_filename, "wb");
+    file.Open(filename, "wb");
   }
 
   // Note - file may have changed above, after ctor
@@ -140,10 +108,9 @@ void MemoryCard::FlushThread()
         "Are you running Dolphin from a CD/DVD, or is the save file maybe write protected?\n\n"
         "Are you receiving this after moving the emulator directory?\nIf so, then you may "
         "need to re-specify your memory card location in the options.",
-          m_filename);
+        filename);
 
-      // Exit the flushing thread - further flushes will be ignored unless
-      // the thread is recreated.
+    // This flush is unsuccessful.
     return;
   }
 
@@ -153,18 +120,14 @@ void MemoryCard::FlushThread()
   }
   file.WriteBytes(&m_flush_buffer[0], m_memory_card_size);
 
-    if (do_exit)
-      return;
-
   Core::DisplayMessage(fmt::format("Wrote to Memory Card {}",
                                    m_card_slot == ExpansionInterface::Slot::A ? 'A' : 'B'),
                        4000);
 }
-}
 
 void MemoryCard::MakeDirty()
 {
-  m_dirty.Set();
+  m_flush_thread.SetDirty();
 }
 
 s32 MemoryCard::Read(u32 src_address, s32 length, u8* dest_address)
@@ -203,7 +166,7 @@ void MemoryCard::ClearBlock(u32 address)
     PanicAlertFmtT("MemoryCard: ClearBlock called on invalid address ({0:#x})", address);
     return;
   }
-  else
+
   {
     std::unique_lock l(m_flush_mutex);
     memset(&m_memcard_data[address], 0xFF, Memcard::BLOCK_SIZE);

Source/Core/Core/HW/GCMemcard/GCMemcardRaw.h

@@ -6,22 +6,19 @@
 #include <memory>
 #include <mutex>
 #include <string>
-#include <thread>
-#include "Common/Event.h"
-#include "Common/Flag.h"
+
+#include "Common/FlushThread.h"
 #include "Core/HW/GCMemcard/GCMemcard.h"
 #include "Core/HW/GCMemcard/GCMemcardBase.h"
 
 class PointerWrap;
 
-class MemoryCard : public MemoryCardBase
+class MemoryCard final : public MemoryCardBase
 {
 public:
-  MemoryCard(const std::string& filename, ExpansionInterface::Slot card_slot,
+  MemoryCard(std::string filename, ExpansionInterface::Slot card_slot,
              u16 size_mbits = Memcard::MBIT_SIZE_MEMORY_CARD_2043);
-  ~MemoryCard();
-  void FlushThread();
-  void MakeDirty();
+  ~MemoryCard() override;
 
   s32 Read(u32 src_address, s32 length, u8* dest_address) override;
   s32 Write(u32 dest_address, s32 length, const u8* src_address) override;
@@ -30,18 +27,18 @@ public:
   void DoState(PointerWrap& p) override;
 
 private:
+  void FlushToFile(const std::string& filename);
+  void MakeDirty();
+
   bool IsAddressInBounds(u32 address, u32 length) const
   {
     u64 end_address = static_cast<u64>(address) + static_cast<u64>(length);
     return end_address <= static_cast<u64>(m_memory_card_size);
   }
 
-  std::string m_filename;
   std::unique_ptr<u8[]> m_memcard_data;
   std::unique_ptr<u8[]> m_flush_buffer;
-  std::thread m_flush_thread;
+  Common::FlushThread m_flush_thread;
   std::mutex m_flush_mutex;
-  Common::Event m_flush_trigger;
-  Common::Flag m_dirty;
   u32 m_memory_card_size;
 };

Source/Core/DolphinLib.props

@@ -60,6 +60,7 @@
     <ClInclude Include="Common\FixedSizeQueue.h" />
     <ClInclude Include="Common\Flag.h" />
     <ClInclude Include="Common\FloatUtils.h" />
+    <ClInclude Include="Common\FlushThread.h" />
     <ClInclude Include="Common\FormatUtil.h" />
     <ClInclude Include="Common\FPURoundMode.h" />
     <ClInclude Include="Common\GekkoDisassembler.h" />

Source/UnitTests/Common/CMakeLists.txt

@@ -13,6 +13,7 @@ add_dolphin_test(FileUtilTest FileUtilTest.cpp)
 add_dolphin_test(FixedSizeQueueTest FixedSizeQueueTest.cpp)
 add_dolphin_test(FlagTest FlagTest.cpp)
 add_dolphin_test(FloatUtilsTest FloatUtilsTest.cpp)
+add_dolphin_test(FlushThreadTest FlushThreadTest.cpp)
 add_dolphin_test(MathUtilTest MathUtilTest.cpp)
 add_dolphin_test(NandPathsTest NandPathsTest.cpp)
 add_dolphin_test(SettingsHandlerTest SettingsHandlerTest.cpp)

Source/UnitTests/Common/FlushThreadTest.cpp

@@ -0,0 +1,79 @@
+// Copyright 2025 Dolphin Emulator Project
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include <gtest/gtest.h>
+
+#include <atomic>
+
+#include "Common/CommonTypes.h"
+#include "Common/FlushThread.h"
+
+TEST(FlushThread, Simple)
+{
+  Common::FlushThread ft;
+
+  std::atomic<int> value = 0;
+
+  ft.Reset("flush", [&] { ++value; });
+
+  // No flush on start.
+  EXPECT_EQ(value.load(), 0);
+
+  ft.SetDirty();
+  ft.WaitForCompletion();
+
+  // One flush.
+  EXPECT_EQ(value.load(), 1);
+
+  ft.Reset("flush", [&] { ++value; });
+
+  // No change after reset.
+  EXPECT_EQ(value.load(), 1);
+
+  ft.Shutdown();
+  ft.SetDirty();
+  ft.WaitForCompletion();
+
+  // No change because shutdown.
+  EXPECT_EQ(value.load(), 1);
+
+  ft.Reset("flush", [&] {
+    ++value;
+    value.notify_one();
+  });
+  ft.WaitForCompletion();
+
+  // Dirty state persits on reset.
+  EXPECT_EQ(value.load(), 2);
+
+  value = 0;
+
+  ft.SetFlushDelay(std::chrono::milliseconds{999999});
+  ft.SetDirty();
+  ft.SetDirty();
+  ft.SetDirty();
+
+  // Not using EXPECT_ here because the tests are technically racey.
+
+  // Probably no flush yet, because of the delay.
+  GTEST_LOG_(INFO) << "Ideally 0: " << value.load();
+
+  const auto start = std::chrono::steady_clock::now();
+  ft.WaitForCompletion();
+  const auto end = std::chrono::steady_clock::now();
+
+  GTEST_LOG_(INFO) << "Ideally 0: "
+                   << duration_cast<std::chrono::milliseconds>(end - start).count();
+
+  // At least one flush happened. Probably just one.
+  EXPECT_GT(value.load(), 0);
+  GTEST_LOG_(INFO) << "Ideally 1: " << value.load();
+
+  value = 0;
+
+  ft.SetDirty();
+  ft.Reset("flush", [] {});
+
+  // Reset first causes a shutdown, so we have an additional immediate flush.
+  EXPECT_EQ(value.load(), 1);
+}

Source/UnitTests/UnitTests.vcxproj

@@ -52,6 +52,7 @@
     <ClCompile Include="Common\FixedSizeQueueTest.cpp" />
     <ClCompile Include="Common\FlagTest.cpp" />
     <ClCompile Include="Common\FloatUtilsTest.cpp" />
+    <ClCompile Include="Common\FlushThreadTest.cpp" />
     <ClCompile Include="Common\MathUtilTest.cpp" />
     <ClCompile Include="Common\NandPathsTest.cpp" />
     <ClCompile Include="Common\SettingsHandlerTest.cpp" />