Audit our use of condition variables, fix some minor issues

hrydgard · Jan 29, 2024 · ee62ffd · ee62ffd
1 parent a93b55c
commit ee62ffd
Show file tree

Hide file tree

Showing 4 changed files with 32 additions and 27 deletions.
diff --git a/Common/File/PathBrowser.cpp b/Common/File/PathBrowser.cpp
@@ -111,12 +111,12 @@ bool LoadRemoteFileList(const Path &url, const std::string &userAgent, bool *can
 }
 
 PathBrowser::~PathBrowser() {
-	std::unique_lock<std::mutex> guard(pendingLock_);
-	pendingCancel_ = true;
-	pendingStop_ = true;
-	pendingCond_.notify_all();
-	guard.unlock();
-
+	{
+		std::unique_lock<std::mutex> guard(pendingLock_);
+		pendingCancel_ = true;
+		pendingStop_ = true;
+		pendingCond_.notify_all();
+	}
 	if (pendingThread_.joinable()) {
 		pendingThread_.join();
 	}

diff --git a/Common/GPU/Vulkan/VulkanRenderManager.cpp b/Common/GPU/Vulkan/VulkanRenderManager.cpp
@@ -385,13 +385,7 @@ void VulkanRenderManager::StopThreads() {
 		pushCondVar_.notify_one();
 		// Once the render thread encounters the above exit task, it'll exit.
 		renderThread_.join();
-	}
-
-	// Compiler and present thread still relies on this.
-	runCompileThread_ = false;
-
-	if (presentWaitThread_.joinable()) {
-		presentWaitThread_.join();
+		INFO_LOG(G3D, "Vulkan submission thread joined. Frame=%d", vulkan_->GetCurFrame());
 	}
 
 	for (int i = 0; i < vulkan_->GetInflightFrames(); i++) {
@@ -400,12 +394,18 @@ void VulkanRenderManager::StopThreads() {
 		frameData.profile.timestampDescriptions.clear();
 	}
 
-	INFO_LOG(G3D, "Vulkan submission thread joined. Frame=%d", vulkan_->GetCurFrame());
-
-	_assert_(compileThread_.joinable());
-	compileCond_.notify_all();
+	{
+		std::unique_lock<std::mutex> lock(compileMutex_);
+		runCompileThread_ = false;  // Compiler and present thread both look at this bool.
+		_assert_(compileThread_.joinable());
+		compileCond_.notify_one();
+	}
 	compileThread_.join();
 
+	if (presentWaitThread_.joinable()) {
+		presentWaitThread_.join();
+	}
+
 	INFO_LOG(G3D, "Vulkan compiler thread joined. Now wait for any straggling compile tasks.");
 	CreateMultiPipelinesTask::WaitForAll();
 
@@ -462,7 +462,7 @@ void VulkanRenderManager::CompileThreadFunc() {
 		std::vector<CompileQueueEntry> toCompile;
 		{
 			std::unique_lock<std::mutex> lock(compileMutex_);
-			if (compileQueue_.empty() && runCompileThread_) {
+			while (compileQueue_.empty() && runCompileThread_) {
 				compileCond_.wait(lock);
 			}
 			toCompile = std::move(compileQueue_);
@@ -786,7 +786,7 @@ VKRGraphicsPipeline *VulkanRenderManager::CreateGraphicsPipeline(VKRGraphicsPipe
 			VKRRenderPassStoreAction::STORE, VKRRenderPassStoreAction::DONT_CARE, VKRRenderPassStoreAction::DONT_CARE,
 		};
 		VKRRenderPass *compatibleRenderPass = queueRunner_.GetRenderPass(key);
-		std::lock_guard<std::mutex> lock(compileMutex_);
+		std::unique_lock<std::mutex> lock(compileMutex_);
 		bool needsCompile = false;
 		for (size_t i = 0; i < (size_t)RenderPassType::TYPE_COUNT; i++) {
 			if (!(variantBitmask & (1 << i)))
@@ -809,7 +809,7 @@ VKRGraphicsPipeline *VulkanRenderManager::CreateGraphicsPipeline(VKRGraphicsPipe
 			}
 
 			pipeline->pipeline[i] = Promise<VkPipeline>::CreateEmpty();
-			compileQueue_.push_back(CompileQueueEntry(pipeline, compatibleRenderPass->Get(vulkan_, rpType, sampleCount), rpType, sampleCount));
+			compileQueue_.emplace_back(pipeline, compatibleRenderPass->Get(vulkan_, rpType, sampleCount), rpType, sampleCount);
 			needsCompile = true;
 		}
 		if (needsCompile)
@@ -1551,10 +1551,12 @@ void VulkanRenderManager::FlushSync() {
 			VLOG("PUSH: Frame[%d]", curFrame);
 			VKRRenderThreadTask *task = new VKRRenderThreadTask(VKRRunType::SYNC);
 			task->frame = curFrame;
-			std::unique_lock<std::mutex> lock(pushMutex_);
-			renderThreadQueue_.push(task);
-			renderThreadQueue_.back()->steps = std::move(steps_);
-			pushCondVar_.notify_one();
+			{
+				std::unique_lock<std::mutex> lock(pushMutex_);
+				renderThreadQueue_.push(task);
+				renderThreadQueue_.back()->steps = std::move(steps_);
+				pushCondVar_.notify_one();
+			}
 			steps_.clear();
 		}
 

diff --git a/Common/Thread/ThreadManager.cpp b/Common/Thread/ThreadManager.cpp
@@ -59,8 +59,8 @@ ThreadManager::~ThreadManager() {
 
 void ThreadManager::Teardown() {
 	for (TaskThreadContext *&threadCtx : global_->threads_) {
-		threadCtx->cancelled = true;
 		std::unique_lock<std::mutex> lock(threadCtx->mutex);
+		threadCtx->cancelled = true;
 		threadCtx->cond.notify_one();
 	}
 

diff --git a/UI/GameSettingsScreen.h b/UI/GameSettingsScreen.h
@@ -175,8 +175,11 @@ class HostnameSelectScreen : public PopupScreen {
 		}, this);
 	}
 	~HostnameSelectScreen() {
-		resolverState_ = ResolverState::QUIT;
-		resolverCond_.notify_one();
+		{
+			std::unique_lock<std::mutex> guard(resolverLock_);
+			resolverState_ = ResolverState::QUIT;
+			resolverCond_.notify_one();
+		}
 		resolver_.join();
 	}