From 1d555fdd25fd33a6b14df59dc3291905573f142c Mon Sep 17 00:00:00 2001 From: Lioncash Date: Wed, 21 Nov 2018 21:37:08 -0500 Subject: [PATCH 1/6] common/thread: Remove unused CurrentThreadId() This is an old function that's no longer necessary. C++11 introduced proper threading support to the language and a thread ID can be retrieved via std::this_thread::get_id() if it's ever needed. --- src/common/thread.cpp | 10 ---------- src/common/thread.h | 2 -- 2 files changed, 12 deletions(-) diff --git a/src/common/thread.cpp b/src/common/thread.cpp index 9e207118f..a7267b637 100644 --- a/src/common/thread.cpp +++ b/src/common/thread.cpp @@ -25,16 +25,6 @@ namespace Common { -int CurrentThreadId() { -#ifdef _MSC_VER - return GetCurrentThreadId(); -#elif defined __APPLE__ - return mach_thread_self(); -#else - return 0; -#endif -} - #ifdef _WIN32 // Supporting functions void SleepCurrentThread(int ms) { diff --git a/src/common/thread.h b/src/common/thread.h index 6cbdb96a3..c20809021 100644 --- a/src/common/thread.h +++ b/src/common/thread.h @@ -13,8 +13,6 @@ namespace Common { -int CurrentThreadId(); - void SetThreadAffinity(std::thread::native_handle_type thread, u32 mask); void SetCurrentThreadAffinity(u32 mask); From d6583d68f630f3bd9a5626ab0fc24f2027ddd50a Mon Sep 17 00:00:00 2001 From: Lioncash Date: Wed, 21 Nov 2018 21:40:08 -0500 Subject: [PATCH 2/6] common/thread: Remove SleepCurrentThread() This is also unused and superceded by standard functionality. The standard library provides std::this_thread::sleep_for(), which provides a much more flexible interface, as different time units can be used with it. --- src/common/thread.cpp | 11 ----------- src/common/thread.h | 1 - 2 files changed, 12 deletions(-) diff --git a/src/common/thread.cpp b/src/common/thread.cpp index a7267b637..4bcb65236 100644 --- a/src/common/thread.cpp +++ b/src/common/thread.cpp @@ -25,13 +25,6 @@ namespace Common { -#ifdef _WIN32 -// Supporting functions -void SleepCurrentThread(int ms) { - Sleep(ms); -} -#endif - #ifdef _MSC_VER void SetThreadAffinity(std::thread::native_handle_type thread, u32 mask) { @@ -97,10 +90,6 @@ void SetCurrentThreadAffinity(u32 mask) { } #ifndef _WIN32 -void SleepCurrentThread(int ms) { - usleep(1000 * ms); -} - void SwitchCurrentThread() { usleep(1000 * 1); } diff --git a/src/common/thread.h b/src/common/thread.h index c20809021..5d3f39bd0 100644 --- a/src/common/thread.h +++ b/src/common/thread.h @@ -83,7 +83,6 @@ private: std::size_t generation; // Incremented once each time the barrier is used }; -void SleepCurrentThread(int ms); void SwitchCurrentThread(); // On Linux, this is equal to sleep 1ms void SetCurrentThreadName(const char* name); From 02602afd100ad7489de6a5da54ead3e7f22cbac7 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Wed, 21 Nov 2018 21:42:30 -0500 Subject: [PATCH 3/6] common/thread: Group non-member functions together Keeps the non-member interface in one spot instead of split into two places, making it nicer to locate functions. --- src/common/thread.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/common/thread.h b/src/common/thread.h index 5d3f39bd0..816183bc8 100644 --- a/src/common/thread.h +++ b/src/common/thread.h @@ -13,9 +13,6 @@ namespace Common { -void SetThreadAffinity(std::thread::native_handle_type thread, u32 mask); -void SetCurrentThreadAffinity(u32 mask); - class Event { public: Event() : is_set(false) {} @@ -83,6 +80,8 @@ private: std::size_t generation; // Incremented once each time the barrier is used }; +void SetThreadAffinity(std::thread::native_handle_type thread, u32 mask); +void SetCurrentThreadAffinity(u32 mask); void SwitchCurrentThread(); // On Linux, this is equal to sleep 1ms void SetCurrentThreadName(const char* name); From 756e773096c5a64cb5c0ff48104ceb36fa1935cb Mon Sep 17 00:00:00 2001 From: Lioncash Date: Wed, 21 Nov 2018 21:44:58 -0500 Subject: [PATCH 4/6] common/thread: Initialize class member variables where applicable Simplifies the constructor interfaces for Barrier and Event classes. --- src/common/thread.h | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/common/thread.h b/src/common/thread.h index 816183bc8..741dce487 100644 --- a/src/common/thread.h +++ b/src/common/thread.h @@ -15,8 +15,6 @@ namespace Common { class Event { public: - Event() : is_set(false) {} - void Set() { std::lock_guard lk(mutex); if (!is_set) { @@ -48,14 +46,14 @@ public: } private: - bool is_set; + bool is_set = false; std::condition_variable condvar; std::mutex mutex; }; class Barrier { public: - explicit Barrier(std::size_t count_) : count(count_), waiting(0), generation(0) {} + explicit Barrier(std::size_t count_) : count(count_) {} /// Blocks until all "count" threads have called Sync() void Sync() { @@ -76,8 +74,8 @@ private: std::condition_variable condvar; std::mutex mutex; const std::size_t count; - std::size_t waiting; - std::size_t generation; // Incremented once each time the barrier is used + std::size_t waiting = 0; + std::size_t generation = 0; // Incremented once each time the barrier is used }; void SetThreadAffinity(std::thread::native_handle_type thread, u32 mask); From 93f7677402e8350843566c714d119f1ab669f2a0 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Wed, 21 Nov 2018 21:47:06 -0500 Subject: [PATCH 5/6] common/thread: Make Barrier's 'count' member non-const While admirable as a means to ensure immutability, this has the unfortunate downside of making the class non-movable. std::move cannot actually perform a move operation if the provided operand has const data members (std::move acts as an operation to "slide" resources out of an object instance). Given Barrier contains move-only types such as std::mutex, this can lead to confusing error messages if an object ever contained a Barrier instance and said object was attempted to be moved. --- src/common/thread.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/common/thread.h b/src/common/thread.h index 741dce487..2cf74452d 100644 --- a/src/common/thread.h +++ b/src/common/thread.h @@ -73,7 +73,7 @@ public: private: std::condition_variable condvar; std::mutex mutex; - const std::size_t count; + std::size_t count; std::size_t waiting = 0; std::size_t generation = 0; // Incremented once each time the barrier is used }; From 1bf5a337a599e4be611711881a2b7fdf98baeb72 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Wed, 21 Nov 2018 21:53:35 -0500 Subject: [PATCH 6/6] common/thread: Drop Hungarian notation on SetCurrentThreadName's parameter This is inconsistent with our coding style. --- src/common/thread.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/common/thread.cpp b/src/common/thread.cpp index 4bcb65236..5144c0d9f 100644 --- a/src/common/thread.cpp +++ b/src/common/thread.cpp @@ -45,7 +45,7 @@ void SwitchCurrentThread() { // This is implemented much nicer in upcoming msvc++, see: // http://msdn.microsoft.com/en-us/library/xcb2z8hs(VS.100).aspx -void SetCurrentThreadName(const char* szThreadName) { +void SetCurrentThreadName(const char* name) { static const DWORD MS_VC_EXCEPTION = 0x406D1388; #pragma pack(push, 8) @@ -58,7 +58,7 @@ void SetCurrentThreadName(const char* szThreadName) { #pragma pack(pop) info.dwType = 0x1000; - info.szName = szThreadName; + info.szName = name; info.dwThreadID = -1; // dwThreadID; info.dwFlags = 0; @@ -97,15 +97,15 @@ void SwitchCurrentThread() { // MinGW with the POSIX threading model does not support pthread_setname_np #if !defined(_WIN32) || defined(_MSC_VER) -void SetCurrentThreadName(const char* szThreadName) { +void SetCurrentThreadName(const char* name) { #ifdef __APPLE__ - pthread_setname_np(szThreadName); + pthread_setname_np(name); #elif defined(__Bitrig__) || defined(__DragonFly__) || defined(__FreeBSD__) || defined(__OpenBSD__) - pthread_set_name_np(pthread_self(), szThreadName); + pthread_set_name_np(pthread_self(), name); #elif defined(__NetBSD__) - pthread_setname_np(pthread_self(), "%s", (void*)szThreadName); + pthread_setname_np(pthread_self(), "%s", (void*)name); #else - pthread_setname_np(pthread_self(), szThreadName); + pthread_setname_np(pthread_self(), name); #endif } #endif