From 6ea0dd8208ec7536da51f30916fc0b7ed2378d43 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Wed, 26 Mar 2025 21:17:00 +0100 Subject: [PATCH] Switch the recent files manager to the "command processor on thread" pattern This removes all instances (except join-thread-on-quit) where the main thread was previously waiting for the cleaning of the recents list and similar. --- Common/File/FileUtil.cpp | 8 +- Common/File/FileUtil.h | 3 +- Core/Util/RecentFiles.cpp | 284 ++++++++++++++++++++++++-------------- Core/Util/RecentFiles.h | 45 ++++-- 4 files changed, 217 insertions(+), 123 deletions(-) diff --git a/Common/File/FileUtil.cpp b/Common/File/FileUtil.cpp index 882fbf4317..bbb357e8b3 100644 --- a/Common/File/FileUtil.cpp +++ b/Common/File/FileUtil.cpp @@ -315,21 +315,21 @@ static bool ResolvePathVista(const std::wstring &path, wchar_t *buf, DWORD bufSi } #endif -std::string ResolvePath(const std::string &path) { +std::string ResolvePath(std::string_view path) { if (LOG_IO) { - INFO_LOG(Log::System, "ResolvePath %s", path.c_str()); + INFO_LOG(Log::System, "ResolvePath %.*s", (int)path.size(), path.data()); } if (SIMULATE_SLOW_IO) { sleep_ms(100, "slow-io-sim"); } if (startsWith(path, "http://") || startsWith(path, "https://")) { - return path; + return std::string(path); } if (Android_IsContentUri(path)) { // Nothing to do? We consider these to only have one canonical form. - return path; + return std::string(path); } #ifdef _WIN32 diff --git a/Common/File/FileUtil.h b/Common/File/FileUtil.h index 858c09072d..7a5b83e1fd 100644 --- a/Common/File/FileUtil.h +++ b/Common/File/FileUtil.h @@ -20,6 +20,7 @@ #include #include #include +#include #include #include @@ -56,7 +57,7 @@ enum OpenFlag { int OpenFD(const Path &filename, OpenFlag flags); // Resolves symlinks and similar. -std::string ResolvePath(const std::string &path); +std::string ResolvePath(std::string_view path); // Returns true if file filename exists bool Exists(const Path &path); diff --git a/Core/Util/RecentFiles.cpp b/Core/Util/RecentFiles.cpp index 1cc4acc4df..2354cda783 100644 --- a/Core/Util/RecentFiles.cpp +++ b/Core/Util/RecentFiles.cpp @@ -3,6 +3,7 @@ #include #include "Common/File/FileUtil.h" +#include "Common/System/System.h" #include "Common/Thread/ThreadUtil.h" #include "Common/Log.h" #include "Common/TimeUtil.h" @@ -13,26 +14,34 @@ RecentFilesManager g_recentFiles; RecentFilesManager::RecentFilesManager() { - + thread_ = std::thread([this] { + ThreadFunc(); + }); } RecentFilesManager::~RecentFilesManager() { - ResetThread(); + { + std::lock_guard guard(cmdLock_); + cmds_.push(RecentCommand{ RecentCmd::Exit }); + cmdCondVar_.notify_one(); + } + thread_.join(); } std::vector RecentFilesManager::GetRecentFiles() const { - std::lock_guard guard(recentIsosLock); - return recentIsos; + std::lock_guard guard(recentLock_); + return recentFiles_; } -bool RecentFilesManager::ContainsFile(const std::string &filename) { +bool RecentFilesManager::ContainsFile(std::string_view filename) { if (g_Config.iMaxRecent <= 0) return false; - // Unfortunately this resolve needs to be done synchonously. + + // Unfortunately this resolve needs to be done synchronously. std::string resolvedFilename = File::ResolvePath(filename); - std::lock_guard guard(recentIsosLock); - for (const auto & file : recentIsos) { + std::lock_guard guard(recentLock_); + for (const auto &file : recentFiles_) { if (file == resolvedFilename) { return true; } @@ -41,33 +50,27 @@ bool RecentFilesManager::ContainsFile(const std::string &filename) { } bool RecentFilesManager::HasAny() const { - std::lock_guard guard(recentIsosLock); - return !recentIsos.empty(); + std::lock_guard guard(recentLock_); + return !recentFiles_.empty(); } void RecentFilesManager::Clear() { - ResetThread(); - std::lock_guard guard(recentIsosLock); - recentIsos.clear(); + std::lock_guard guard(cmdLock_); + // Just zapping any pending command, since they won't matter after this. + WipePendingCommandsUnderLock(); + cmds_.push(RecentCommand{ RecentCmd::Clear }); + cmdCondVar_.notify_one(); } -void RecentFilesManager::ResetThread() { - std::lock_guard guard(recentIsosThreadLock); - if (recentIsosThreadPending && recentIsosThread.joinable()) - recentIsosThread.join(); -} - -void RecentFilesManager::SetThread(std::function f) { - std::lock_guard guard(recentIsosThreadLock); - if (recentIsosThreadPending && recentIsosThread.joinable()) - recentIsosThread.join(); - recentIsosThread = std::thread(f); - recentIsosThreadPending = true; +void RecentFilesManager::WipePendingCommandsUnderLock() { + // Wipe any queued commands. + while (!cmds_.empty()) { + INFO_LOG(Log::System, "Wiped a recent command"); + cmds_.pop(); + } } void RecentFilesManager::Load(const Section *recent, int maxRecent) { - ResetThread(); - std::vector newRecent; for (int i = 0; i < maxRecent; i++) { char keyName[64]; @@ -79,17 +82,21 @@ void RecentFilesManager::Load(const Section *recent, int maxRecent) { } } - std::lock_guard guard(recentIsosLock); - recentIsos = newRecent; + std::lock_guard guard(cmdLock_); + // Just zapping any pending command, since they won't matter after this. + // TODO: Maybe we should let adds through... + WipePendingCommandsUnderLock(); + cmds_.push(RecentCommand{ RecentCmd::ReplaceAll, std::make_unique>(newRecent) }); + cmdCondVar_.notify_one(); } void RecentFilesManager::Save(Section *recent, int maxRecent) { - ResetThread(); + // TODO: Should we wait for any commands? Don't want to block... std::vector recentCopy; { - std::lock_guard guard(recentIsosLock); - recentCopy = recentIsos; + std::lock_guard guard(recentLock_); + recentCopy = recentFiles_; } for (int i = 0; i < maxRecent; i++) { @@ -103,92 +110,155 @@ void RecentFilesManager::Save(Section *recent, int maxRecent) { } } -void RecentFilesManager::RemoveResolved(const std::string &resolvedFilename) { - ResetThread(); - - std::lock_guard guard(recentIsosLock); - auto iter = std::remove_if(recentIsos.begin(), recentIsos.end(), [resolvedFilename](const auto &str) { - return str == resolvedFilename; - }); - // remove_if is weird. - recentIsos.erase(iter, recentIsos.end()); -} - -void RecentFilesManager::Add(const std::string &filename) { +void RecentFilesManager::Add(std::string_view filename) { if (g_Config.iMaxRecent <= 0) { return; } - std::string resolvedFilename = File::ResolvePath(filename); - RemoveResolved(resolvedFilename); - - ResetThread(); - std::lock_guard guard(recentIsosLock); - recentIsos.insert(recentIsos.begin(), resolvedFilename); - if ((int)recentIsos.size() > g_Config.iMaxRecent) - recentIsos.resize(g_Config.iMaxRecent); + std::lock_guard guard(cmdLock_); + cmds_.push(RecentCommand{ RecentCmd::Add, {}, std::make_unique(filename) }); + cmdCondVar_.notify_one(); } -void RecentFilesManager::Remove(const std::string &filename) { +void RecentFilesManager::Remove(std::string_view filename) { if (g_Config.iMaxRecent <= 0) { return; } - std::string resolvedFilename = File::ResolvePath(filename); - RemoveResolved(resolvedFilename); + std::lock_guard guard(cmdLock_); + cmds_.push(RecentCommand{ RecentCmd::Remove, {}, std::make_unique(filename) }); + cmdCondVar_.notify_one(); } void RecentFilesManager::Clean() { - SetThread([this] { - SetCurrentThreadName("RecentISOs"); - - AndroidJNIThreadContext jniContext; // destructor detaches - - double startTime = time_now_d(); - - std::lock_guard guard(recentIsosLock); - std::vector cleanedRecent; - if (recentIsos.empty()) { - INFO_LOG(Log::Loader, "No recents list found."); - } - - for (size_t i = 0; i < recentIsos.size(); i++) { - bool exists = false; - Path path = Path(recentIsos[i]); - switch (path.Type()) { - case PathType::CONTENT_URI: - case PathType::NATIVE: - exists = File::Exists(path); - if (!exists) { - if (TryUpdateSavedPath(&path)) { - exists = File::Exists(path); - INFO_LOG(Log::Loader, "Exists=%d when checking updated path: %s", exists, path.c_str()); - } - } - break; - default: - FileLoader *loader = ConstructFileLoader(path); - exists = loader->ExistsFast(); - delete loader; - break; - } - - if (exists) { - std::string pathStr = path.ToString(); - // Make sure we don't have any redundant items. - auto duplicate = std::find(cleanedRecent.begin(), cleanedRecent.end(), pathStr); - if (duplicate == cleanedRecent.end()) { - cleanedRecent.push_back(pathStr); - } - } else { - DEBUG_LOG(Log::Loader, "Removed %s from recent. errno=%d", path.c_str(), errno); - } - } - - double recentTime = time_now_d() - startTime; - if (recentTime > 0.1) { - INFO_LOG(Log::System, "CleanRecent took %0.2f", recentTime); - } - recentIsos = cleanedRecent; - }); + std::lock_guard guard(cmdLock_); + cmds_.push(RecentCommand{ RecentCmd::CleanMissing }); + cmdCondVar_.notify_one(); +} + +void RecentFilesManager::ThreadFunc() { + SetCurrentThreadName("RecentISOs"); + AndroidJNIThreadContext jniContext; // destructor detaches + + while (true) { + RecentCommand cmd; + { + std::unique_lock guard(cmdLock_); + cmdCondVar_.wait(guard, [this]() { return !cmds_.empty(); }); + cmd = std::move(cmds_.front()); + cmds_.pop(); + } + + switch (cmd.cmd) { + case RecentCmd::Exit: + // done! + return; + case RecentCmd::Clear: + { + std::lock_guard guard(recentLock_); + recentFiles_.clear(); + System_PostUIMessage(UIMessage::RECENT_FILES_CHANGED); + break; + } + case RecentCmd::Add: + { + std::string resolvedFilename = File::ResolvePath(*cmd.sarg); + std::lock_guard guard(recentLock_); + // First, remove the existing one. + // remove_if is weird. + if (!recentFiles_.empty()) { + recentFiles_.erase(std::remove_if(recentFiles_.begin(), recentFiles_.end(), [&resolvedFilename](const auto &str) { + return str == resolvedFilename; + }), recentFiles_.end()); + } + recentFiles_.insert(recentFiles_.begin(), resolvedFilename); + if ((int)recentFiles_.size() > g_Config.iMaxRecent) + recentFiles_.resize(g_Config.iMaxRecent); + System_PostUIMessage(UIMessage::RECENT_FILES_CHANGED); + break; + } + case RecentCmd::Remove: + { + std::string resolvedFilename = File::ResolvePath(*cmd.sarg); + std::lock_guard guard(recentLock_); + size_t count = recentFiles_.size(); + // remove_if is weird. + recentFiles_.erase(std::remove_if(recentFiles_.begin(), recentFiles_.end(), [&resolvedFilename](const auto &str) { + return str == resolvedFilename; + }), recentFiles_.end()); + if (recentFiles_.size() != count) { + System_PostUIMessage(UIMessage::RECENT_FILES_CHANGED); + } + break; + } + case RecentCmd::ReplaceAll: + { + std::lock_guard guard(recentLock_); + recentFiles_ = *cmd.varg; + System_PostUIMessage(UIMessage::RECENT_FILES_CHANGED); + break; + } + case RecentCmd::CleanMissing: + { + PerformCleanMissing(); + break; + } + } + } +} + +void RecentFilesManager::PerformCleanMissing() { + std::vector initialRecent; + + // Work on a copy, so we don't have to hold the lock. + { + std::lock_guard guard(recentLock_); + initialRecent = recentFiles_; + } + + std::vector cleanedRecent; + double startTime = time_now_d(); + + for (const auto &filename : initialRecent) { + bool exists = false; + Path path(filename); + switch (path.Type()) { + case PathType::CONTENT_URI: + case PathType::NATIVE: + exists = File::Exists(path); + if (!exists && TryUpdateSavedPath(&path)) { + // iOS only stuff + exists = File::Exists(path); + INFO_LOG(Log::Loader, "Exists=%d when checking updated path: %s", exists, path.c_str()); + } + break; + default: + FileLoader *loader = ConstructFileLoader(path); + exists = loader->ExistsFast(); + delete loader; + break; + } + + if (exists) { + const std::string pathStr = path.ToString(); + // Make sure we don't have any redundant items. + auto duplicate = std::find(cleanedRecent.begin(), cleanedRecent.end(), pathStr); + if (duplicate == cleanedRecent.end()) { + cleanedRecent.push_back(pathStr); + } + } else { + DEBUG_LOG(Log::Loader, "Removed %s from recent. errno=%d", path.c_str(), errno); + } + } + + double recentTime = time_now_d() - startTime; + if (recentTime > 0.1) { + INFO_LOG(Log::System, "CleanRecent took %0.2f", recentTime); + } + + if (cleanedRecent.size() != initialRecent.size()) { + std::lock_guard guard(recentLock_); + recentFiles_ = cleanedRecent; + System_PostUIMessage(UIMessage::RECENT_FILES_CHANGED); + } } diff --git a/Core/Util/RecentFiles.h b/Core/Util/RecentFiles.h index 6d27f7d7b6..f5f39b58ff 100644 --- a/Core/Util/RecentFiles.h +++ b/Core/Util/RecentFiles.h @@ -2,9 +2,12 @@ #include #include +#include #include +#include #include #include +#include #include "Common/Data/Format/IniFile.h" @@ -18,23 +21,43 @@ public: void Load(const Section *recent, int maxRecent); void Save(Section *recent, int maxRecent); - void Add(const std::string &filename); - void Remove(const std::string &filename); + void Add(std::string_view filename); + void Remove(std::string_view filename); void Clean(); bool HasAny() const; void Clear(); - bool ContainsFile(const std::string &filename); + bool ContainsFile(std::string_view filename); std::vector GetRecentFiles() const; private: - void ResetThread(); - void SetThread(std::function f); - void RemoveResolved(const std::string &resolvedFilename); - std::vector recentIsos; - mutable std::mutex recentIsosLock; - mutable std::mutex recentIsosThreadLock; - mutable std::thread recentIsosThread; - bool recentIsosThreadPending = false; + enum class RecentCmd { + Exit, + Clear, + CleanMissing, + Add, + Remove, + ReplaceAll, + }; + + struct RecentCommand { + RecentCmd cmd; + std::unique_ptr> varg; + std::unique_ptr sarg; + }; + + void PerformCleanMissing(); + void WipePendingCommandsUnderLock(); + void ThreadFunc(); + + std::queue cmds_; + + mutable std::mutex recentLock_; + std::vector recentFiles_; + + std::thread thread_; + std::mutex cmdLock_; + std::condition_variable cmdCondVar_; + }; // Singleton, don't make more.