From a001e6ef09320f340ab8c9c5d87f176b288262b4 Mon Sep 17 00:00:00 2001 From: Eladash Date: Mon, 21 Aug 2023 19:14:21 +0300 Subject: [PATCH] Progress Dialog: Fix race on PPU compilation status --- rpcs3/Emu/Cell/PPUThread.cpp | 6 +-- rpcs3/Emu/Cell/SPURecompiler.cpp | 2 +- rpcs3/Emu/System.cpp | 2 +- rpcs3/Emu/system_progress.cpp | 10 ++--- rpcs3/Emu/system_progress.hpp | 67 ++++++++++++++++++++++++++++++-- rpcs3/rpcs3qt/gs_frame.cpp | 2 +- 6 files changed, 74 insertions(+), 15 deletions(-) diff --git a/rpcs3/Emu/Cell/PPUThread.cpp b/rpcs3/Emu/Cell/PPUThread.cpp index b0aac9b280..32290dbe26 100644 --- a/rpcs3/Emu/Cell/PPUThread.cpp +++ b/rpcs3/Emu/Cell/PPUThread.cpp @@ -3971,7 +3971,7 @@ extern void ppu_initialize() // Validate analyser results (not required) _main.validate(0); - g_progr = "Scanning PPU Modules..."; + progr = "Scanning PPU Modules..."; bool compile_main = false; @@ -4556,7 +4556,7 @@ bool ppu_initialize(const ppu_module& info, bool check_only) if (!workload.empty()) { - g_progr = "Compiling PPU modules..."; + *progr = "Compiling PPU modules..."; } // Create worker threads for compilation (TODO: how many threads) @@ -4624,7 +4624,7 @@ bool ppu_initialize(const ppu_module& info, bool check_only) if (workload.size() < link_workload.size()) { // Only show this message if this task is relevant - g_progr = "Linking PPU modules..."; + *progr = "Linking PPU modules..."; } for (auto [obj_name, is_compiled] : link_workload) diff --git a/rpcs3/Emu/Cell/SPURecompiler.cpp b/rpcs3/Emu/Cell/SPURecompiler.cpp index dc5ddc657d..952fee9857 100644 --- a/rpcs3/Emu/Cell/SPURecompiler.cpp +++ b/rpcs3/Emu/Cell/SPURecompiler.cpp @@ -654,7 +654,7 @@ void spu_cache::initialize() break; } - g_progr_ptotal.wait(v); + thread_ctrl::wait_on(g_progr_ptotal, v); } g_progr_ptotal += ::size32(func_list); diff --git a/rpcs3/Emu/System.cpp b/rpcs3/Emu/System.cpp index f2c390be7c..e85cc6a6cf 100644 --- a/rpcs3/Emu/System.cpp +++ b/rpcs3/Emu/System.cpp @@ -2678,7 +2678,7 @@ void Emulator::Kill(bool allow_autoexit, bool savestate) { // Show visual feedback to the user in case that stopping takes a while. // This needs to be done before actually stopping, because otherwise the necessary threads will be terminated before we can show an image. - if (auto progress_dialog = g_fxo->try_get>(); progress_dialog && +g_progr) + if (auto progress_dialog = g_fxo->try_get>(); progress_dialog && g_progr.load()) { // We are currently showing a progress dialog. Notify it that we are going to stop emulation. g_system_progress_stopping = true; diff --git a/rpcs3/Emu/system_progress.cpp b/rpcs3/Emu/system_progress.cpp index 3869262349..522dbc79fe 100644 --- a/rpcs3/Emu/system_progress.cpp +++ b/rpcs3/Emu/system_progress.cpp @@ -11,7 +11,7 @@ LOG_CHANNEL(sys_log, "SYS"); // Progress display server synchronization variables -atomic_t g_progr{nullptr}; +atomic_t g_progr{}; atomic_t g_progr_ftotal{0}; atomic_t g_progr_fdone{0}; atomic_t g_progr_ptotal{0}; @@ -40,7 +40,7 @@ void progress_dialog_server::operator()() while (!g_system_progress_stopping && thread_ctrl::state() != thread_state::aborting) { // Wait for the start condition - auto text0 = +g_progr; + const char* text0 = g_progr.load(); while (!text0) { @@ -50,7 +50,7 @@ void progress_dialog_server::operator()() } thread_ctrl::wait_for(5000); - text0 = +g_progr; + text0 = g_progr.load(); } if (g_system_progress_stopping || thread_ctrl::state() == thread_state::aborting) @@ -120,7 +120,7 @@ void progress_dialog_server::operator()() // Update progress while (!g_system_progress_stopping && thread_ctrl::state() != thread_state::aborting) { - const auto text_new = g_progr.load(); + const auto text_new = +g_progr.load(); const u32 ftotal_new = g_progr_ftotal; const u32 fdone_new = g_progr_fdone; @@ -239,5 +239,5 @@ progress_dialog_server::~progress_dialog_server() g_progr_fdone.release(0); g_progr_ptotal.release(0); g_progr_pdone.release(0); - g_progr.release(nullptr); + g_progr.release(progress_dialog_string_t{}); } diff --git a/rpcs3/Emu/system_progress.hpp b/rpcs3/Emu/system_progress.hpp index 92139b4afe..76e8e38ca7 100644 --- a/rpcs3/Emu/system_progress.hpp +++ b/rpcs3/Emu/system_progress.hpp @@ -3,7 +3,19 @@ #include "util/types.hpp" #include "util/atomic.hpp" -extern atomic_t g_progr; +struct alignas(16) progress_dialog_string_t +{ + const char* m_text; + u32 m_user_count; + u32 m_update_id; + + operator const char*() const noexcept + { + return m_text; + } +}; + +extern atomic_t g_progr; extern atomic_t g_progr_ftotal; extern atomic_t g_progr_fdone; extern atomic_t g_progr_ptotal; @@ -15,21 +27,68 @@ extern atomic_t g_system_progress_stopping; class scoped_progress_dialog final { // Saved previous value - const char* const m_prev; + const char* m_prev; + u32 m_prev_id; + u32 m_id; public: scoped_progress_dialog(const char* text) noexcept - : m_prev(g_progr.exchange(text ? text : "")) { + std::tie(m_prev, m_prev_id, m_id) = g_progr.atomic_op([this, text = ensure(text)](progress_dialog_string_t& progr) + { + const char* old = progr.m_text; + progr.m_user_count++; + progr.m_update_id++; + progr.m_text = text; + + ensure(progr.m_user_count > 1 || !old); // Ensure it was nullptr before first use + return std::make_tuple(old, progr.m_update_id - 1, progr.m_update_id); + }); } scoped_progress_dialog(const scoped_progress_dialog&) = delete; scoped_progress_dialog& operator=(const scoped_progress_dialog&) = delete; + scoped_progress_dialog& operator=(const char* text) noexcept + { + // This method is destroying the previous value and replacing it with a new one + std::tie(m_prev, m_prev_id, m_id) = g_progr.atomic_op([this, text = ensure(text)](progress_dialog_string_t& progr) + { + if (m_id == progr.m_update_id) + { + progr.m_update_id = m_prev_id; + progr.m_text = m_prev; + } + + const char* old = progr.m_text; + progr.m_text = text; + progr.m_update_id++; + + ensure(progr.m_user_count > 0); + return std::make_tuple(old, progr.m_update_id - 1, progr.m_update_id); + }); + + return *this; + } + ~scoped_progress_dialog() noexcept { - g_progr.release(m_prev); + g_progr.atomic_op([this](progress_dialog_string_t& progr) + { + if (progr.m_user_count-- == 1) + { + // Clean text only on last user + progr.m_text = nullptr; + progr.m_update_id = 0; + } + else if (m_id == progr.m_update_id) + { + // Restore text only if no other updates were made by other threads + progr.m_text = ensure(m_prev); + progr.m_update_id = m_prev_id; + } + }); } }; diff --git a/rpcs3/rpcs3qt/gs_frame.cpp b/rpcs3/rpcs3qt/gs_frame.cpp index dfc3d6917b..c0c4b60a6d 100644 --- a/rpcs3/rpcs3qt/gs_frame.cpp +++ b/rpcs3/rpcs3qt/gs_frame.cpp @@ -579,7 +579,7 @@ bool gs_frame::get_mouse_lock_state() void gs_frame::hide_on_close() { - if (!(+g_progr)) + if (!g_progr.load()) { // Hide the dialog before stopping if no progress bar is being shown. // Otherwise users might think that the game softlocked if stopping takes too long.