[Local GC] Unify background GC thread and server GC thread creation (#…

…14821)

* Initial cut, ignoring thread affinity

* Integrate thread affinity

* Affinity for standalone Windows

* Add 'specialness' and the thread name as arguments to CreateThread

* First crack at unified implementation

* Set priority for server GC threads

* Remove unused parameter

* Address code review feedback and remove some dead code that broke the clang build

* Use char* on the interface instead of wchar_t (doesn't play well cross-platform)

* Rename IsGCSpecialThread -> CurrentThreadWasCreatedByGC

* Code review feedback and fix up the build

* rename CurrentThreadWasCreatedByGC -> WasCurrentThreadCreatedByGC

* Fix a contract violation when converting string encodings

* Thread::CreateUtilityThread returns a thread that is not suspended - restarting a non-suspended thread is incorrect

* CreateUnsuspendableThread -> CreateNonSuspendableThread

src/gc/env/gcenv.ee.h

@@ -56,9 +56,6 @@ class GCToEEInterface
     static bool CatchAtSafePoint(Thread * pThread);
 
     static void GcEnumAllocContexts(enum_alloc_context_func* fn, void* param);
-
-    static Thread* CreateBackgroundThread(GCBackgroundThreadFunction threadStart, void* arg);
-
     // Diagnostics methods.
     static void DiagGCStart(int gen, bool isInduced);
     static void DiagUpdateGenerationBounds();
@@ -81,7 +78,8 @@ class GCToEEInterface
     static bool GetStringConfigValue(const char* key, const char** value);
     static void FreeStringConfigValue(const char* key);
     static bool IsGCThread();
-    static bool IsGCSpecialThread();
+    static bool WasCurrentThreadCreatedByGC();
+    static bool CreateThread(void (*threadStart)(void*), void* arg, bool is_suspendable, const char* name);
 };
 
 #endif // __GCENV_EE_H__

src/gc/env/gcenv.os.h

@@ -243,15 +243,6 @@ class GCToOSInterface
     // Thread and process
     //
 
-    // Create a new thread
-    // Parameters:
-    //  function - the function to be executed by the thread
-    //  param    - parameters of the thread
-    //  affinity - processor affinity of the thread
-    // Return:
-    //  true if it has succeeded, false if it has failed
-    static bool CreateThread(GCThreadFunction function, void* param, GCThreadAffinity* affinity);
-
     // Causes the calling thread to sleep for the specified number of milliseconds
     // Parameters:
     //  sleepMSec   - time to sleep before switching to another thread
@@ -307,6 +298,23 @@ class GCToOSInterface
     //  The number of processors
     static uint32_t GetCurrentProcessCpuCount();
 
+    // Sets the calling thread's affinity to only run on the processor specified
+    // in the GCThreadAffinity structure.
+    // Parameters:
+    //  affinity - The requested affinity for the calling thread. At most one processor
+    //             can be provided.
+    // Return:
+    //  true if setting the affinity was successful, false otherwise.
+    static bool SetThreadAffinity(GCThreadAffinity* affinity);
+
+    // Boosts the calling thread's thread priority to a level higher than the default
+    // for new threads.
+    // Parameters:
+    //  None.
+    // Return:
+    //  true if the priority boost was successful, false otherwise.
+    static bool BoostThreadPriority();
+
     // Get affinity mask of the current process
     // Parameters:
     //  processMask - affinity mask for the specified process

src/gc/gc.cpp

@@ -1740,7 +1740,7 @@ static BOOL try_enter_spin_lock(GCSpinLock *pSpinLock)
 inline
 static void leave_spin_lock(GCSpinLock *pSpinLock)
 {
-    bool gc_thread_p = GCToEEInterface::IsGCSpecialThread();
+    bool gc_thread_p = GCToEEInterface::WasCurrentThreadCreatedByGC();
 //    _ASSERTE((pSpinLock->holding_thread == GCToEEInterface::GetThread()) || gc_thread_p || pSpinLock->released_by_gc_p);
     pSpinLock->released_by_gc_p = gc_thread_p;
     pSpinLock->holding_thread = (Thread*) -1;
@@ -5351,23 +5351,7 @@ void set_thread_affinity_mask_for_heap(int heap_number, GCThreadAffinity* affini
 bool gc_heap::create_gc_thread ()
 {
     dprintf (3, ("Creating gc thread\n"));
-
-    GCThreadAffinity affinity;
-    affinity.Group = GCThreadAffinity::None;
-    affinity.Processor = GCThreadAffinity::None;
-
-    if (!gc_thread_no_affinitize_p)
-    {
-        // We are about to set affinity for GC threads. It is a good place to set up NUMA and
-        // CPU groups because the process mask, processor number, and group number are all
-        // readily available.
-        if (CPUGroupInfo::CanEnableGCCPUGroups()) 
-            set_thread_group_affinity_for_heap(heap_number, &affinity);
-        else
-            set_thread_affinity_mask_for_heap(heap_number, &affinity);
-    }
-
-    return GCToOSInterface::CreateThread(gc_thread_stub, this, &affinity);
+    return GCToEEInterface::CreateThread(gc_thread_stub, this, false, "Server GC");
 }
 
 #ifdef _MSC_VER
@@ -24917,27 +24901,29 @@ void gc_heap::compact_phase (int condemned_gen_number,
 #endif //_MSC_VER
 void gc_heap::gc_thread_stub (void* arg)
 {
-    ClrFlsSetThreadType (ThreadType_GC);
-    STRESS_LOG_RESERVE_MEM (GC_STRESSLOG_MULTIPLY);
+    gc_heap* heap = (gc_heap*)arg;
+    if (!gc_thread_no_affinitize_p)
+    {
+        GCThreadAffinity affinity;
+        affinity.Group = GCThreadAffinity::None;
+        affinity.Processor = GCThreadAffinity::None;
 
-#ifndef FEATURE_REDHAWK
-    // We commit the thread's entire stack to ensure we're robust in low memory conditions.
-    BOOL fSuccess = Thread::CommitThreadStack(NULL);
+        // We are about to set affinity for GC threads. It is a good place to set up NUMA and
+        // CPU groups because the process mask, processor number, and group number are all
+        // readily available.
+        if (CPUGroupInfo::CanEnableGCCPUGroups())
+            set_thread_group_affinity_for_heap(heap->heap_number, &affinity);
+        else
+            set_thread_affinity_mask_for_heap(heap->heap_number, &affinity);
 
-    if (!fSuccess)
-    {
-#ifdef BACKGROUND_GC
-        // For background GC we revert to doing a blocking GC.
-        return;
-#else
-        STRESS_LOG0(LF_GC, LL_ALWAYS, "Thread::CommitThreadStack failed.");
-        _ASSERTE(!"Thread::CommitThreadStack failed.");
-        GCToEEInterface::HandleFatalError(COR_E_STACKOVERFLOW);
-#endif //BACKGROUND_GC
+        if (!GCToOSInterface::SetThreadAffinity(&affinity))
+        {
+            dprintf(1, ("Failed to set thread affinity for server GC thread"));
+        }
     }
-#endif // FEATURE_REDHAWK
 
-    gc_heap* heap = (gc_heap*)arg;
+    // server GC threads run at a higher priority than normal.
+    GCToOSInterface::BoostThreadPriority();
     _alloca (256*heap->heap_number);
     heap->gc_thread_function();
 }
@@ -24953,10 +24939,12 @@ void gc_heap::gc_thread_stub (void* arg)
 #pragma warning(push)
 #pragma warning(disable:4702) // C4702: unreachable code: gc_thread_function may not return
 #endif //_MSC_VER
-uint32_t __stdcall gc_heap::bgc_thread_stub (void* arg)
+void gc_heap::bgc_thread_stub (void* arg)
 {
     gc_heap* heap = (gc_heap*)arg;
-    return heap->bgc_thread_function();
+    heap->bgc_thread = GCToEEInterface::GetThread();
+    assert(heap->bgc_thread != nullptr);
+    heap->bgc_thread_function();
 }
 #ifdef _MSC_VER
 #pragma warning(pop)
@@ -26700,7 +26688,6 @@ BOOL gc_heap::prepare_bgc_thread(gc_heap* gh)
     BOOL success = FALSE;
     BOOL thread_created = FALSE;
     dprintf (2, ("Preparing gc thread"));
-
     gh->bgc_threads_timeout_cs.Enter();
     if (!(gh->bgc_thread_running))
     {
@@ -26730,9 +26717,7 @@ BOOL gc_heap::create_bgc_thread(gc_heap* gh)
 
     //dprintf (2, ("Creating BGC thread"));
 
-    gh->bgc_thread = GCToEEInterface::CreateBackgroundThread(gh->bgc_thread_stub, gh);
-    gh->bgc_thread_running = (gh->bgc_thread != NULL);    
-
+    gh->bgc_thread_running = GCToEEInterface::CreateThread(gh->bgc_thread_stub, gh, true, "Background GC");
     return gh->bgc_thread_running;
 }
 
@@ -26908,7 +26893,7 @@ void gc_heap::kill_gc_thread()
     recursive_gc_sync::shutdown();
 }
 
-uint32_t gc_heap::bgc_thread_function()
+void gc_heap::bgc_thread_function()
 {
     assert (background_gc_done_event.IsValid());
     assert (bgc_start_event.IsValid());
@@ -27066,7 +27051,7 @@ uint32_t gc_heap::bgc_thread_function()
     FireEtwGCTerminateConcurrentThread_V1(GetClrInstanceId());
 
     dprintf (3, ("bgc_thread thread exiting"));
-    return 0;
+    return;
 }
 
 #endif //BACKGROUND_GC
@@ -34028,7 +34013,7 @@ bool GCHeap::StressHeap(gc_alloc_context * context)
 
 #ifdef BACKGROUND_GC
         // don't trigger a GC from the GC threads but still trigger GCs from user threads.
-        if (GCToEEInterface::IsGCSpecialThread())
+        if (GCToEEInterface::WasCurrentThreadCreatedByGC())
         {
             return FALSE;
         }

src/gc/gcenv.ee.standalone.inl

@@ -132,12 +132,6 @@ inline void GCToEEInterface::GcEnumAllocContexts(enum_alloc_context_func* fn, vo
     g_theGCToCLR->GcEnumAllocContexts(fn, param);
 }
 
-inline Thread* GCToEEInterface::CreateBackgroundThread(GCBackgroundThreadFunction threadStart, void* arg)
-{
-    assert(g_theGCToCLR != nullptr);
-    return g_theGCToCLR->CreateBackgroundThread(threadStart, arg);
-}
-
 inline void GCToEEInterface::DiagGCStart(int gen, bool isInduced)
 {
     assert(g_theGCToCLR != nullptr);
@@ -252,10 +246,17 @@ inline bool GCToEEInterface::IsGCThread()
     return g_theGCToCLR->IsGCThread();
 }
 
-inline bool GCToEEInterface::IsGCSpecialThread()
+inline bool GCToEEInterface::WasCurrentThreadCreatedByGC()
 {
     assert(g_theGCToCLR != nullptr);
-    return g_theGCToCLR->IsGCSpecialThread();
+    return g_theGCToCLR->WasCurrentThreadCreatedByGC();
 }
 
+inline bool GCToEEInterface::CreateThread(void (*threadStart)(void*), void* arg, bool is_suspendable, const char* name)
+{
+    assert(g_theGCToCLR != nullptr);
+    return g_theGCToCLR->CreateThread(threadStart, arg, is_suspendable, name);
+}
+
+
 #endif // __GCTOENV_EE_STANDALONE_INL__

src/gc/gcinterface.ee.h

@@ -79,6 +79,9 @@ class IGCToCLR {
 
     // Gets the Thread instance for the current thread, or null if no thread
     // instance is associated with this thread.
+    //
+    // If the GC created the current thread, GetThread returns null for threads
+    // that were not created as suspendable (see `IGCHeap::CreateThread`).
     virtual
     Thread* GetThread() = 0;
 
@@ -98,9 +101,21 @@ class IGCToCLR {
     virtual
     void GcEnumAllocContexts(enum_alloc_context_func* fn, void* param) = 0;
 
-    // Creates and returns a new background thread.
-    virtual
-    Thread* CreateBackgroundThread(GCBackgroundThreadFunction threadStart, void* arg) = 0;
+    // Creates and returns a new thread.
+    // Parameters:
+    //  threadStart - The function that will serve as the thread stub for the
+    //                new thread. It will be invoked immediately upon the
+    //                new thread upon creation.
+    //  arg - The argument that will be passed verbatim to threadStart.
+    //  is_suspendable - Whether or not the thread that is created should be suspendable
+    //                   from a runtime perspective. Threads that are suspendable have
+    //                   a VM Thread object associated with them that can be accessed
+    //                   using `IGCHeap::GetThread`.
+    //  name - The name of this thread, optionally used for diagnostic purposes.
+    // Returns:
+    //  true if the thread was started successfully, false if not.
+    virtual
+    bool CreateThread(void (*threadStart)(void*), void* arg, bool is_suspendable, const char* name) = 0;
 
     // When a GC starts, gives the diagnostics code a chance to run.
     virtual
@@ -192,16 +207,18 @@ class IGCToCLR {
     virtual
     void FreeStringConfigValue(const char* value) = 0;
 
-    // Asks the EE about whether or not the current thread is a GC thread:
-    // a server GC thread, background GC thread, or the thread that suspended
-    // the EE at the start of a GC.
+    // Returns true if this thread is a "GC thread", or a thread capable of
+    // doing GC work. Threads are either /always/ GC threads
+    // (if they were created for this purpose - background GC threads
+    // and server GC threads) or they became GC threads by suspending the EE
+    // and initiating a collection.
     virtual
     bool IsGCThread() = 0;
 
-    // Asks the EE about whether or not the current thread is a GC "special"
-    // thread: a server GC thread or a background GC thread.
+    // Returns true if the current thread is either a background GC thread
+    // or a server GC thread.
     virtual
-    bool IsGCSpecialThread() = 0;
+    bool WasCurrentThreadCreatedByGC() = 0;
 };
 
 #endif // _GCINTERFACE_EE_H_

src/gc/gcpriv.h

@@ -2671,11 +2671,11 @@ class gc_heap
     PER_HEAP
     void kill_gc_thread();
     PER_HEAP
-    uint32_t bgc_thread_function();
+    void bgc_thread_function();
     PER_HEAP_ISOLATED
     void do_background_gc();
     static
-    uint32_t __stdcall bgc_thread_stub (void* arg);
+    void bgc_thread_stub (void* arg);
 
 #endif //BACKGROUND_GC
 

src/gc/sample/gcenv.ee.cpp

@@ -231,12 +231,6 @@ void GCToEEInterface::SyncBlockCachePromotionsGranted(int /*max_gen*/)
 {
 }
 
-Thread* GCToEEInterface::CreateBackgroundThread(GCBackgroundThreadFunction threadStart, void* arg)
-{
-    // TODO: Implement for background GC
-    return NULL;
-}
-
 void GCToEEInterface::DiagGCStart(int gen, bool isInduced)
 {
 }
@@ -321,7 +315,7 @@ bool GCToEEInterface::IsGCThread()
     return false;
 }
 
-bool GCToEEInterface::IsGCSpecialThread()
+bool GCToEEInterface::WasCurrentThreadCreatedByGC()
 {
     return false;
 }
@@ -330,3 +324,8 @@ MethodTable* GCToEEInterface::GetFreeObjectMethodTable()
 {
     return g_pFreeObjectMethodTable;
 }
+
+bool GCToEEInterface::CreateThread(void (*threadStart)(void*), void* arg, bool is_suspendable, const char* name)
+{
+    return false;
+}