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

@@ -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

Common/File/FileUtil.h

@@ -20,6 +20,7 @@
 #include <fstream>
 #include <cstdio>
 #include <string>
+#include <string_view>
 #include <time.h>
 #include <cstdint>
 
@@ -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);

Core/Util/RecentFiles.cpp

@@ -3,6 +3,7 @@
 #include <condition_variable>
 
 #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<std::mutex> guard(cmdLock_);
+		cmds_.push(RecentCommand{ RecentCmd::Exit });
+		cmdCondVar_.notify_one();
+	}
+	thread_.join();
 }
 
 std::vector<std::string> RecentFilesManager::GetRecentFiles() const {
-	std::lock_guard<std::mutex> guard(recentIsosLock);
-	return recentIsos;
+	std::lock_guard<std::mutex> 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<std::mutex> guard(recentIsosLock);
-	for (const auto & file : recentIsos) {
+	std::lock_guard<std::mutex> 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<std::mutex> guard(recentIsosLock);
-	return !recentIsos.empty();
+	std::lock_guard<std::mutex> guard(recentLock_);
+	return !recentFiles_.empty();
 }
 
 void RecentFilesManager::Clear() {
-	ResetThread();
-	std::lock_guard<std::mutex> guard(recentIsosLock);
-	recentIsos.clear();
+	std::lock_guard<std::mutex> 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<std::mutex> guard(recentIsosThreadLock);
-	if (recentIsosThreadPending && recentIsosThread.joinable())
-		recentIsosThread.join();
-}
-
-void RecentFilesManager::SetThread(std::function<void()> f) {
-	std::lock_guard<std::mutex> 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<std::string> 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<std::mutex> guard(recentIsosLock);
-	recentIsos = newRecent;
+	std::lock_guard<std::mutex> 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<std::vector<std::string>>(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<std::string> recentCopy;
 	{
-		std::lock_guard<std::mutex> guard(recentIsosLock);
-		recentCopy = recentIsos;
+		std::lock_guard<std::mutex> 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<std::mutex> 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<std::mutex> guard(recentIsosLock);
-	recentIsos.insert(recentIsos.begin(), resolvedFilename);
-	if ((int)recentIsos.size() > g_Config.iMaxRecent)
-		recentIsos.resize(g_Config.iMaxRecent);
+	std::lock_guard<std::mutex> guard(cmdLock_);
+	cmds_.push(RecentCommand{ RecentCmd::Add, {}, std::make_unique<std::string>(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<std::mutex> guard(cmdLock_);
+	cmds_.push(RecentCommand{ RecentCmd::Remove, {}, std::make_unique<std::string>(filename) });
+	cmdCondVar_.notify_one();
 }
 
 void RecentFilesManager::Clean() {
-	SetThread([this] {
-		SetCurrentThreadName("RecentISOs");
-
-		AndroidJNIThreadContext jniContext;  // destructor detaches
-
-		double startTime = time_now_d();
-
-		std::lock_guard<std::mutex> guard(recentIsosLock);
-		std::vector<std::string> 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<std::mutex> 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<std::mutex> 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<std::mutex> guard(recentLock_);
+			recentFiles_.clear();
+			System_PostUIMessage(UIMessage::RECENT_FILES_CHANGED);
+			break;
+		}
+		case RecentCmd::Add:
+		{
+			std::string resolvedFilename = File::ResolvePath(*cmd.sarg);
+			std::lock_guard<std::mutex> 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<std::mutex> 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<std::mutex> guard(recentLock_);
+			recentFiles_ = *cmd.varg;
+			System_PostUIMessage(UIMessage::RECENT_FILES_CHANGED);
+			break;
+		}
+		case RecentCmd::CleanMissing:
+		{
+			PerformCleanMissing();
+			break;
+		}
+		}
+	}
+}
+
+void RecentFilesManager::PerformCleanMissing() {
+	std::vector<std::string> initialRecent;
+
+	// Work on a copy, so we don't have to hold the lock.
+	{
+		std::lock_guard<std::mutex> guard(recentLock_);
+		initialRecent = recentFiles_;
+	}
+
+	std::vector<std::string> 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<std::mutex> guard(recentLock_);
+		recentFiles_ = cleanedRecent;
+		System_PostUIMessage(UIMessage::RECENT_FILES_CHANGED);
+	}
 }

Core/Util/RecentFiles.h

@@ -2,9 +2,12 @@
 
 #include <functional>
 #include <vector>
+#include <queue>
 #include <string>
+#include <string_view>
 #include <mutex>
 #include <thread>
+#include <memory>
 
 #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<std::string> GetRecentFiles() const;
 private:
-	void ResetThread();
-	void SetThread(std::function<void()> f);
-	void RemoveResolved(const std::string &resolvedFilename);
-	std::vector<std::string> 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<std::vector<std::string>> varg;
+		std::unique_ptr<std::string> sarg;
+	};
+
+	void PerformCleanMissing();
+	void WipePendingCommandsUnderLock();
+	void ThreadFunc();
+
+	std::queue<RecentCommand> cmds_;
+
+	mutable std::mutex recentLock_;
+	std::vector<std::string> recentFiles_;
+
+	std::thread thread_;
+	std::mutex cmdLock_;
+	std::condition_variable cmdCondVar_;
+
 };
 
 // Singleton, don't make more.