Progress Dialog: Fix recursion and concurrency use of text updates

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 <typename T, usz N>
+// 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 <typename T, usz N = std::max<usz>(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<T, N> 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<typename T, usz N>
+// Simple lock-free FIFO queue base. Based on lf_array<T, N> 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<typename T, usz N = std::max<usz>(256 / sizeof(T), 1)>
 class lf_fifo : public lf_array<T, N>
 {
 	// LSB 32-bit: push, MSB 32-bit: pop

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

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);
 

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);

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<named_thread<progress_dialog_server>>(); progress_dialog && g_progr_text.load())
+		if (auto progress_dialog = g_fxo->try_get<named_thread<progress_dialog_server>>(); 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;

rpcs3/Emu/scoped_progress_dialog.cpp

@@ -0,0 +1,206 @@
+#include "stdafx.h"
+#include "system_progress.hpp"
+
+shared_ptr<std::string> progress_dialog_string_t::get_string_ptr() const noexcept
+{
+	shared_ptr<std::string> 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<std::string> installed_ptr = make_single_value(std::move(text));
+	const shared_ptr<std::string> c_null_ptr;
+
+	for (usz init_size = g_progr_text_queue.size(), new_text_index = std::max<usz>(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<progress_dialog_string_t::data_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<std::string> installed_ptr = make_single_value(std::move(text));
+	const shared_ptr<std::string> c_null_ptr;
+
+	for (usz init_size = g_progr_text_queue.size(), new_text_index = std::max<usz>(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<progress_dialog_string_t::data_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<progress_dialog_string_t::data_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();
+	}
+}

rpcs3/Emu/system_progress.cpp

@@ -13,7 +13,8 @@
 LOG_CHANNEL(sys_log, "SYS");
 
 // Progress display server synchronization variables
-atomic_t<progress_dialog_string_t> g_progr_text{};
+lf_array<atomic_ptr<std::string>> g_progr_text_queue;
+progress_dialog_string_t g_progr_text{};
 atomic_t<u32> g_progr_ftotal{0};
 atomic_t<u32> g_progr_fdone{0};
 atomic_t<u64> 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<progress_dialog_string_t::data_t>{});
 }

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<atomic_ptr<std::string>> 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_t> data{};
+
+	shared_ptr<std::string> get_string_ptr() const noexcept;
+
+	operator std::string() const noexcept
+	{
+		if (shared_ptr<std::string> ptr = get_string_ptr())
+		{
+			return *ptr;
+		}
+
+		return {};
+	}
+
+	explicit operator bool() const noexcept
+	{
+		return get_string_ptr().operator bool();
 	}
 };
 
-extern atomic_t<progress_dialog_string_t> g_progr_text;
+extern progress_dialog_string_t g_progr_text;
 extern atomic_t<u32> g_progr_ftotal;
 extern atomic_t<u32> g_progr_fdone;
 extern atomic_t<u64> g_progr_ftotal_bits;
@@ -29,76 +49,18 @@ extern atomic_t<bool> 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

rpcs3/emucore.vcxproj

@@ -141,6 +141,7 @@
     <ClCompile Include="Emu\RSX\RSXZCULL.cpp" />
     <ClCompile Include="Emu\RSX\rsx_vertex_data.cpp" />
     <ClCompile Include="Emu\savestate_utils.cpp" />
+    <ClCompile Include="Emu\scoped_progress_dialog.cpp" />
     <ClCompile Include="Emu\system_config_types.cpp" />
     <ClCompile Include="Emu\perf_meter.cpp" />
     <ClCompile Include="Emu\system_progress.cpp" />

rpcs3/emucore.vcxproj.filters

@@ -1138,6 +1138,9 @@
     <ClCompile Include="Emu\savestate_utils.cpp">
       <Filter>Emu</Filter>
     </ClCompile>
+    <ClCompile Include="Emu\scoped_progress_dialog.cpp">
+      <Filter>Emu</Filter>
+    </ClCompile>
     <ClCompile Include="Emu\Cell\Modules\cellMusicSelectionContext.cpp">
       <Filter>Emu\Cell\Modules</Filter>
     </ClCompile>

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.