From 8b57c7023420e83787352bbfba6462d11c2ab0e7 Mon Sep 17 00:00:00 2001 From: Koundinya Veluri Date: Thu, 28 May 2020 09:32:31 -0400 Subject: [PATCH] Move thread pool initialization to native side (#36789) Move thread pool initialization to native side - Following up from https://github.com/dotnet/runtime/pull/36697. Currently the initialization occurs when a static variable is accessed before calling into the VM, and after thinking about it some more the intent and need to initialize the thread pool can be a bit unclear - The initialization is specific to this implementation and the managed side doesn't necessarily need to know about it - Moved the initialization to the native side similarly to other thread pool APIs - Added some more assertions to relevant managed-to-native entry point paths to ensure that they are not called before the thread pool is initialized - This adds a non-volatile check when requesting a worker thread, which is already a very slow path. I ran into some issues with testing on the arm64 machine with updated locally built libcoreclr.so, so wasn't able to test that. I looked at the baseline profile and looking at time spent exclusively in ThreadPoolNative::RequestWorkerThread() and what it already does, I don't think the change would result in any significant perf difference. --- .../System/Threading/ThreadPool.CoreCLR.cs | 11 +++-------- src/coreclr/src/vm/comthreadpool.cpp | 14 ++++++++------ src/coreclr/src/vm/comthreadpool.h | 2 +- src/coreclr/src/vm/ecalllist.h | 2 +- src/coreclr/src/vm/win32threadpool.cpp | 19 ++++++++++++++++++- src/coreclr/src/vm/win32threadpool.h | 5 +++++ .../System/Threading/ThreadPool.Portable.cs | 2 ++ .../src/System/Threading/ThreadPool.cs | 5 +++-- .../src/System/Threading/ThreadPool.Mono.cs | 2 -- 9 files changed, 41 insertions(+), 21 deletions(-) diff --git a/src/coreclr/src/System.Private.CoreLib/src/System/Threading/ThreadPool.CoreCLR.cs b/src/coreclr/src/System.Private.CoreLib/src/System/Threading/ThreadPool.CoreCLR.cs index 719576c06d033..8f8b1b7b7ac66 100644 --- a/src/coreclr/src/System.Private.CoreLib/src/System/Threading/ThreadPool.CoreCLR.cs +++ b/src/coreclr/src/System.Private.CoreLib/src/System/Threading/ThreadPool.CoreCLR.cs @@ -192,12 +192,7 @@ public static partial class ThreadPool // Time in ms for which ThreadPoolWorkQueue.Dispatch keeps executing work items before returning to the OS private const uint DispatchQuantum = 30; - private static bool GetEnableWorkerTracking() - { - bool enableWorkerTracking = false; - InitializeVMTp(ref enableWorkerTracking); - return enableWorkerTracking; - } + internal static readonly bool EnableWorkerTracking = GetEnableWorkerTracking(); internal static bool KeepDispatching(int startTickCount) { @@ -340,8 +335,8 @@ internal static void NotifyWorkItemProgress() [MethodImpl(MethodImplOptions.InternalCall)] internal static extern void NotifyWorkItemProgressNative(); - [DllImport(RuntimeHelpers.QCall, CharSet = CharSet.Unicode)] - private static extern void InitializeVMTp(ref bool enableWorkerTracking); + [MethodImpl(MethodImplOptions.InternalCall)] + private static extern bool GetEnableWorkerTracking(); [MethodImpl(MethodImplOptions.InternalCall)] private static extern IntPtr RegisterWaitForSingleObjectNative( diff --git a/src/coreclr/src/vm/comthreadpool.cpp b/src/coreclr/src/vm/comthreadpool.cpp index a87d954615a4b..9a0ab9200160a 100644 --- a/src/coreclr/src/vm/comthreadpool.cpp +++ b/src/coreclr/src/vm/comthreadpool.cpp @@ -217,6 +217,7 @@ FCIMPLEND FCIMPL0(VOID, ThreadPoolNative::NotifyRequestProgress) { FCALL_CONTRACT; + _ASSERTE(ThreadpoolMgr::IsInitialized()); // can't be here without requesting a thread first ThreadpoolMgr::NotifyWorkItemCompleted(); @@ -247,6 +248,7 @@ FCIMPLEND FCIMPL0(FC_BOOL_RET, ThreadPoolNative::NotifyRequestComplete) { FCALL_CONTRACT; + _ASSERTE(ThreadpoolMgr::IsInitialized()); // can't be here without requesting a thread first ThreadpoolMgr::NotifyWorkItemCompleted(); @@ -305,15 +307,14 @@ FCIMPLEND /*****************************************************************************************************/ -void QCALLTYPE ThreadPoolNative::InitializeVMTp(CLR_BOOL* pEnableWorkerTracking) +FCIMPL0(FC_BOOL_RET, ThreadPoolNative::GetEnableWorkerTracking) { - QCALL_CONTRACT; + FCALL_CONTRACT; - BEGIN_QCALL; - ThreadpoolMgr::EnsureInitialized(); - *pEnableWorkerTracking = CLRConfig::GetConfigValue(CLRConfig::INTERNAL_ThreadPool_EnableWorkerTracking) ? TRUE : FALSE; - END_QCALL; + BOOL result = CLRConfig::GetConfigValue(CLRConfig::INTERNAL_ThreadPool_EnableWorkerTracking) ? TRUE : FALSE; + FC_RETURN_BOOL(result); } +FCIMPLEND /*****************************************************************************************************/ @@ -473,6 +474,7 @@ BOOL QCALLTYPE ThreadPoolNative::RequestWorkerThread() BEGIN_QCALL; + ThreadpoolMgr::EnsureInitialized(); ThreadpoolMgr::SetAppDomainRequestsActive(); res = ThreadpoolMgr::QueueUserWorkItem(NULL, diff --git a/src/coreclr/src/vm/comthreadpool.h b/src/coreclr/src/vm/comthreadpool.h index 23426fa0a9fad..ceef964099eb7 100644 --- a/src/coreclr/src/vm/comthreadpool.h +++ b/src/coreclr/src/vm/comthreadpool.h @@ -35,7 +35,7 @@ class ThreadPoolNative static FCDECL0(VOID, NotifyRequestProgress); static FCDECL0(FC_BOOL_RET, NotifyRequestComplete); - static void QCALLTYPE InitializeVMTp(CLR_BOOL* pEnableWorkerTracking); + static FCDECL0(FC_BOOL_RET, GetEnableWorkerTracking); static FCDECL1(void, ReportThreadStatus, CLR_BOOL isWorking); diff --git a/src/coreclr/src/vm/ecalllist.h b/src/coreclr/src/vm/ecalllist.h index 243a735c2b2e4..955bf9837e804 100644 --- a/src/coreclr/src/vm/ecalllist.h +++ b/src/coreclr/src/vm/ecalllist.h @@ -657,7 +657,7 @@ FCFuncStart(gThreadPoolFuncs) FCFuncElement("GetMaxThreadsNative", ThreadPoolNative::CorGetMaxThreads) FCFuncElement("NotifyWorkItemComplete", ThreadPoolNative::NotifyRequestComplete) FCFuncElement("NotifyWorkItemProgressNative", ThreadPoolNative::NotifyRequestProgress) - QCFuncElement("InitializeVMTp", ThreadPoolNative::InitializeVMTp) + FCFuncElement("GetEnableWorkerTracking", ThreadPoolNative::GetEnableWorkerTracking) FCFuncElement("ReportThreadStatus", ThreadPoolNative::ReportThreadStatus) QCFuncElement("RequestWorkerThread", ThreadPoolNative::RequestWorkerThread) FCFuncEnd() diff --git a/src/coreclr/src/vm/win32threadpool.cpp b/src/coreclr/src/vm/win32threadpool.cpp index b7106ccd8108a..6afe8ba374c2d 100644 --- a/src/coreclr/src/vm/win32threadpool.cpp +++ b/src/coreclr/src/vm/win32threadpool.cpp @@ -235,7 +235,7 @@ void ThreadpoolMgr::EnsureInitialized() { CONTRACTL { - THROWS; // Initialize can throw + THROWS; // EnsureInitializedSlow can throw MODE_ANY; GC_NOTRIGGER; } @@ -244,6 +244,19 @@ void ThreadpoolMgr::EnsureInitialized() if (IsInitialized()) return; + EnsureInitializedSlow(); +} + +NOINLINE void ThreadpoolMgr::EnsureInitializedSlow() +{ + CONTRACTL + { + THROWS; // Initialize can throw + MODE_ANY; + GC_NOTRIGGER; + } + CONTRACTL_END; + DWORD dwSwitchCount = 0; retry: @@ -758,7 +771,10 @@ void ThreadpoolMgr::ReportThreadStatus(bool isWorking) MODE_ANY; } CONTRACTL_END; + + _ASSERTE(IsInitialized()); // can't be here without requesting a thread first _ASSERTE(CLRConfig::GetConfigValue(CLRConfig::INTERNAL_ThreadPool_EnableWorkerTracking)); + while (true) { WorkingThreadCounts currentCounts, newCounts; @@ -3069,6 +3085,7 @@ void ThreadpoolMgr::DeregisterWait(WaitInfo* pArgs) void ThreadpoolMgr::WaitHandleCleanup(HANDLE hWaitObject) { LIMITED_METHOD_CONTRACT; + _ASSERTE(IsInitialized()); // cannot call cleanup before first registering WaitInfo* waitInfo = (WaitInfo*) hWaitObject; _ASSERTE(waitInfo->refCount > 0); diff --git a/src/coreclr/src/vm/win32threadpool.h b/src/coreclr/src/vm/win32threadpool.h index fb0804cb02f81..36ae2d3f636ea 100644 --- a/src/coreclr/src/vm/win32threadpool.h +++ b/src/coreclr/src/vm/win32threadpool.h @@ -824,7 +824,12 @@ class ThreadpoolMgr return entry; } +public: static void EnsureInitialized(); +private: + static void EnsureInitializedSlow(); + +public: static void InitPlatformVariables(); inline static BOOL IsInitialized() diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPool.Portable.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPool.Portable.cs index ed0bf45ed85c9..2a68a0b52c414 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPool.Portable.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPool.Portable.cs @@ -317,6 +317,8 @@ internal void WaitForRemoval() public static partial class ThreadPool { + internal const bool EnableWorkerTracking = false; + internal static void InitializeForThreadPoolThread() { } public static bool SetMaxThreads(int workerThreads, int completionPortThreads) diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPool.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPool.cs index de78cc1a7002f..6d06533b1bcf3 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPool.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPool.cs @@ -607,7 +607,8 @@ internal static bool Dispatch() // // Execute the workitem outside of any finally blocks, so that it can be aborted if needed. // - if (ThreadPool.s_enableWorkerTracking) +#pragma warning disable CS0162 // Unreachable code detected. EnableWorkerTracking may be constant false in some runtimes. + if (ThreadPool.EnableWorkerTracking) { bool reportedStatus = false; try @@ -630,6 +631,7 @@ internal static bool Dispatch() ThreadPool.ReportThreadStatus(isWorking: false); } } +#pragma warning restore CS0162 else if (workItem is Task task) { // Check for Task first as it's currently faster to type check @@ -935,7 +937,6 @@ internal static void PerformWaitOrTimerCallback(_ThreadPoolWaitOrTimerCallback h public static partial class ThreadPool { internal static readonly ThreadPoolWorkQueue s_workQueue = new ThreadPoolWorkQueue(); - internal static readonly bool s_enableWorkerTracking = GetEnableWorkerTracking(); /// Shim used to invoke of the supplied . internal static readonly Action s_invokeAsyncStateMachineBox = state => diff --git a/src/mono/netcore/System.Private.CoreLib/src/System/Threading/ThreadPool.Mono.cs b/src/mono/netcore/System.Private.CoreLib/src/System/Threading/ThreadPool.Mono.cs index ae757df4df433..4708d297f37b3 100644 --- a/src/mono/netcore/System.Private.CoreLib/src/System/Threading/ThreadPool.Mono.cs +++ b/src/mono/netcore/System.Private.CoreLib/src/System/Threading/ThreadPool.Mono.cs @@ -8,8 +8,6 @@ namespace System.Threading { public static partial class ThreadPool { - private static bool GetEnableWorkerTracking() => false; - internal static void ReportThreadStatus(bool isWorking) { }