From 68d74bc28a663cff7b03451fc09128ea7c802d69 Mon Sep 17 00:00:00 2001 From: Elad <18193363+elad335@users.noreply.github.com> Date: Fri, 15 Nov 2024 13:25:37 +0200 Subject: [PATCH] Progress Dialog: Fix recursion and concurrency use of text updates --- Utilities/lockless.h | 19 +-- rpcs3/Emu/CMakeLists.txt | 1 + rpcs3/Emu/Cell/PPUThread.cpp | 2 +- rpcs3/Emu/Cell/SPUCommonRecompiler.cpp | 4 +- rpcs3/Emu/System.cpp | 2 +- rpcs3/Emu/scoped_progress_dialog.cpp | 206 +++++++++++++++++++++++++ rpcs3/Emu/system_progress.cpp | 31 ++-- rpcs3/Emu/system_progress.hpp | 100 ++++-------- rpcs3/emucore.vcxproj | 1 + rpcs3/emucore.vcxproj.filters | 3 + rpcs3/rpcs3qt/gs_frame.cpp | 2 +- 11 files changed, 273 insertions(+), 98 deletions(-) create mode 100644 rpcs3/Emu/scoped_progress_dialog.cpp diff --git a/Utilities/lockless.h b/Utilities/lockless.h index 7e10e107b8..95121e1084 100644 --- a/Utilities/lockless.h +++ b/Utilities/lockless.h @@ -4,12 +4,13 @@ #include "util/atomic.hpp" #include "util/bless.hpp" -//! Simple unshrinkable array base for concurrent access. Only growths automatically. -//! There is no way to know the current size. The smaller index is, the faster it's accessed. -//! -//! T is the type of elements. Currently, default constructor of T shall be constexpr. -//! N is initial element count, available without any memory allocation and only stored contiguously. -template +// Simple unshrinkable array base for concurrent access. Only growths automatically. +// There is no way to know the current size. The smaller index is, the faster it's accessed. +// +// T is the type of elements. Currently, default constructor of T shall be constexpr. +// N is initial element count, available without any memory allocation and only stored contiguously. +// Let's have around 256 bytes or less worth of preallocated elements +template (256 / sizeof(T), 1)> class lf_array { // Data (default-initialized) @@ -137,9 +138,9 @@ public: } }; -//! Simple lock-free FIFO queue base. Based on lf_array itself. Currently uses 32-bit counters. -//! There is no "push_end" or "pop_begin" provided, the queue element must signal its state on its own. -template +// Simple lock-free FIFO queue base. Based on lf_array itself. Currently uses 32-bit counters. +// There is no "push_end" or "pop_begin" provided, the queue element must signal its state on its own. +template(256 / sizeof(T), 1)> class lf_fifo : public lf_array { // LSB 32-bit: push, MSB 32-bit: pop diff --git a/rpcs3/Emu/CMakeLists.txt b/rpcs3/Emu/CMakeLists.txt index 2d151341c3..8fb53caae8 100644 --- a/rpcs3/Emu/CMakeLists.txt +++ b/rpcs3/Emu/CMakeLists.txt @@ -4,6 +4,7 @@ add_library(rpcs3_emu STATIC IdManager.cpp localized_string.cpp savestate_utils.cpp + scoped_progress_dialog.cpp System.cpp system_config.cpp system_config_types.cpp diff --git a/rpcs3/Emu/Cell/PPUThread.cpp b/rpcs3/Emu/Cell/PPUThread.cpp index d921603c1f..bc8c55910a 100644 --- a/rpcs3/Emu/Cell/PPUThread.cpp +++ b/rpcs3/Emu/Cell/PPUThread.cpp @@ -5143,7 +5143,7 @@ bool ppu_initialize(const ppu_module& info, bool check_only, u64 file_size) // Try to patch all single and unregistered BLRs with the same function (TODO: Maybe generalize it into PIC code detection and patching) ppu_intrp_func_t BLR_func = nullptr; - const bool showing_only_apply_stage = !g_progr_text.load() && !g_progr_ptotal && !g_progr_ftotal && g_progr_ptotal.compare_and_swap_test(0, 1); + const bool showing_only_apply_stage = !g_progr_text.operator bool() && !g_progr_ptotal && !g_progr_ftotal && g_progr_ptotal.compare_and_swap_test(0, 1); progress_dialog = get_localized_string(localized_string_id::PROGRESS_DIALOG_APPLYING_PPU_CODE); diff --git a/rpcs3/Emu/Cell/SPUCommonRecompiler.cpp b/rpcs3/Emu/Cell/SPUCommonRecompiler.cpp index e987d701df..698e5bd178 100644 --- a/rpcs3/Emu/Cell/SPUCommonRecompiler.cpp +++ b/rpcs3/Emu/Cell/SPUCommonRecompiler.cpp @@ -938,7 +938,7 @@ void spu_cache::initialize(bool build_existing_cache) if (is_first_thread && !showing_progress) { - if (!g_progr_text.load() && !g_progr_ptotal && !g_progr_ftotal) + if (!g_progr_text && !g_progr_ptotal && !g_progr_ftotal) { showing_progress = true; g_progr_pdone += pending_progress.exchange(0); @@ -1115,7 +1115,7 @@ void spu_cache::initialize(bool build_existing_cache) if (is_first_thread && !showing_progress) { - if (!g_progr_text.load() && !g_progr_ptotal && !g_progr_ftotal) + if (!g_progr_text && !g_progr_ptotal && !g_progr_ftotal) { showing_progress = true; g_progr_pdone += pending_progress.exchange(0); diff --git a/rpcs3/Emu/System.cpp b/rpcs3/Emu/System.cpp index c5db0d8891..96f4df324e 100644 --- a/rpcs3/Emu/System.cpp +++ b/rpcs3/Emu/System.cpp @@ -3111,7 +3111,7 @@ void Emulator::Kill(bool allow_autoexit, bool savestate, savestate_stage* save_s { // 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_text.load()) + if (auto progress_dialog = g_fxo->try_get>(); progress_dialog && g_progr_text.operator bool()) { // 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/scoped_progress_dialog.cpp b/rpcs3/Emu/scoped_progress_dialog.cpp new file mode 100644 index 0000000000..a1775d5d18 --- /dev/null +++ b/rpcs3/Emu/scoped_progress_dialog.cpp @@ -0,0 +1,206 @@ +#include "stdafx.h" +#include "system_progress.hpp" + +shared_ptr progress_dialog_string_t::get_string_ptr() const noexcept +{ + shared_ptr text; + data_t old_val = data.load(); + + while (true) + { + text.reset(); + + if (old_val.text_index != umax) + { + if (auto ptr = g_progr_text_queue[old_val.text_index].load()) + { + text = ptr; + } + } + + auto new_val = data.load(); + + if (old_val.text_index == new_val.text_index) + { + break; + } + + old_val = new_val; + } + + return text; +} + +scoped_progress_dialog::scoped_progress_dialog(std::string text) noexcept +{ + shared_ptr installed_ptr = make_single_value(std::move(text)); + const shared_ptr c_null_ptr; + + for (usz init_size = g_progr_text_queue.size(), new_text_index = std::max(init_size, 1) - 1;;) + { + if (new_text_index >= init_size) + { + // Search element in new memeory + new_text_index++; + } + else if (new_text_index == 0) + { + // Search element in new memeory + new_text_index = init_size; + } + else + { + new_text_index--; + } + + auto& info = g_progr_text_queue[new_text_index]; + + if (!info && info.compare_and_swap_test(c_null_ptr, installed_ptr)) + { + m_text_index = new_text_index; + break; + } + } + + usz unmap_index = umax; + + g_progr_text.data.atomic_op([&](std::common_type_t& progr) + { + unmap_index = progr.text_index; + progr.update_id++; + progr.text_index = m_text_index; + }); + + // Note: unmap_index may be m_text_index if picked up at the destructor + // Which is technically the newest value to be inserted so it serves its logic + if (unmap_index != umax && unmap_index != m_text_index) + { + g_progr_text_queue[unmap_index].reset(); + } +} + +scoped_progress_dialog& scoped_progress_dialog::operator=(std::string text) noexcept +{ + shared_ptr installed_ptr = make_single_value(std::move(text)); + const shared_ptr c_null_ptr; + + for (usz init_size = g_progr_text_queue.size(), new_text_index = std::max(init_size, 1) - 1;;) + { + if (new_text_index >= init_size) + { + // Search element in new memeory + new_text_index++; + } + else if (new_text_index == 0) + { + // Search element in new memeory + new_text_index = init_size; + } + else + { + new_text_index--; + } + + auto& info = g_progr_text_queue[new_text_index]; + + if (!info && info.compare_and_swap_test(c_null_ptr, installed_ptr)) + { + m_text_index = new_text_index; + break; + } + } + + usz unmap_index = umax; + + g_progr_text.data.atomic_op([&](std::common_type_t& progr) + { + unmap_index = progr.text_index; + progr.update_id++; + progr.text_index = m_text_index; + }); + + // Note: unmap_index may be m_text_index if picked up at the destructor + // Which is technically the newest value to be inserted so it serves its logic + if (unmap_index != umax && unmap_index != m_text_index) + { + g_progr_text_queue[unmap_index].reset(); + } + + return *this; +} + +scoped_progress_dialog::~scoped_progress_dialog() noexcept +{ + bool unmap = false; + + while (true) + { + auto progr = g_progr_text.data.load(); + usz restored_text_index = umax; + + if (progr.text_index == m_text_index) + { + // Search for available text + // Out of scope of atomic to keep atomic_op as clean as possible (for potential future enhacements) + const u64 queue_size = g_progr_text_queue.size(); + + for (u64 i = queue_size - 1;; i--) + { + if (i == umax) + { + restored_text_index = umax; + break; + } + + if (i == m_text_index) + { + continue; + } + + if (g_progr_text_queue[i].operator bool()) + { + restored_text_index = i; + break; + } + } + } + + if (!g_progr_text.data.atomic_op([&](std::common_type_t& progr0) + { + if (progr.update_id != progr0.update_id) + { + // Repeat the loop + return false; + } + + unmap = false; + + if (progr0.text_index == m_text_index) + { + unmap = true; + + if (restored_text_index == umax) + { + progr0.text_index = umax; + progr0.update_id = 0; + return true; + } + + progr0.text_index = restored_text_index; + progr0.update_id++; + } + + return true; + })) + { + continue; + } + + break; + } + + if (unmap) + { + g_progr_text_queue[m_text_index].reset(); + } +} diff --git a/rpcs3/Emu/system_progress.cpp b/rpcs3/Emu/system_progress.cpp index 834b2c182a..feeaea352d 100644 --- a/rpcs3/Emu/system_progress.cpp +++ b/rpcs3/Emu/system_progress.cpp @@ -13,7 +13,8 @@ LOG_CHANNEL(sys_log, "SYS"); // Progress display server synchronization variables -atomic_t g_progr_text{}; +lf_array> g_progr_text_queue; +progress_dialog_string_t g_progr_text{}; atomic_t g_progr_ftotal{0}; atomic_t g_progr_fdone{0}; atomic_t g_progr_ftotal_bits{0}; @@ -43,11 +44,11 @@ void progress_dialog_server::operator()() const auto get_state = []() { - auto whole_state = std::make_tuple(+g_progr_text.load(), +g_progr_ftotal, +g_progr_fdone, +g_progr_ftotal_bits, +g_progr_fknown_bits, +g_progr_ptotal, +g_progr_pdone); + auto whole_state = std::make_tuple(g_progr_text.operator std::string(), +g_progr_ftotal, +g_progr_fdone, +g_progr_ftotal_bits, +g_progr_fknown_bits, +g_progr_ptotal, +g_progr_pdone); while (true) { - auto new_state = std::make_tuple(+g_progr_text.load(), +g_progr_ftotal, +g_progr_fdone, +g_progr_ftotal_bits, +g_progr_fknown_bits, +g_progr_ptotal, +g_progr_pdone); + auto new_state = std::make_tuple(g_progr_text.operator std::string(), +g_progr_ftotal, +g_progr_fdone, +g_progr_ftotal_bits, +g_progr_fknown_bits, +g_progr_ptotal, +g_progr_pdone); if (new_state == whole_state) { @@ -64,9 +65,9 @@ void progress_dialog_server::operator()() while (!g_system_progress_stopping && thread_ctrl::state() != thread_state::aborting) { // Wait for the start condition - const char* text0 = g_progr_text.load(); + std::string text0 = g_progr_text; - while (!text0) + while (text0.empty()) { if (g_system_progress_stopping || thread_ctrl::state() == thread_state::aborting) { @@ -75,9 +76,9 @@ void progress_dialog_server::operator()() if (g_progr_ftotal || g_progr_fdone || g_progr_ptotal || g_progr_pdone) { - const auto& [text_new, ftotal, fdone, ftotal_bits, fknown_bits, ptotal, pdone] = get_state(); + const auto [text_new, ftotal, fdone, ftotal_bits, fknown_bits, ptotal, pdone] = get_state(); - if (text_new) + if (!text_new.empty()) { text0 = text_new; break; @@ -97,7 +98,7 @@ void progress_dialog_server::operator()() } thread_ctrl::wait_for(5000); - text0 = g_progr_text.load(); + text0 = g_progr_text; } if (g_system_progress_stopping || thread_ctrl::state() == thread_state::aborting) @@ -164,7 +165,7 @@ void progress_dialog_server::operator()() u64 ftotal_bits = 0; u32 ptotal = 0; u32 pdone = 0; - const char* text1 = nullptr; + std::string text1; const u64 start_time = get_system_time(); u64 wait_no_update_count = 0; @@ -179,7 +180,7 @@ void progress_dialog_server::operator()() !g_system_progress_stopping && thread_ctrl::state() != thread_state::aborting; thread_ctrl::wait_until(&sleep_until, std::exchange(sleep_for, 500))) { - const auto& [text_new, ftotal_new, fdone_new, ftotal_bits_new, fknown_bits_new, ptotal_new, pdone_new] = get_state(); + const auto [text_new, ftotal_new, fdone_new, ftotal_bits_new, fknown_bits_new, ptotal_new, pdone_new] = get_state(); // Force-update every 20 seconds to update remaining time if (wait_no_update_count == 100u * 20 || ftotal != ftotal_new || fdone != fdone_new || fknown_bits != fknown_bits_new @@ -193,14 +194,14 @@ void progress_dialog_server::operator()() ptotal = ptotal_new; pdone = pdone_new; - const bool text_changed = text_new && text_new != text1; + const bool text_changed = !text_new.empty() && text_new != text1; - if (text_new) + if (!text_new.empty()) { text1 = text_new; } - if (!text1) + if (text1.empty()) { // Cannot do anything continue; @@ -363,7 +364,7 @@ void progress_dialog_server::operator()() } // Leave only if total count is equal to done count - if (ftotal == fdone && ptotal == pdone && !text_new) + if (ftotal == fdone && ptotal == pdone && text_new.empty()) { // Complete state, empty message: close dialog break; @@ -429,5 +430,5 @@ progress_dialog_server::~progress_dialog_server() g_progr_fknown_bits.release(0); g_progr_ptotal.release(0); g_progr_pdone.release(0); - g_progr_text.release(progress_dialog_string_t{}); + g_progr_text.data.release(std::common_type_t{}); } diff --git a/rpcs3/Emu/system_progress.hpp b/rpcs3/Emu/system_progress.hpp index 59dad4898f..98fe854875 100644 --- a/rpcs3/Emu/system_progress.hpp +++ b/rpcs3/Emu/system_progress.hpp @@ -2,20 +2,40 @@ #include "util/types.hpp" #include "util/atomic.hpp" +#include "Utilities/lockless.h" +#include "util/shared_ptr.hpp" + +extern lf_array> g_progr_text_queue; struct alignas(16) progress_dialog_string_t { - const char* m_text; - u32 m_user_count; - u32 m_update_id; - - operator const char*() const noexcept + struct alignas(16) data_t { - return m_text; + usz update_id = 0; + usz text_index = umax; + }; + + atomic_t data{}; + + shared_ptr get_string_ptr() const noexcept; + + operator std::string() const noexcept + { + if (shared_ptr ptr = get_string_ptr()) + { + return *ptr; + } + + return {}; + } + + explicit operator bool() const noexcept + { + return get_string_ptr().operator bool(); } }; -extern atomic_t g_progr_text; +extern progress_dialog_string_t g_progr_text; extern atomic_t g_progr_ftotal; extern atomic_t g_progr_fdone; extern atomic_t g_progr_ftotal_bits; @@ -29,76 +49,18 @@ extern atomic_t g_system_progress_stopping; class scoped_progress_dialog final { private: - std::string m_text; // Saved current value - std::string m_prev; // Saved previous value - u32 m_prev_id; - u32 m_id; + usz m_text_index = 0; public: - scoped_progress_dialog(const std::string& text) noexcept - { - ensure(!text.empty()); - m_text = text; - - std::tie(m_prev, m_prev_id, m_id) = g_progr_text.atomic_op([this](progress_dialog_string_t& progr) - { - std::string old = progr.m_text ? progr.m_text : std::string(); - progr.m_user_count++; - progr.m_update_id++; - progr.m_text = m_text.c_str(); - - ensure(progr.m_user_count > 1 || old.empty()); // Ensure it was empty before first use - return std::make_tuple(std::move(old), progr.m_update_id - 1, progr.m_update_id); - }); - } + scoped_progress_dialog(std::string text) noexcept; scoped_progress_dialog(const scoped_progress_dialog&) = delete; scoped_progress_dialog& operator=(const scoped_progress_dialog&) = delete; - scoped_progress_dialog& operator=(const std::string& text) noexcept - { - ensure(!text.empty()); - m_text = text; + scoped_progress_dialog& operator=(std::string 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_text.atomic_op([this](progress_dialog_string_t& progr) - { - if (m_id == progr.m_update_id) - { - progr.m_update_id = m_prev_id; - progr.m_text = m_prev.c_str(); - } - - std::string old = progr.m_text ? progr.m_text : std::string(); - progr.m_text = m_text.c_str(); - progr.m_update_id++; - - ensure(progr.m_user_count > 0); - return std::make_tuple(std::move(old), progr.m_update_id - 1, progr.m_update_id); - }); - - return *this; - } - - ~scoped_progress_dialog() noexcept - { - g_progr_text.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 = m_prev.c_str(); - progr.m_update_id = m_prev_id; - } - }); - } + ~scoped_progress_dialog() noexcept; }; struct progress_dialog_server diff --git a/rpcs3/emucore.vcxproj b/rpcs3/emucore.vcxproj index 580a403653..517fc2833b 100644 --- a/rpcs3/emucore.vcxproj +++ b/rpcs3/emucore.vcxproj @@ -141,6 +141,7 @@ + diff --git a/rpcs3/emucore.vcxproj.filters b/rpcs3/emucore.vcxproj.filters index 7a2bf3ed16..8b661191e5 100644 --- a/rpcs3/emucore.vcxproj.filters +++ b/rpcs3/emucore.vcxproj.filters @@ -1138,6 +1138,9 @@ Emu + + Emu + Emu\Cell\Modules diff --git a/rpcs3/rpcs3qt/gs_frame.cpp b/rpcs3/rpcs3qt/gs_frame.cpp index ede79d50f3..dd94f41857 100644 --- a/rpcs3/rpcs3qt/gs_frame.cpp +++ b/rpcs3/rpcs3qt/gs_frame.cpp @@ -568,7 +568,7 @@ void gs_frame::hide_on_close() m_gui_settings->SetValue(gui::gs_visibility, current_visibility == Visibility::Hidden ? Visibility::AutomaticVisibility : current_visibility, false); m_gui_settings->SetValue(gui::gs_geometry, geometry(), true); - if (!g_progr_text.load()) + if (!g_progr_text) { // 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.