Merge pull request #16716 from hrydgard/jni-detach-threads

Android JNI: Try a bit harder to properly detach threads from the JVM when they exit

Common/File/PathBrowser.cpp

@@ -141,6 +141,8 @@ void PathBrowser::HandlePath() {
 	pendingThread_ = std::thread([&] {
 		SetCurrentThreadName("PathBrowser");
 
+		AndroidJNIThreadContext jniContext;  // destructor detaches
+
 		std::unique_lock<std::mutex> guard(pendingLock_);
 		std::vector<File::FileInfo> results;
 		Path lastPath("NONSENSE THAT WONT EQUAL A PATH");

Common/GPU/Vulkan/VulkanRenderManager.cpp

@@ -121,9 +121,9 @@ bool VKRGraphicsPipeline::Create(VulkanContext *vulkan, VkRenderPass compatibleR
 	double taken_ms = (time_now_d() - start) * 1000.0;
 
 	if (taken_ms < 0.1) {
-		DEBUG_LOG(G3D, "Pipeline creation time: %0.2f ms (fast) rpType: %08x sampleBits: %d\n(%s)", taken_ms, (u32)rpType, (u32)sampleCount, tag_.c_str());
+		DEBUG_LOG(G3D, "Pipeline creation time: %0.2f ms (fast) rpType: %08x sampleBits: %d (%s)", taken_ms, (u32)rpType, (u32)sampleCount, tag_.c_str());
 	} else {
-		INFO_LOG(G3D, "Pipeline creation time: %0.2f ms  rpType: %08x sampleBits: %d\n(%s)", taken_ms, (u32)rpType, (u32)sampleCount, tag_.c_str());
+		INFO_LOG(G3D, "Pipeline creation time: %0.2f ms  rpType: %08x sampleBits: %d (%s)", taken_ms, (u32)rpType, (u32)sampleCount, tag_.c_str());
 	}
 
 	bool success = true;

Common/Net/HTTPClient.cpp

@@ -526,6 +526,9 @@ std::string Download::RedirectLocation(const std::string &baseUrl) {
 
 void Download::Do() {
 	SetCurrentThreadName("Downloader::Do");
+
+	AndroidJNIThreadContext jniContext;
+
 	resultCode_ = 0;
 
 	std::string downloadURL = url_;

Common/Thread/ThreadManager.cpp

@@ -44,6 +44,7 @@ struct ThreadContext {
 	std::atomic<bool> cancelled;
 	std::atomic<Task *> private_single;
 	std::deque<Task *> private_queue;
+	char name[16];
 };
 
 ThreadManager::ThreadManager() : global_(new GlobalThreadContext()) {
@@ -124,14 +125,17 @@ bool ThreadManager::TeardownTask(Task *task, bool enqueue) {
 }
 
 static void WorkerThreadFunc(GlobalThreadContext *global, ThreadContext *thread) {
-	char threadName[16];
 	if (thread->type == TaskType::CPU_COMPUTE) {
-		snprintf(threadName, sizeof(threadName), "PoolWorker %d", thread->index);
+		snprintf(thread->name, sizeof(thread->name), "PoolWorker %d", thread->index);
 	} else {
 		_assert_(thread->type == TaskType::IO_BLOCKING);
-		snprintf(threadName, sizeof(threadName), "PoolWorkerIO %d", thread->index);
+		snprintf(thread->name, sizeof(thread->name), "PoolWorkerIO %d", thread->index);
+	}
+	SetCurrentThreadName(thread->name);
+
+	if (thread->type == TaskType::IO_BLOCKING) {
+		AttachThreadToJNI();
 	}
-	SetCurrentThreadName(threadName);
 
 	const bool isCompute = thread->type == TaskType::CPU_COMPUTE;
 	const auto global_queue_size = [isCompute, &global]() -> int {
@@ -185,6 +189,11 @@ static void WorkerThreadFunc(GlobalThreadContext *global, ThreadContext *thread)
 			thread->queue_size--;
 		}
 	}
+
+	// In case it got attached to JNI, detach it. Don't think this has any side effects if called redundantly.
+	if (thread->type == TaskType::IO_BLOCKING) {
+		DetachThreadFromJNI();
+	}
 }
 
 void ThreadManager::Init(int numRealCores, int numLogicalCoresPerCpu) {

Common/Thread/ThreadUtil.cpp

@@ -12,17 +12,43 @@
 
 #elif defined(__ANDROID__)
 
+#include "android/jni/app-android.h"
+
 #define TLS_SUPPORTED
 
 #endif
 
+// TODO: Many other platforms also support TLS, in fact probably nearly all that we support
+// these days.
+
 #include <cstring>
 #include <cstdint>
 
 #include "Common/Log.h"
 #include "Common/Thread/ThreadUtil.h"
 #include "Common/Data/Encoding/Utf8.h"
 
+AttachDetachFunc g_attach;
+AttachDetachFunc g_detach;
+
+void AttachThreadToJNI() {
+	if (g_attach) {
+		g_attach();
+	}
+}
+
+
+void DetachThreadFromJNI() {
+	if (g_detach) {
+		g_detach();
+	}
+}
+
+void RegisterAttachDetach(AttachDetachFunc attach, AttachDetachFunc detach) {
+	g_attach = attach;
+	g_detach = detach;
+}
+
 #if (PPSSPP_PLATFORM(ANDROID) || PPSSPP_PLATFORM(LINUX)) && !defined(_GNU_SOURCE)
 #define _GNU_SOURCE
 #endif
@@ -62,7 +88,6 @@ static EXCEPTION_DISPOSITION NTAPI ignore_handler(EXCEPTION_RECORD *rec,
 }
 #endif
 
-
 #if PPSSPP_PLATFORM(WINDOWS) && !PPSSPP_PLATFORM(UWP)
 typedef HRESULT (WINAPI *TSetThreadDescription)(HANDLE, PCWSTR);
 
@@ -90,7 +115,15 @@ static void InitializeSetThreadDescription() {
 void SetCurrentThreadNameThroughException(const char *threadName);
 #endif
 
-void SetCurrentThreadName(const char* threadName) {
+const char *GetCurrentThreadName() {
+#ifdef TLS_SUPPORTED
+	return curThreadName;
+#else
+	return "";
+#endif
+}
+
+void SetCurrentThreadName(const char *threadName) {
 #if PPSSPP_PLATFORM(WINDOWS) && !PPSSPP_PLATFORM(UWP)
 	InitializeSetThreadDescription();
 	if (g_pSetThreadDescription) {

Common/Thread/ThreadUtil.h

@@ -2,11 +2,37 @@
 
 #include <mutex>
 
-// Note that name must be a global string that lives until the end of the process,
+// Note that the string pointed to must have a lifetime until the end of the thread,
 // for AssertCurrentThreadName to work.
 void SetCurrentThreadName(const char *threadName);
 void AssertCurrentThreadName(const char *threadName);
 
+// If TLS is not supported, this will return an empty string.
+const char *GetCurrentThreadName();
+
 // Just gets a cheap thread identifier so that you can see different threads in debug output,
 // exactly what it is is badly specified and not useful for anything.
 int GetCurrentThreadIdForDebug();
+
+typedef void (*AttachDetachFunc)();
+
+void RegisterAttachDetach(AttachDetachFunc attach, AttachDetachFunc detach);
+
+// When you know that a thread potentially will make JNI calls, call this after setting its name.
+void AttachThreadToJNI();
+
+// Call when leaving threads. On Android, calls DetachCurrentThread.
+// Threads that use scoped storage I/O end up attached as JNI threads, and will thus
+// need this in order to follow the rules correctly. Some devices seem to enforce this.
+void DetachThreadFromJNI();
+
+// Utility to call the above two functions.
+class AndroidJNIThreadContext {
+public:
+	AndroidJNIThreadContext() {
+		AttachThreadToJNI();
+	}
+	~AndroidJNIThreadContext() {
+		DetachThreadFromJNI();
+	}
+};

Core/Config.cpp

@@ -32,6 +32,7 @@
 
 #include "Common/Log.h"
 #include "Common/TimeUtil.h"
+#include "Common/Thread/ThreadUtil.h"
 #include "Common/Data/Format/IniFile.h"
 #include "Common/Data/Format/JSONReader.h"
 #include "Common/Data/Text/I18n.h"
@@ -43,6 +44,7 @@
 #include "Common/System/Display.h"
 #include "Common/System/System.h"
 #include "Common/StringUtils.h"
+#include "Common/Thread/ThreadUtil.h"
 #include "Common/GPU/Vulkan/VulkanLoader.h"
 #include "Common/VR/PPSSPPVR.h"
 #include "Core/Config.h"
@@ -1733,6 +1735,10 @@ void Config::RemoveRecent(const std::string &file) {
 
 void Config::CleanRecent() {
 	private_->SetRecentIsosThread([this] {
+		SetCurrentThreadName("RecentISOs");
+
+		AndroidJNIThreadContext jniContext;  // destructor detaches
+
 		double startTime = time_now_d();
 
 		std::lock_guard<std::mutex> guard(private_->recentIsosLock);

Core/Dialog/PSPSaveDialog.cpp

@@ -1210,6 +1210,8 @@ void PSPSaveDialog::JoinIOThread() {
 
 static void DoExecuteIOAction(PSPSaveDialog *dialog) {
 	SetCurrentThreadName("SaveIO");
+
+	AndroidJNIThreadContext jniContext;
 	dialog->ExecuteIOAction();
 }
 

Core/FileLoaders/CachingFileLoader.cpp

@@ -269,6 +269,8 @@ void CachingFileLoader::StartReadAhead(s64 pos) {
 	aheadThread_ = std::thread([this, pos] {
 		SetCurrentThreadName("FileLoaderReadAhead");
 
+		AndroidJNIThreadContext jniContext;
+
 		std::unique_lock<std::recursive_mutex> guard(blocksMutex_);
 		s64 cacheStartPos = pos >> BLOCK_SHIFT;
 		s64 cacheEndPos = cacheStartPos + BLOCK_READAHEAD - 1;

Core/FileLoaders/RamCachingFileLoader.cpp

@@ -227,6 +227,8 @@ void RamCachingFileLoader::StartReadAhead(s64 pos) {
 	aheadThread_ = std::thread([this] {
 		SetCurrentThreadName("FileLoaderReadAhead");
 
+		AndroidJNIThreadContext jniContext;
+
 		while (aheadRemaining_ != 0 && !aheadCancel_) {
 			// Where should we look?
 			const u32 cacheStartPos = NextAheadBlock();

Core/HLE/sceIo.cpp

@@ -579,6 +579,7 @@ static void __IoAsyncEndCallback(SceUID threadID, SceUID prevCallbackId) {
 
 static void __IoManagerThread() {
 	SetCurrentThreadName("IO");
+	AndroidJNIThreadContext jniContext;
 	while (ioManagerThreadEnabled && coreState != CORE_BOOT_ERROR && coreState != CORE_RUNTIME_ERROR && coreState != CORE_POWERDOWN) {
 		ioManager.RunEventsUntil(CoreTiming::GetTicks() + msToCycles(1000));
 	}

Core/HW/MemoryStick.cpp

@@ -19,8 +19,10 @@
 #include <condition_variable>
 #include <mutex>
 #include <thread>
+
 #include "Common/Serialize/Serializer.h"
 #include "Common/Serialize/SerializeFuncs.h"
+#include "Common/Thread/ThreadUtil.h"
 #include "Core/Config.h"
 #include "Core/CoreTiming.h"
 #include "Core/Compatibility.h"
@@ -97,6 +99,10 @@ static void MemoryStick_CalcInitialFree() {
 	std::unique_lock<std::mutex> guard(freeCalcMutex);
 	freeCalcStatus = FreeCalcStatus::RUNNING;
 	freeCalcThread = std::thread([] {
+		SetCurrentThreadName("CalcInitialFree");
+
+		AndroidJNIThreadContext jniContext;
+
 		memstickInitialFree = pspFileSystem.FreeSpace("ms0:/") + pspFileSystem.ComputeRecursiveDirectorySize("ms0:/PSP/SAVEDATA/");
 
 		std::unique_lock<std::mutex> guard(freeCalcMutex);

Core/PSPLoaders.cpp

@@ -307,7 +307,7 @@ bool Load_PSP_ISO(FileLoader *fileLoader, std::string *error_string) {
 	//in case we didn't go through EmuScreen::boot
 	g_Config.loadGameConfig(id, g_paramSFO.GetValueString("TITLE"));
 	host->SendUIMessage("config_loaded", "");
-	INFO_LOG(LOADER,"Loading %s...", bootpath.c_str());
+	INFO_LOG(LOADER, "Loading %s...", bootpath.c_str());
 
 	PSPLoaders_Shutdown();
 	// Note: this thread reads the game binary, loads caches, and links HLE while UI spins.
@@ -319,6 +319,8 @@ bool Load_PSP_ISO(FileLoader *fileLoader, std::string *error_string) {
 		if (coreState != CORE_POWERUP)
 			return;
 
+		AndroidJNIThreadContext jniContext;
+
 		PSP_SetLoading("Loading executable...");
 		// TODO: We can't use the initial error_string pointer.
 		bool success = __KernelLoadExec(bootpath.c_str(), 0, &PSP_CoreParameter().errorString);
@@ -455,6 +457,8 @@ bool Load_PSP_ELF_PBP(FileLoader *fileLoader, std::string *error_string) {
 		if (coreState != CORE_POWERUP)
 			return;
 
+		AndroidJNIThreadContext jniContext;
+
 		bool success = __KernelLoadExec(finalName.c_str(), 0, &PSP_CoreParameter().errorString);
 		if (success && coreState == CORE_POWERUP) {
 			coreState = PSP_CoreParameter().startBreak ? CORE_STEPPING : CORE_RUNNING;
@@ -479,6 +483,8 @@ bool Load_PSP_GE_Dump(FileLoader *fileLoader, std::string *error_string) {
 		if (coreState != CORE_POWERUP)
 			return;
 
+		AndroidJNIThreadContext jniContext;
+
 		bool success = __KernelLoadGEDump("disc0:/data.ppdmp", &PSP_CoreParameter().errorString);
 		if (success && coreState == CORE_POWERUP) {
 			coreState = PSP_CoreParameter().startBreak ? CORE_STEPPING : CORE_RUNNING;

Core/Reporting.cpp

@@ -111,6 +111,8 @@ namespace Reporting
 	static int CalculateCRCThread() {
 		SetCurrentThreadName("ReportCRC");
 
+		AndroidJNIThreadContext jniContext;
+
 		FileLoader *fileLoader = ResolveFileLoaderTarget(ConstructFileLoader(crcFilename));
 		BlockDevice *blockDevice = constructBlockDevice(fileLoader);
 

Core/SaveState.cpp

@@ -164,6 +164,8 @@ namespace SaveState
 				compressThread_.join();
 			compressThread_ = std::thread([=]{
 				SetCurrentThreadName("SaveStateCompress");
+
+				// Should do no I/O, so no JNI thread context needed.
 				Compress(*result, *state, *base);
 			});
 		}

Core/Util/GameManager.cpp

@@ -38,6 +38,7 @@
 #include "Common/Log.h"
 #include "Common/File/FileUtil.h"
 #include "Common/StringUtils.h"
+#include "Common/Thread/ThreadUtil.h"
 #include "Core/Config.h"
 #include "Core/Loaders.h"
 #include "Core/ELF/ParamSFO.h"
@@ -278,11 +279,15 @@ ZipFileContents DetectZipFileContents(struct zip *z, ZipFileInfo *info) {
 
 // Parameters need to be by value, since this is a thread func.
 bool GameManager::InstallGame(Path url, Path fileName, bool deleteAfter) {
+	SetCurrentThreadName("InstallGame");
+
 	if (installInProgress_ || installDonePending_) {
 		ERROR_LOG(HLE, "Cannot have two installs in progress at the same time");
 		return false;
 	}
 
+	AndroidJNIThreadContext context;  // Destructor detaches.
+
 	if (!File::Exists(fileName)) {
 		ERROR_LOG(HLE, "Game file '%s' doesn't exist", fileName.c_str());
 		return false;
@@ -445,6 +450,7 @@ std::string GameManager::GetISOGameID(FileLoader *loader) const {
 	if (!bd) {
 		return "";
 	}
+
 	ISOFileSystem umd(&handles, bd);
 
 	PSPFileInfo info = umd.GetFileInfo("/PSP_GAME/PARAM.SFO");

Core/WebServer.cpp

@@ -212,6 +212,8 @@ static void DiscHandler(const http::Request &request, const Path &filename) {
 }
 
 static void HandleListing(const http::Request &request) {
+	AndroidJNIThreadContext jniContext;
+
 	request.WriteHttpResponseHeader("1.0", 200, -1, "text/plain");
 	request.Out()->Printf("/\n");
 	if (serverFlags & (int)WebServerFlags::DISCS) {
@@ -269,6 +271,8 @@ static void RedirectToDebugger(const http::Request &request) {
 }
 
 static void HandleFallback(const http::Request &request) {
+	AndroidJNIThreadContext jniContext;
+
 	if (serverFlags & (int)WebServerFlags::DISCS) {
 		Path filename = LocalFromRemotePath(request.resource());
 		if (!filename.empty()) {
@@ -293,6 +297,8 @@ static void HandleFallback(const http::Request &request) {
 }
 
 static void ForwardDebuggerRequest(const http::Request &request) {
+	AndroidJNIThreadContext jniContext;
+
 	if (serverFlags & (int)WebServerFlags::DEBUGGER) {
 		// Check if this is a websocket request...
 		std::string upgrade;
@@ -314,6 +320,8 @@ static void ForwardDebuggerRequest(const http::Request &request) {
 static void ExecuteWebServer() {
 	SetCurrentThreadName("HTTPServer");
 
+	AndroidJNIThreadContext context;  // Destructor detaches.
+
 	auto http = new http::Server(new NewThreadExecutor());
 	http->RegisterHandler("/", &HandleListing);
 	// This lists all the (current) recent ISOs.