Move thread pool initialization to native side (#36789)

Move thread pool initialization to native side - Following up from #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.
dotnet · May 28, 2020 · 8b57c70 · 8b57c70
1 parent f95b2b2
commit 8b57c70
Show file tree

Hide file tree

Showing 9 changed files with 41 additions and 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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();
 
         /// <summary>Shim used to invoke <see cref="IAsyncStateMachineBox.MoveNext"/> of the supplied <see cref="IAsyncStateMachineBox"/>.</summary>
         internal static readonly Action<object?> 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
@@ -8,8 +8,6 @@ namespace System.Threading
 {
     public static partial class ThreadPool
     {
-        private static bool GetEnableWorkerTracking() => false;
-
         internal static void ReportThreadStatus(bool isWorking)
         {
         }