From 23b3d4199fd5fdf098ba41464e31d742e2977007 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Mon, 23 Sep 2024 23:50:13 -0700 Subject: [PATCH] Misc cleanup of VM threading code (#108171) - Delete special-casing of thread finalizer - Factor out Get/SetApartment of unstarted threads - Delete some dead code - Fix comments --- .../src/System/Threading/Thread.CoreCLR.cs | 5 +- .../Threading/Thread.NativeAot.Windows.cs | 21 +--- src/coreclr/vm/assembly.hpp | 5 - src/coreclr/vm/comsynchronizable.cpp | 54 +++++---- src/coreclr/vm/comutilnative.cpp | 4 +- src/coreclr/vm/finalizerthread.cpp | 61 ---------- src/coreclr/vm/methodtable.h | 9 -- src/coreclr/vm/threads.cpp | 107 +++++++----------- src/coreclr/vm/threads.h | 7 +- src/coreclr/vm/tieredcompilation.cpp | 2 +- .../System/Threading/WaitHandle.Windows.cs | 2 +- 11 files changed, 83 insertions(+), 194 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs index 08b3ae8944d63..9a75255b8e442 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs @@ -240,10 +240,7 @@ public ThreadPriority Priority { Thread _this = this; SetPriority(ObjectHandleOnStack.Create(ref _this), (int)value); - if (value != ThreadPriority.Normal) - { - _mayNeedResetForThreadPool = true; - } + _mayNeedResetForThreadPool = true; } } diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Thread.NativeAot.Windows.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Thread.NativeAot.Windows.cs index d6ea54412e105..d708a313b077c 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Thread.NativeAot.Windows.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Thread.NativeAot.Windows.cs @@ -14,9 +14,6 @@ namespace System.Threading { public sealed partial class Thread { - [ThreadStatic] - private static int t_reentrantWaitSuppressionCount; - [ThreadStatic] private static ApartmentType t_apartmentType; @@ -404,24 +401,8 @@ internal static Thread EnsureThreadPoolThreadInitialized() public void Interrupt() { throw new PlatformNotSupportedException(); } - // - // Suppresses reentrant waits on the current thread, until a matching call to RestoreReentrantWaits. - // This should be used by code that's expected to be called inside the STA message pump, so that it won't - // reenter itself. In an ASTA, this should only be the CCW implementations of IUnknown and IInspectable. - // - internal static void SuppressReentrantWaits() - { - t_reentrantWaitSuppressionCount++; - } - - internal static void RestoreReentrantWaits() - { - Debug.Assert(t_reentrantWaitSuppressionCount > 0); - t_reentrantWaitSuppressionCount--; - } - internal static bool ReentrantWaitsEnabled => - GetCurrentApartmentType() == ApartmentType.STA && t_reentrantWaitSuppressionCount == 0; + GetCurrentApartmentType() == ApartmentType.STA; internal static ApartmentType GetCurrentApartmentType() { diff --git a/src/coreclr/vm/assembly.hpp b/src/coreclr/vm/assembly.hpp index 441e1c828e202..20bcbbcaf6623 100644 --- a/src/coreclr/vm/assembly.hpp +++ b/src/coreclr/vm/assembly.hpp @@ -372,11 +372,6 @@ class Assembly OBJECTHANDLE GetLoaderAllocatorObjectHandle() { WRAPPER_NO_CONTRACT; return GetLoaderAllocator()->GetLoaderAllocatorObjectHandle(); } #endif // FEATURE_COLLECTIBLE_TYPES -#ifdef FEATURE_READYTORUN - BOOL IsInstrumented(); - BOOL IsInstrumentedHelper(); -#endif // FEATURE_READYTORUN - #ifdef FEATURE_COMINTEROP static ITypeLib * const InvalidTypeLib; diff --git a/src/coreclr/vm/comsynchronizable.cpp b/src/coreclr/vm/comsynchronizable.cpp index 7f1751aaa15f5..e87f93a04095a 100644 --- a/src/coreclr/vm/comsynchronizable.cpp +++ b/src/coreclr/vm/comsynchronizable.cpp @@ -40,15 +40,7 @@ static inline BOOL ThreadNotStarted(Thread *t) { WRAPPER_NO_CONTRACT; - return (t && t->IsUnstarted() && !t->HasValidThreadHandle()); -} - -static inline BOOL ThreadIsRunning(Thread *t) -{ - WRAPPER_NO_CONTRACT; - return (t && - (t->m_State & (Thread::TS_ReportDead|Thread::TS_Dead)) == 0 && - (t->HasValidThreadHandle())); + return (t && t->IsUnstarted()); } static inline BOOL ThreadIsDead(Thread *t) @@ -255,10 +247,10 @@ void ThreadNative::Start(Thread* pNewThread, int threadStackSize, int priority, #ifdef FEATURE_COMINTEROP_APARTMENT_SUPPORT // Attempt to eagerly set the apartment state during thread startup. - Thread::ApartmentState as = pNewThread->GetExplicitApartment(); + Thread::ApartmentState as = pNewThread->GetApartmentOfUnstartedThread(); if (as == Thread::AS_Unknown) { - pNewThread->SetApartment(Thread::AS_InMTA); + pNewThread->SetApartmentOfUnstartedThread(Thread::AS_InMTA); } #endif @@ -553,13 +545,23 @@ extern "C" INT32 QCALLTYPE ThreadNative_SetApartmentState(QCall::ObjectHandleOnS // We can only change the apartment if the thread is unstarted or // running, and if it's running we have to be in the thread's // context. - if (!ThreadNotStarted(thread) - && (!ThreadIsRunning(thread) || (GetThread() != thread))) + if (ThreadNotStarted(thread)) { - COMPlusThrow(kThreadStateException); + // Compat: Disallow resetting the initial apartment state + if (thread->GetApartmentOfUnstartedThread() == Thread::AS_Unknown) + thread->SetApartmentOfUnstartedThread((Thread::ApartmentState)iState); + + retVal = thread->GetApartmentOfUnstartedThread(); } + else + { + if (GetThread() != thread) + { + COMPlusThrow(kThreadStateException); + } - retVal = thread->SetApartment((Thread::ApartmentState)iState); + retVal = thread->SetApartment((Thread::ApartmentState)iState); + } END_QCALL; return retVal; @@ -713,19 +715,31 @@ void ThreadBaseObject::InitExisting() m_Priority = ThreadNative::PRIORITY_NORMAL; break; } - } FCIMPL1(void, ThreadNative::Finalize, ThreadBaseObject* pThisUNSAFE) { FCALL_CONTRACT; - // This function is intentionally blank. - // See comment in code:MethodTable::CallFinalizer. + THREADBASEREF refThis = (THREADBASEREF)pThisUNSAFE; + Thread* thread = refThis->GetInternal(); + + // Prevent multiple calls to Finalize + // Objects can be resurrected after being finalized. However, there is no + // race condition here. We always check whether an exposed thread object is + // still attached to the internal Thread object, before proceeding. + if (thread) + { + refThis->ResetStartHelper(); - _ASSERTE (!"Should not be called"); + if (GetThreadNULLOk() != thread) + { + refThis->ClearInternal(); + } - FCUnique(0x21); + thread->SetThreadState(Thread::TS_Finalized); + Thread::SetCleanupNeededForFinalizedThread(); + } } FCIMPLEND diff --git a/src/coreclr/vm/comutilnative.cpp b/src/coreclr/vm/comutilnative.cpp index f15b1085ebab4..c43bb35378f24 100644 --- a/src/coreclr/vm/comutilnative.cpp +++ b/src/coreclr/vm/comutilnative.cpp @@ -1401,7 +1401,7 @@ void GCInterface::RemoveMemoryPressure(UINT64 bytesAllocated) CONTRACTL { NOTHROW; - GC_TRIGGERS; + GC_NOTRIGGER; MODE_ANY; } CONTRACTL_END; @@ -1437,7 +1437,7 @@ NOINLINE void GCInterface::SendEtwRemoveMemoryPressureEvent(UINT64 bytesAllocate CONTRACTL { NOTHROW; - GC_TRIGGERS; + GC_NOTRIGGER; MODE_ANY; } CONTRACTL_END; diff --git a/src/coreclr/vm/finalizerthread.cpp b/src/coreclr/vm/finalizerthread.cpp index 97ace9a32353b..dc2ad02a9fc5e 100644 --- a/src/coreclr/vm/finalizerthread.cpp +++ b/src/coreclr/vm/finalizerthread.cpp @@ -52,42 +52,6 @@ BOOL FinalizerThread::HaveExtraWorkForFinalizer() return GetFinalizerThread()->HaveExtraWorkForFinalizer(); } -static void CallFinalizerOnThreadObject(OBJECTREF obj) -{ - STATIC_CONTRACT_MODE_COOPERATIVE; - - THREADBASEREF refThis = (THREADBASEREF)obj; - Thread* thread = refThis->GetInternal(); - - // Prevent multiple calls to Finalize - // Objects can be resurrected after being finalized. However, there is no - // race condition here. We always check whether an exposed thread object is - // still attached to the internal Thread object, before proceeding. - if (thread) - { - refThis->ResetStartHelper(); - - // During process shutdown, we finalize even reachable objects. But if we break - // the link between the System.Thread and the internal Thread object, the runtime - // may not work correctly. In particular, we won't be able to transition between - // contexts and domains to finalize other objects. Since the runtime doesn't - // require that Threads finalize during shutdown, we need to disable this. If - // we wait until phase 2 of shutdown finalization (when the EE is suspended and - // will never resume) then we can simply skip the side effects of Thread - // finalization. - if ((g_fEEShutDown & ShutDown_Finalize2) == 0) - { - if (GetThreadNULLOk() != thread) - { - refThis->ClearInternal(); - } - - thread->SetThreadState(Thread::TS_Finalized); - Thread::SetCleanupNeededForFinalizedThread(); - } - } -} - OBJECTREF FinalizerThread::GetNextFinalizableObject() { STATIC_CONTRACT_THROWS; @@ -138,20 +102,6 @@ OBJECTREF FinalizerThread::GetNextFinalizableObject() } while (pMTCur != NULL); } - - if (pMT == g_pThreadClass) - { - // Finalizing Thread object requires ThreadStoreLock. It is expensive if - // we keep taking ThreadStoreLock. This is very bad if we have high retiring - // rate of Thread objects. - // To avoid taking ThreadStoreLock multiple times, we mark Thread with TS_Finalized - // and clean up a batch of them when we take ThreadStoreLock next time. - - // To avoid possible hierarchy requirement between critical finalizers, we call cleanup - // code directly. - CallFinalizerOnThreadObject(obj); - goto Again; - } return obj; } @@ -426,19 +376,8 @@ DWORD WINAPI FinalizerThread::FinalizerThreadStart(void *args) ASSERT(args == 0); ASSERT(hEventFinalizer->IsValid()); - // TODO: The following line should be removed after contract violation is fixed. - // See bug 27409 - SCAN_IGNORE_THROW; - SCAN_IGNORE_TRIGGER; - LOG((LF_GC, LL_INFO10, "Finalizer thread starting...\n")); -#if defined(FEATURE_COMINTEROP_APARTMENT_SUPPORT) && !defined(FEATURE_COMINTEROP) - // Make sure the finalizer thread is set to MTA to avoid hitting - // DevDiv Bugs 180773 - [Stress Failure] AV at CoreCLR!SafeQueryInterfaceHelper - GetFinalizerThread()->SetApartment(Thread::AS_InMTA); -#endif // FEATURE_COMINTEROP_APARTMENT_SUPPORT && !FEATURE_COMINTEROP - s_FinalizerThreadOK = GetFinalizerThread()->HasStarted(); _ASSERTE(s_FinalizerThreadOK); diff --git a/src/coreclr/vm/methodtable.h b/src/coreclr/vm/methodtable.h index 496d216079237..6c454cf4ac149 100644 --- a/src/coreclr/vm/methodtable.h +++ b/src/coreclr/vm/methodtable.h @@ -2691,15 +2691,6 @@ class MethodTable void DebugDumpGCDesc(LPCUTF8 pszClassName, BOOL debug); #endif //_DEBUG - inline BOOL IsAgileAndFinalizable() - { - LIMITED_METHOD_CONTRACT; - // Right now, System.Thread is the only cases of this. - // Things should stay this way - please don't change without talking to EE team. - return this == g_pThreadClass; - } - - //------------------------------------------------------------------- // ENUMS, DELEGATES, VALUE TYPES, ARRAYS // diff --git a/src/coreclr/vm/threads.cpp b/src/coreclr/vm/threads.cpp index 99a366e9a2f13..c7ad8ef380d2c 100644 --- a/src/coreclr/vm/threads.cpp +++ b/src/coreclr/vm/threads.cpp @@ -4684,10 +4684,10 @@ Thread::ApartmentState Thread::GetApartment() return as; } - return GetApartmentRare(as); + return GetApartmentFromOS(); } -Thread::ApartmentState Thread::GetApartmentRare(Thread::ApartmentState as) +Thread::ApartmentState Thread::GetApartmentFromOS() { CONTRACTL { @@ -4702,54 +4702,25 @@ Thread::ApartmentState Thread::GetApartmentRare(Thread::ApartmentState as) THDTYPE type; HRESULT hr = S_OK; - if (as == AS_Unknown) + hr = GetCurrentThreadTypeNT5(&type); + if (hr == S_OK) { - hr = GetCurrentThreadTypeNT5(&type); - if (hr == S_OK) - { - as = (type == THDTYPE_PROCESSMESSAGES) ? AS_InSTA : AS_InMTA; - - // If we get back THDTYPE_PROCESSMESSAGES, we are guaranteed to - // be an STA thread. If not, we are an MTA thread, however - // we can't know if the thread has been explicitly set to MTA - // (via a call to CoInitializeEx) or if it has been implicitly - // made MTA (if it hasn't been CoInitializeEx'd but CoInitialize - // has already been called on some other thread in the process. - if (as == AS_InSTA) - SetThreadState(TS_InSTA); - } - } - } + ApartmentState as = (type == THDTYPE_PROCESSMESSAGES) ? AS_InSTA : AS_InMTA; - return as; -} + // If we get back THDTYPE_PROCESSMESSAGES, we are guaranteed to + // be an STA thread. If not, we are an MTA thread, however + // we can't know if the thread has been explicitly set to MTA + // (via a call to CoInitializeEx) or if it has been implicitly + // made MTA (if it hasn't been CoInitializeEx'd but CoInitialize + // has already been called on some other thread in the process. + if (as == AS_InSTA) + SetThreadState(TS_InSTA); -// Retrieve the explicit apartment state of the current thread. There are three possible -// states: thread hosts an STA, thread is part of the MTA or thread state is -// undecided. The last state may indicate that the apartment has not been set at -// all (nobody has called CoInitializeEx), the EE does not know the -// current state (EE has not called CoInitializeEx), or the thread is implicitly in -// the MTA. -Thread::ApartmentState Thread::GetExplicitApartment() -{ - CONTRACTL - { - NOTHROW; - GC_TRIGGERS; - MODE_ANY; + return as; + } } - CONTRACTL_END; - - _ASSERTE(!((m_State & TS_InSTA) && (m_State & TS_InMTA))); - // Initialize m_State by calling GetApartment. - GetApartment(); - - ApartmentState as = (m_State & TS_InSTA) ? AS_InSTA : - (m_State & TS_InMTA) ? AS_InMTA : - AS_Unknown; - - return as; + return AS_Unknown; } Thread::ApartmentState Thread::GetFinalApartment() @@ -4799,6 +4770,9 @@ Thread::ApartmentState Thread::SetApartment(ApartmentState state) } CONTRACTL_END; + // This method can be called on current thread only + _ASSERTE(m_OSThreadId == ::GetCurrentThreadId()); + // Reset any bits that request for CoInitialize ResetRequiresCoInitialize(); @@ -4819,14 +4793,6 @@ Thread::ApartmentState Thread::SetApartment(ApartmentState state) // If we haven't CoInitialized the thread, then we don't have anything to do. if (m_State & TS_CoInitialized) { - // We should never be attempting to CoUninitialize another thread than - // the currently running thread. -#ifdef TARGET_UNIX - _ASSERTE(m_OSThreadId == ::PAL_GetCurrentOSThreadId()); -#else - _ASSERTE(m_OSThreadId == ::GetCurrentThreadId()); -#endif - // CoUninitialize the thread and reset the STA/MTA/CoInitialized state bits. ::CoUninitialize(); @@ -4869,22 +4835,6 @@ Thread::ApartmentState Thread::SetApartment(ApartmentState state) return AS_InMTA; } - // If the thread isn't even started yet, we mark the state bits without - // calling CoInitializeEx (since we're obviously not in the correct thread - // context yet). We'll retry this call when the thread is started. - // Don't use the TS_Unstarted state bit to check for this, it's cleared far - // too late in the day for us. Instead check whether we're in the correct - // thread context. -#ifdef TARGET_UNIX - if (m_OSThreadId != ::PAL_GetCurrentOSThreadId()) -#else - if (m_OSThreadId != ::GetCurrentThreadId()) -#endif - { - SetThreadState((state == AS_InSTA) ? TS_InSTA : TS_InMTA); - return state; - } - HRESULT hr; { GCX_PREEMP(); @@ -4992,6 +4942,25 @@ Thread::ApartmentState Thread::SetApartment(ApartmentState state) return GetApartment(); } + +Thread::ApartmentState Thread::GetApartmentOfUnstartedThread() +{ + LIMITED_METHOD_CONTRACT; + _ASSERTE(IsUnstarted()); + ThreadState maskedTs = (ThreadState)(((DWORD)m_State) & (TS_InSTA|TS_InMTA)); + return (maskedTs != 0) ? TS_TO_AS(maskedTs) : AS_Unknown; +} + +void Thread::SetApartmentOfUnstartedThread(Thread::ApartmentState state) +{ + LIMITED_METHOD_CONTRACT; + _ASSERTE(IsUnstarted()); + _ASSERTE(GetApartmentOfUnstartedThread() == AS_Unknown); + if (state != AS_Unknown) + { + SetThreadState((state == AS_InSTA) ? TS_InSTA : TS_InMTA); + } +} #endif // FEATURE_COMINTEROP_APARTMENT_SUPPORT diff --git a/src/coreclr/vm/threads.h b/src/coreclr/vm/threads.h index 5c0f0bc8e1c31..8de07ab7e5cde 100644 --- a/src/coreclr/vm/threads.h +++ b/src/coreclr/vm/threads.h @@ -2158,8 +2158,7 @@ class Thread enum ApartmentState { AS_InSTA, AS_InMTA, AS_Unknown }; ApartmentState GetApartment(); - ApartmentState GetApartmentRare(Thread::ApartmentState as); - ApartmentState GetExplicitApartment(); + ApartmentState GetApartmentFromOS(); // Sets the apartment state if it has not already been set and // returns the state. @@ -2170,6 +2169,10 @@ class Thread // call CoInitializeEx on this thread first (note that calls to SetApartment made // before the thread has started are guaranteed to succeed). ApartmentState SetApartment(ApartmentState state); + + // Get/set apartment of a thread that was not started yet + ApartmentState GetApartmentOfUnstartedThread(); + void SetApartmentOfUnstartedThread(ApartmentState state); #endif // FEATURE_COMINTEROP_APARTMENT_SUPPORT // Either perform WaitForSingleObject or MsgWaitForSingleObject as appropriate. diff --git a/src/coreclr/vm/tieredcompilation.cpp b/src/coreclr/vm/tieredcompilation.cpp index 4a2fe0329d214..52db054296e9a 100644 --- a/src/coreclr/vm/tieredcompilation.cpp +++ b/src/coreclr/vm/tieredcompilation.cpp @@ -408,7 +408,7 @@ void TieredCompilationManager::CreateBackgroundWorker() _ASSERTE(newThread != nullptr); INDEBUG(s_backgroundWorkerThread = newThread); #ifdef FEATURE_COMINTEROP - newThread->SetApartment(Thread::AS_InMTA); + newThread->SetApartmentOfUnstartedThread(Thread::AS_InMTA); #endif newThread->SetBackground(true); diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/WaitHandle.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/WaitHandle.Windows.cs index e18bdebdd0359..d4d0197143763 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/WaitHandle.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/WaitHandle.Windows.cs @@ -26,7 +26,7 @@ private static unsafe int WaitForMultipleObjectsIgnoringSyncContext(IntPtr* pHan if (numHandles == 1) waitAll = false; -#if NATIVEAOT // TODO: reentrant wait support https://github.com/dotnet/runtime/issues/49518 +#if NATIVEAOT // TODO: reentrant wait support in Mono https://github.com/dotnet/runtime/issues/49518 // Trivial waits don't allow reentrance bool reentrantWait = !useTrivialWaits && Thread.ReentrantWaitsEnabled;