From f0b84a4fe60bb62fc7b3f2983b0aa2ced600224d Mon Sep 17 00:00:00 2001 From: Koundinya Veluri Date: Mon, 3 Oct 2016 20:41:39 -0700 Subject: [PATCH 1/5] Add heuristic to trigger GC to finalize dead threads and clean up handles and memory A thread that is started allocates some handles in Thread::AllocHandles(). After it terminates, the managed thread object needs to be collected by a GC for the handles to be released. Some applications that are mostly idle but have a timer ticking periodically will cause a thread pool thread to be created on each tick, since the thread pool preserves threads for a maximum of 20 seconds currently. Over time the number of handles accumulates to a high value. Thread creation adds some memory pressure, but that does not force a GC until a sufficiently large handle count, and for some mostly idle apps, that can be very long. The same issue arises with directly starting threads as well. Fixes #6602: - Track a dead thread count separately from the current dead thread count. This is the count that will contribute to triggering a GC, once it reaches a threshold. The count is tracked separately so that it may be reset to zero when a max-generation GC occurs, preventing dead threads that survive a GC due to references from contributing to triggering a GC again in this fashion. - If the count exceeds a threshold, enumerate dead threads to see which GC generation the corresponding managed thread objects are in. If the duration of time since the last GC of the desired generation also exceeds a threshold, trigger a preferably non-blocking GC on the finalizer thread. - Removed a couple of handles and some code paths specific to user-requested thread suspension, which is not supported on CoreCLR --- src/debug/daccess/dacdbiimpl.cpp | 5 ++ src/gc/env/gcenv.ee.h | 2 +- src/gc/gc.cpp | 3 +- src/gc/gc.h | 5 ++ src/gc/gccommon.cpp | 1 + src/gc/gcenv.ee.standalone.inl | 4 +- src/gc/gcinterface.h | 5 ++ src/gc/sample/gcenv.ee.cpp | 2 +- src/inc/clrconfigvalues.h | 6 ++ src/vm/comsynchronizable.cpp | 5 ++ src/vm/debugdebugger.cpp | 5 ++ src/vm/eedbginterfaceimpl.cpp | 5 ++ src/vm/finalizerthread.cpp | 4 ++ src/vm/gcenv.ee.cpp | 7 +- src/vm/gcenv.ee.h | 2 +- src/vm/threads.cpp | 118 ++++++++++++++++++++++++++++++- src/vm/threads.h | 39 ++++++++++ src/vm/threadsuspend.cpp | 54 ++++++++++++-- 18 files changed, 258 insertions(+), 14 deletions(-) diff --git a/src/debug/daccess/dacdbiimpl.cpp b/src/debug/daccess/dacdbiimpl.cpp index c7595c46c19a..eff9e02f9a49 100644 --- a/src/debug/daccess/dacdbiimpl.cpp +++ b/src/debug/daccess/dacdbiimpl.cpp @@ -5452,6 +5452,10 @@ CorDebugUserState DacDbiInterfaceImpl::GetPartialUserState(VMPTR_Thread vmThread result |= USER_WAIT_SLEEP_JOIN; } +#ifdef FEATURE_CORECLR + // CoreCLR does not support user-requested thread suspension + _ASSERTE(!(ts & Thread::TS_UserSuspendPending)); +#else // !FEATURE_CORECLR // Don't report a SuspendRequested if the thread has actually Suspended. if ((ts & Thread::TS_UserSuspendPending) && (ts & Thread::TS_SyncSuspended)) { @@ -5461,6 +5465,7 @@ CorDebugUserState DacDbiInterfaceImpl::GetPartialUserState(VMPTR_Thread vmThread { result |= USER_SUSPEND_REQUESTED; } +#endif // FEATURE_CORECLR if (pThread->IsThreadPoolThread()) { diff --git a/src/gc/env/gcenv.ee.h b/src/gc/env/gcenv.ee.h index 9f7f266a89f7..1f377bbe59ab 100644 --- a/src/gc/env/gcenv.ee.h +++ b/src/gc/env/gcenv.ee.h @@ -25,7 +25,7 @@ class GCToEEInterface // // start of GC call back - single threaded - static void GcStartWork(int condemned, int max_gen); + static void GcStartWork(int condemned, int min_finalizing_gen, int max_gen); //EE can perform post stack scanning action, while the // user threads are still suspended diff --git a/src/gc/gc.cpp b/src/gc/gc.cpp index 11faa79e4283..c9f55b60f4db 100644 --- a/src/gc/gc.cpp +++ b/src/gc/gc.cpp @@ -16734,7 +16734,8 @@ int gc_heap::garbage_collect (int n) // Call the EE for start of GC work // just one thread for MP GC GCToEEInterface::GcStartWork (settings.condemned_generation, - max_generation); + min_finalizing_generation, + max_generation); // TODO: we could fire an ETW event to say this GC as a concurrent GC but later on due to not being able to // create threads or whatever, this could be a non concurrent GC. Maybe for concurrent GC we should fire diff --git a/src/gc/gc.h b/src/gc/gc.h index d521f93a9962..62b8c4a42664 100644 --- a/src/gc/gc.h +++ b/src/gc/gc.h @@ -215,6 +215,11 @@ class IGCHeapInternal : public IGCHeap { virtual int GetHomeHeapNumber () = 0; virtual size_t GetPromotedBytes(int heap_index) = 0; + unsigned GetMinFinalizingGeneration() + { + return IGCHeap::minFinalizingGeneration; + } + unsigned GetMaxGeneration() { return IGCHeap::maxGeneration; diff --git a/src/gc/gccommon.cpp b/src/gc/gccommon.cpp index c1760515a69e..4440396938b5 100644 --- a/src/gc/gccommon.cpp +++ b/src/gc/gccommon.cpp @@ -18,6 +18,7 @@ SVAL_IMPL_INIT(uint32_t,IGCHeap,gcHeapType,IGCHeap::GC_HEAP_INVALID); #endif // FEATURE_SVR_GC +const uint32_t IGCHeap::minFinalizingGeneration = 1; SVAL_IMPL_INIT(uint32_t,IGCHeap,maxGeneration,2); IGCHeapInternal* g_theGCHeap; diff --git a/src/gc/gcenv.ee.standalone.inl b/src/gc/gcenv.ee.standalone.inl index 31f3d1d8daa9..5f064f1a4b2b 100644 --- a/src/gc/gcenv.ee.standalone.inl +++ b/src/gc/gcenv.ee.standalone.inl @@ -62,10 +62,10 @@ ALWAYS_INLINE void GCToEEInterface::GcScanRoots(promote_func* fn, int condemned, g_theGCToCLR->GcScanRoots(fn, condemned, max_gen, sc); } -ALWAYS_INLINE void GCToEEInterface::GcStartWork(int condemned, int max_gen) +ALWAYS_INLINE void GCToEEInterface::GcStartWork(int condemned, int min_finalizing_gen, int max_gen) { assert(g_theGCToCLR != nullptr); - g_theGCToCLR->GcStartWork(condemned, max_gen); + g_theGCToCLR->GcStartWork(condemned, int min_finalizing_gen, max_gen); } ALWAYS_INLINE void GCToEEInterface::AfterGcScanRoots(int condemned, int max_gen, ScanContext* sc) diff --git a/src/gc/gcinterface.h b/src/gc/gcinterface.h index f67aef711a20..60897c243975 100644 --- a/src/gc/gcinterface.h +++ b/src/gc/gcinterface.h @@ -163,6 +163,7 @@ struct segment_info // one for the object header, and one for the first field in the object. #define min_obj_size ((sizeof(uint8_t*) + sizeof(uintptr_t) + sizeof(size_t))) +#define min_finalizing_generation 1 #define max_generation 2 // The bit shift used to convert a memory address into an index into the @@ -526,6 +527,9 @@ class IGCHeap { // throughout the VM. virtual HRESULT GarbageCollect(int generation = -1, BOOL low_memory_p = FALSE, int mode = collection_blocking) = 0; + // Gets the smallest GC generation that runs finalizers. + virtual unsigned GetMinFinalizingGeneration() = 0; + // Gets the largest GC generation. Also used extensively throughout the VM. virtual unsigned GetMaxGeneration() = 0; @@ -727,6 +731,7 @@ class IGCHeap { SVAL_DECL(uint32_t, gcHeapType); #endif + static const uint32_t minFinalizingGeneration; SVAL_DECL(uint32_t, maxGeneration); }; diff --git a/src/gc/sample/gcenv.ee.cpp b/src/gc/sample/gcenv.ee.cpp index 8403bba7d104..c788ab69e9a0 100644 --- a/src/gc/sample/gcenv.ee.cpp +++ b/src/gc/sample/gcenv.ee.cpp @@ -154,7 +154,7 @@ void GCToEEInterface::GcScanRoots(promote_func* fn, int condemned, int max_gen, // TODO: Implement - Scan stack roots on given thread } -void GCToEEInterface::GcStartWork(int condemned, int max_gen) +void GCToEEInterface::GcStartWork(int condemned, int min_finalizing_gen, int max_gen) { } diff --git a/src/inc/clrconfigvalues.h b/src/inc/clrconfigvalues.h index 6160f2061aaf..b24ef3978078 100644 --- a/src/inc/clrconfigvalues.h +++ b/src/inc/clrconfigvalues.h @@ -936,6 +936,12 @@ CONFIG_DWORD_INFO(INTERNAL_SuspendDeadlockTimeout, W("SuspendDeadlockTimeout"), CONFIG_DWORD_INFO(INTERNAL_SuspendThreadDeadlockTimeoutMs, W("SuspendThreadDeadlockTimeoutMs"), 2000, "") RETAIL_CONFIG_DWORD_INFO(INTERNAL_ThreadSuspendInjection, W("INTERNAL_ThreadSuspendInjection"), 1, "Specifies whether to inject activations for thread suspension on Unix") +// +// Thread (miscellaneous) +// +RETAIL_CONFIG_DWORD_INFO(INTERNAL_Thread_DeadThreadCountThresholdForGCTrigger, W("Thread_DeadThreadCountThresholdForGCTrigger"), 75, "In the heuristics to clean up dead threads, this threshold must be reached before triggering a GC will be considered. Set to 0 to disable triggering a GC based on dead threads.") +RETAIL_CONFIG_DWORD_INFO(INTERNAL_Thread_DeadThreadGCTriggerPeriodMilliseconds, W("Thread_DeadThreadGCTriggerPeriodMilliseconds"), 1000 * 60 * 30, "In the heuristics to clean up dead threads, this much time must have elapsed since the previous max-generation GC before triggering another GC will be considered") + // // Threadpool // diff --git a/src/vm/comsynchronizable.cpp b/src/vm/comsynchronizable.cpp index bd24f6d39281..cf48fbb71ad7 100644 --- a/src/vm/comsynchronizable.cpp +++ b/src/vm/comsynchronizable.cpp @@ -926,6 +926,10 @@ FCIMPL1(INT32, ThreadNative::GetThreadState, ThreadBaseObject* pThisUNSAFE) if (state & Thread::TS_Interruptible) res |= ThreadWaitSleepJoin; +#ifdef FEATURE_CORECLR + // CoreCLR does not support user-requested thread suspension + _ASSERTE(!(state & Thread::TS_UserSuspendPending)); +#else // !FEATURE_CORECLR // Don't report a SuspendRequested if the thread has actually Suspended. if ((state & Thread::TS_UserSuspendPending) && (state & Thread::TS_SyncSuspended) @@ -938,6 +942,7 @@ FCIMPL1(INT32, ThreadNative::GetThreadState, ThreadBaseObject* pThisUNSAFE) { res |= ThreadSuspendRequested; } +#endif // FEATURE_CORECLR HELPER_METHOD_POLL(); HELPER_METHOD_FRAME_END(); diff --git a/src/vm/debugdebugger.cpp b/src/vm/debugdebugger.cpp index 32c6cd6cce4d..a4544191752e 100644 --- a/src/vm/debugdebugger.cpp +++ b/src/vm/debugdebugger.cpp @@ -937,6 +937,10 @@ void DebugStackTrace::GetStackFramesHelper(Frame *pStartFrame, goto LSafeToTrace; } +#ifdef FEATURE_CORECLR + // CoreCLR does not support user-requested thread suspension + _ASSERTE(!(state & Thread::TS_UserSuspendPending)); +#else // !FEATURE_CORECLR if (state & Thread::TS_UserSuspendPending) { if (state & Thread::TS_SyncSuspended) @@ -993,6 +997,7 @@ void DebugStackTrace::GetStackFramesHelper(Frame *pStartFrame, } #endif // DISABLE_THREADSUSPEND } +#endif // FEATURE_CORECLR COMPlusThrow(kThreadStateException, IDS_EE_THREAD_BAD_STATE); diff --git a/src/vm/eedbginterfaceimpl.cpp b/src/vm/eedbginterfaceimpl.cpp index b7a4359350be..c4ec9ed5f9cd 100644 --- a/src/vm/eedbginterfaceimpl.cpp +++ b/src/vm/eedbginterfaceimpl.cpp @@ -1588,6 +1588,10 @@ CorDebugUserState EEDbgInterfaceImpl::GetPartialUserState(Thread *pThread) ret |= (unsigned)USER_WAIT_SLEEP_JOIN; } +#ifdef FEATURE_CORECLR + // CoreCLR does not support user-requested thread suspension + _ASSERTE(!(ts & Thread::TS_UserSuspendPending)); +#else // !FEATURE_CORECLR // Don't report a SuspendRequested if the thread has actually Suspended. if ( ((ts & Thread::TS_UserSuspendPending) && (ts & Thread::TS_SyncSuspended))) { @@ -1597,6 +1601,7 @@ CorDebugUserState EEDbgInterfaceImpl::GetPartialUserState(Thread *pThread) { ret |= (unsigned)USER_SUSPEND_REQUESTED; } +#endif // FEATURE_CORECLR LOG((LF_CORDB,LL_INFO1000, "EEDbgII::GUS: thread 0x%x (id:0x%x)" " userThreadState is 0x%x\n", pThread, pThread->GetThreadId(), ret)); diff --git a/src/vm/finalizerthread.cpp b/src/vm/finalizerthread.cpp index 51a2ed49c0db..e799f8d4a759 100644 --- a/src/vm/finalizerthread.cpp +++ b/src/vm/finalizerthread.cpp @@ -1345,6 +1345,10 @@ BOOL FinalizerThread::FinalizerThreadWatchDogHelper() } ULONGLONG dwCurTickCount = CLRGetTickCount64(); if (pThread && pThread->m_State & (Thread::TS_UserSuspendPending | Thread::TS_DebugSuspendPending)) { +#ifdef FEATURE_CORECLR + // CoreCLR does not support user-requested thread suspension + _ASSERTE(!(pThread->m_State & Thread::TS_UserSuspendPending)); +#endif // FEATURE_CORECLR dwBeginTickCount = dwCurTickCount; } if (dwCurTickCount - dwBeginTickCount >= maxTotalWait) diff --git a/src/vm/gcenv.ee.cpp b/src/vm/gcenv.ee.cpp index 2833c99aa683..15e6801e078e 100644 --- a/src/vm/gcenv.ee.cpp +++ b/src/vm/gcenv.ee.cpp @@ -590,7 +590,7 @@ void GCToEEInterface::GcScanRoots(promote_func* fn, int condemned, int max_gen, } } -void GCToEEInterface::GcStartWork (int condemned, int max_gen) +void GCToEEInterface::GcStartWork (int condemned, int min_finalizing_gen, int max_gen) { CONTRACTL { @@ -626,6 +626,11 @@ void GCToEEInterface::GcStartWork (int condemned, int max_gen) // RCWWalker::OnGCStarted(condemned); #endif // FEATURE_COMINTEROP + + if (condemned >= min_finalizing_gen) + { + ThreadStore::s_pThreadStore->OnFinalizingGCStarted(); + } } void GCToEEInterface::GcDone(int condemned) diff --git a/src/vm/gcenv.ee.h b/src/vm/gcenv.ee.h index a7ab0b5ddae2..2569c16a7410 100644 --- a/src/vm/gcenv.ee.h +++ b/src/vm/gcenv.ee.h @@ -17,7 +17,7 @@ class GCToEEInterface : public IGCToCLR { void SuspendEE(SUSPEND_REASON reason); void RestartEE(bool bFinishedGC); void GcScanRoots(promote_func* fn, int condemned, int max_gen, ScanContext* sc); - void GcStartWork(int condemned, int max_gen); + void GcStartWork(int condemned, int min_finalizing_gen, int max_gen); void AfterGcScanRoots(int condemned, int max_gen, ScanContext* sc); void GcBeforeBGCSweepWork(); void GcDone(int condemned); diff --git a/src/vm/threads.cpp b/src/vm/threads.cpp index b5f750778b5f..4637d403b299 100644 --- a/src/vm/threads.cpp +++ b/src/vm/threads.cpp @@ -2170,21 +2170,26 @@ BOOL Thread::AllocHandles() { WRAPPER_NO_CONTRACT; +#ifndef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension _ASSERTE(!m_SafeEvent.IsValid()); _ASSERTE(!m_UserSuspendEvent.IsValid()); +#endif // !FEATURE_CORECLR _ASSERTE(!m_DebugSuspendEvent.IsValid()); _ASSERTE(!m_EventWait.IsValid()); BOOL fOK = TRUE; EX_TRY { +#ifndef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension // create a manual reset event for getting the thread to a safe point m_SafeEvent.CreateManualEvent(FALSE); m_UserSuspendEvent.CreateManualEvent(FALSE); +#endif // !FEATURE_CORECLR m_DebugSuspendEvent.CreateManualEvent(FALSE); m_EventWait.CreateManualEvent(TRUE); } EX_CATCH { fOK = FALSE; +#ifndef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension if (!m_SafeEvent.IsValid()) { m_SafeEvent.CloseEvent(); } @@ -2192,6 +2197,7 @@ BOOL Thread::AllocHandles() if (!m_UserSuspendEvent.IsValid()) { m_UserSuspendEvent.CloseEvent(); } +#endif // !FEATURE_CORECLR if (!m_DebugSuspendEvent.IsValid()) { m_DebugSuspendEvent.CloseEvent(); @@ -2404,6 +2410,10 @@ BOOL Thread::HasStarted(BOOL bRequiresTSL) } #endif // PROFILING_SUPPORTED +#ifdef FEATURE_CORECLR + // CoreCLR does not support user-requested thread suspension + _ASSERTE(!(m_State & TS_SuspendUnstarted)); +#else // !FEATURE_CORECLR // Is there a pending user suspension? if (m_State & TS_SuspendUnstarted) { @@ -2428,6 +2438,7 @@ BOOL Thread::HasStarted(BOOL bRequiresTSL) WaitSuspendEvents(); } } +#endif // FEATURE_CORECLR } return res; @@ -3064,6 +3075,7 @@ Thread::~Thread() CloseHandle(GetThreadHandle()); } +#ifndef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension if (m_SafeEvent.IsValid()) { m_SafeEvent.CloseEvent(); @@ -3072,6 +3084,7 @@ Thread::~Thread() { m_UserSuspendEvent.CloseEvent(); } +#endif // !FEATURE_CORECLR if (m_DebugSuspendEvent.IsValid()) { m_DebugSuspendEvent.CloseEvent(); @@ -3503,6 +3516,7 @@ void Thread::OnThreadTerminate(BOOL holdingLock) FastInterlockOr((ULONG *) &m_State, TS_Dead); ThreadStore::s_pThreadStore->m_DeadThreadCount++; + ThreadStore::s_pThreadStore->IncrementDeadThreadCountForGCTrigger(); if (IsUnstarted()) ThreadStore::s_pThreadStore->m_UnstartedThreadCount--; @@ -3527,8 +3541,13 @@ void Thread::OnThreadTerminate(BOOL holdingLock) if (m_State & TS_DebugSuspendPending) UnmarkForSuspension(~TS_DebugSuspendPending); +#ifdef FEATURE_CORECLR + // CoreCLR does not support user-requested thread suspension + _ASSERTE(!(m_State & TS_UserSuspendPending)); +#else // !FEATURE_CORECLR if (m_State & TS_UserSuspendPending) UnmarkForSuspension(~TS_UserSuspendPending); +#endif // FEATURE_CORECLR if (CurrentThreadID == ThisThreadID && IsAbortRequested()) { @@ -5739,6 +5758,8 @@ ThreadStore::ThreadStore() m_BackgroundThreadCount(0), m_PendingThreadCount(0), m_DeadThreadCount(0), + m_DeadThreadCountForGCTrigger(0), + m_TriggerFinalizingGC(false), m_GuidCreated(FALSE), m_HoldingThread(0) { @@ -5778,6 +5799,15 @@ void ThreadStore::InitThreadStore() s_pWaitForStackCrawlEvent = new CLREvent(); s_pWaitForStackCrawlEvent->CreateManualEvent(FALSE); + + s_DeadThreadCountThresholdForGCTrigger = + static_cast(CLRConfig::GetConfigValue(CLRConfig::INTERNAL_Thread_DeadThreadCountThresholdForGCTrigger)); + if (s_DeadThreadCountThresholdForGCTrigger < 0) + { + s_DeadThreadCountThresholdForGCTrigger = 0; + } + s_DeadThreadGCTriggerPeriodMilliseconds = + CLRConfig::GetConfigValue(CLRConfig::INTERNAL_Thread_DeadThreadGCTriggerPeriodMilliseconds); } // Enter and leave the critical section around the thread store. Clients should @@ -5920,7 +5950,10 @@ BOOL ThreadStore::RemoveThread(Thread *target) s_pThreadStore->m_ThreadCount--; if (target->IsDead()) + { s_pThreadStore->m_DeadThreadCount--; + s_pThreadStore->DecrementDeadThreadCountForGCTrigger(); + } // Unstarted threads are not in the Background count: if (target->IsUnstarted()) @@ -6009,6 +6042,80 @@ void ThreadStore::TransferStartedThread(Thread *thread, BOOL bRequiresTSL) CheckForEEShutdown(); } +LONG ThreadStore::s_DeadThreadCountThresholdForGCTrigger = 0; +DWORD ThreadStore::s_DeadThreadGCTriggerPeriodMilliseconds = 0; + +void ThreadStore::IncrementDeadThreadCountForGCTrigger() +{ + // Although all increments and decrements are usually done inside a lock, that is not sufficient to synchronize with a + // background GC thread resetting this value, hence the interlocked operation. Ignore overflow; overflow would likely never + // happen, the count is treated as unsigned, and nothing bad would happen if it were to overflow. + SIZE_T count = static_cast(FastInterlockIncrement(&m_DeadThreadCountForGCTrigger)); + + SIZE_T countThreshold = static_cast(s_DeadThreadCountThresholdForGCTrigger); + if (count < countThreshold || countThreshold == 0) + { + return; + } + + IGCHeap *gcHeap = GCHeapUtilities::GetGCHeap(); + SIZE_T millisecondsSinceLastFinalizingGC = + gcHeap->GetNow() - gcHeap->GetLastGCStartTime(gcHeap->GetMinFinalizingGeneration()); + if (millisecondsSinceLastFinalizingGC < s_DeadThreadGCTriggerPeriodMilliseconds) + { + return; + } + + if (!g_fEEStarted) + { + return; + } + + // The dead thread count used for triggering a GC is tracked separately from the actual dead thread count so that a dead + // thread does not contribute to triggering a GC more than once. The count exceeds a certain threshold and a finalizing GC + // has not occurred in a certain duration, so reset the count and trigger a GC now to finalize unreachable dead threads. + // The GC is triggered on the finalizer thread since it's not safe to trigger it on DLL_THREAD_DETACH. + m_DeadThreadCountForGCTrigger = 0; + m_TriggerFinalizingGC = true; + FinalizerThread::EnableFinalization(); +} + +void ThreadStore::DecrementDeadThreadCountForGCTrigger() +{ + // Although all increments and decrements are usually done inside a lock, that is not sufficient to synchronize with a + // background GC thread resetting this value, hence the interlocked operation. + if (FastInterlockDecrement(&m_DeadThreadCountForGCTrigger) < 0) + { + FastInterlockExchange(&m_DeadThreadCountForGCTrigger, 0); + } +} + +void ThreadStore::OnFinalizingGCStarted() +{ + // A dead thread may contribute to triggering at most once. After a finalizing GC occurs, if some dead thread objects are + // still reachable due to references to the thread objects, they will not contribute to triggering a GC again. Synchronize + // the store with increment/decrement operations occurring on different threads, and make the change visible to other + // threads in order to prevent unnecessary GC triggers. + FastInterlockExchange(&m_DeadThreadCountForGCTrigger, 0); +} + +bool ThreadStore::ShouldTriggerFinalizingGC() +{ + return m_TriggerFinalizingGC; +} + +void ThreadStore::TriggerFinalizingGCIfNecessary() +{ + if (!m_TriggerFinalizingGC) + { + return; + } + m_TriggerFinalizingGC = false; + + IGCHeap *gcHeap = GCHeapUtilities::GetGCHeap(); + gcHeap->GarbageCollect(gcHeap->GetMinFinalizingGeneration(), FALSE, collection_non_blocking); +} + #endif // #ifndef DACCESS_COMPILE @@ -6226,8 +6333,13 @@ BOOL ThreadStore::DbgFindThread(Thread *target) if (cur->m_State & Thread::TS_DebugSuspendPending) cntReturn++; +#ifdef FEATURE_CORECLR + // CoreCLR does not support user-requested thread suspension + _ASSERTE(!(cur->m_State & Thread::TS_UserSuspendPending)); +#else // !FEATURE_CORECLR if (cur->m_State & Thread::TS_UserSuspendPending) cntReturn++; +#endif // FEATURE_CORECLR if (cur->m_TraceCallCount > 0) cntReturn++; @@ -8697,7 +8809,8 @@ BOOL Thread::HaveExtraWorkForFinalizer() || Thread::CleanupNeededForFinalizedThread() || (m_DetachCount > 0) || AppDomain::HasWorkForFinalizerThread() - || SystemDomain::System()->RequireAppDomainCleanup(); + || SystemDomain::System()->RequireAppDomainCleanup() + || ThreadStore::s_pThreadStore->ShouldTriggerFinalizingGC(); } void Thread::DoExtraWorkForFinalizer() @@ -8754,7 +8867,8 @@ void Thread::DoExtraWorkForFinalizer() // If there were any TimerInfos waiting to be released, they'll get flushed now ThreadpoolMgr::FlushQueueOfTimerInfos(); - + + ThreadStore::s_pThreadStore->TriggerFinalizingGCIfNecessary(); } diff --git a/src/vm/threads.h b/src/vm/threads.h index ff0d1669ceec..020805b8e4ce 100644 --- a/src/vm/threads.h +++ b/src/vm/threads.h @@ -3712,9 +3712,15 @@ class Thread: public IUnknown void SetupForSuspension(ULONG bit) { WRAPPER_NO_CONTRACT; + +#ifdef FEATURE_CORECLR + // CoreCLR does not support user-requested thread suspension + _ASSERTE(!(bit & TS_UserSuspendPending)); +#else // !FEATURE_CORECLR if (bit & TS_UserSuspendPending) { m_UserSuspendEvent.Reset(); } +#endif // FEATURE_CORECLR if (bit & TS_DebugSuspendPending) { m_DebugSuspendEvent.Reset(); } @@ -3731,8 +3737,18 @@ class Thread: public IUnknown // ThreadState oldState = m_State; +#ifdef FEATURE_CORECLR + // CoreCLR does not support user-requested thread suspension + _ASSERTE(!(oldState & TS_UserSuspendPending)); +#endif // FEATURE_CORECLR + while ((oldState & (TS_UserSuspendPending | TS_DebugSuspendPending)) == 0) { +#ifdef FEATURE_CORECLR + // CoreCLR does not support user-requested thread suspension + _ASSERTE(!(oldState & TS_UserSuspendPending)); +#endif // FEATURE_CORECLR + // // Construct the destination state we desire - all suspension bits turned off. // @@ -3751,9 +3767,14 @@ class Thread: public IUnknown oldState = m_State; } +#ifdef FEATURE_CORECLR + // CoreCLR does not support user-requested thread suspension + _ASSERTE(!(bit & TS_UserSuspendPending)); +#else // !FEATURE_CORECLR if (bit & TS_UserSuspendPending) { m_UserSuspendEvent.Set(); } +#endif // FEATURE_CORECLR if (bit & TS_DebugSuspendPending) { m_DebugSuspendEvent.Set(); @@ -3761,9 +3782,11 @@ class Thread: public IUnknown } +#ifndef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension // For getting a thread to a safe point. A client waits on the event, which is // set by the thread when it reaches a safe spot. void SetSafeEvent(); +#endif // !FEATURE_CORECLR public: FORCEINLINE void UnhijackThreadNoAlloc() @@ -3885,8 +3908,10 @@ class Thread: public IUnknown private: // For suspends: +#ifndef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension CLREvent m_SafeEvent; CLREvent m_UserSuspendEvent; +#endif // !FEATURE_CORECLR CLREvent m_DebugSuspendEvent; // For Object::Wait, Notify and NotifyAll, we use an Event inside the @@ -5516,6 +5541,8 @@ class ThreadStore LONG m_PendingThreadCount; LONG m_DeadThreadCount; + LONG m_DeadThreadCountForGCTrigger; + bool m_TriggerFinalizingGC; private: // Space for the lazily-created GUID. @@ -5528,6 +5555,10 @@ class ThreadStore Thread *m_HoldingThread; EEThreadId m_holderthreadid; // current holder (or NULL) +private: + static LONG s_DeadThreadCountThresholdForGCTrigger; + static DWORD s_DeadThreadGCTriggerPeriodMilliseconds; + public: static BOOL HoldingThreadStore() @@ -5601,6 +5632,14 @@ class ThreadStore LIMITED_METHOD_CONTRACT; s_pWaitForStackCrawlEvent->Reset(); } + +private: + void IncrementDeadThreadCountForGCTrigger(); + void DecrementDeadThreadCountForGCTrigger(); +public: + void OnFinalizingGCStarted(); + bool ShouldTriggerFinalizingGC(); + void TriggerFinalizingGCIfNecessary(); }; struct TSSuspendHelper { diff --git a/src/vm/threadsuspend.cpp b/src/vm/threadsuspend.cpp index 10032fd08f22..cf7d15a91404 100644 --- a/src/vm/threadsuspend.cpp +++ b/src/vm/threadsuspend.cpp @@ -1978,6 +1978,11 @@ Thread::UserAbort(ThreadAbortRequester requester, m_dwAbortPoint = 7; #endif +#ifdef FEATURE_CORECLR + // CoreCLR does not support user-requested thread suspension + _ASSERTE(!(m_State & TS_UserSuspendPending)); +#endif // FEATURE_CORECLR + // // If it's stopped by the debugger, we don't want to throw an exception. // Debugger suspension is to have no effect of the runtime behaviour. @@ -3090,6 +3095,11 @@ void Thread::RareDisablePreemptiveGC() goto Exit; } +#ifdef FEATURE_CORECLR + // CoreCLR does not support user-requested thread suspension + _ASSERTE(!(m_State & TS_UserSuspendPending)); +#endif // !FEATURE_CORECLR + // Note IsGCInProgress is also true for say Pause (anywhere SuspendEE happens) and GCThread is the // thread that did the Pause. While in Pause if another thread attempts Rev/Pinvoke it should get inside the following and // block until resume @@ -3105,6 +3115,11 @@ void Thread::RareDisablePreemptiveGC() do { +#ifdef FEATURE_CORECLR + // CoreCLR does not support user-requested thread suspension + _ASSERTE(!(m_State & TS_UserSuspendPending)); +#endif // !FEATURE_CORECLR + EnablePreemptiveGC(); // Cannot use GCX_PREEMP_NO_DTOR here because we're inside of the thread @@ -3558,7 +3573,9 @@ void Thread::RareEnablePreemptiveGC() #endif // FEATURE_HIJACK // wake up any threads waiting to suspend us, like the GC thread. +#ifndef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension SetSafeEvent(); +#endif // !FEATURE_CORECLR ThreadSuspend::g_pGCSuspendEvent->Set(); // for GC, the fact that we are leaving the EE means that it no longer needs to @@ -3566,6 +3583,11 @@ void Thread::RareEnablePreemptiveGC() // Give the debugger precedence over user suspensions: while (m_State & (TS_DebugSuspendPending | TS_UserSuspendPending)) { +#ifdef FEATURE_CORECLR + // CoreCLR does not support user-requested thread suspension + _ASSERTE(!(m_State & TS_UserSuspendPending)); +#endif // !FEATURE_CORECLR + #ifdef DEBUGGING_SUPPORTED // We don't notify the debugger that this thread is now suspended. We'll just // let the debugger's helper thread sweep and pick it up. @@ -3903,12 +3925,14 @@ void __stdcall Thread::RedirectedHandledJITCase(RedirectReason reason) // Enable PGC before calling out to the client to allow runtime suspend to finish GCX_PREEMP_NO_DTOR(); +#ifndef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension // @TODO: Is this necessary? Does debugger wait on the events, or does it just // poll every so often? // Notify the thread that is performing the suspension that this thread // is now in PGC mode and that it can remove this thread from the list of // threads it needs to wait for. pThread->SetSafeEvent(); +#endif // !FEATURE_CORECLR // Notify the interface of the pending suspension switch (reason) { @@ -6016,8 +6040,7 @@ void Thread::SysResumeFromDebug(AppDomain *pAppDomain) LOG((LF_CORDB, LL_INFO1000, "RESUME: resume complete. Trap count: %d\n", g_TrapReturningThreads.Load())); } - - +#ifndef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension void Thread::SetSafeEvent() { CONTRACTL { @@ -6028,7 +6051,7 @@ void Thread::SetSafeEvent() m_SafeEvent.Set(); } - +#endif // !FEATURE_CORECLR /* * @@ -6056,6 +6079,10 @@ BOOL Thread::WaitSuspendEventsHelper(void) EX_TRY { +#ifdef FEATURE_CORECLR + // CoreCLR does not support user-requested thread suspension + _ASSERTE(!(m_State & TS_UserSuspendPending)); +#else // !FEATURE_CORECLR if (m_State & TS_UserSuspendPending) { ThreadState oldState = m_State; @@ -6076,8 +6103,9 @@ BOOL Thread::WaitSuspendEventsHelper(void) oldState = m_State; } - - } else if (m_State & TS_DebugSuspendPending) { + } else +#endif // FEATURE_CORECLR + if (m_State & TS_DebugSuspendPending) { ThreadState oldState = m_State; @@ -6127,6 +6155,11 @@ void Thread::WaitSuspendEvents(BOOL fDoWait) ThreadState oldState = m_State; +#ifdef FEATURE_CORECLR + // CoreCLR does not support user-requested thread suspension + _ASSERTE(!(oldState & TS_UserSuspendPending)); +#endif // !FEATURE_CORECLR + // // If all reasons to suspend are off, we think we can exit // this loop, but we need to check atomically. @@ -7106,9 +7139,15 @@ void Thread::MarkForSuspension(ULONG bit) } CONTRACTL_END; +#ifdef FEATURE_CORECLR + // CoreCLR does not support user-requested thread suspension + _ASSERTE(bit == TS_DebugSuspendPending || + bit == (TS_DebugSuspendPending | TS_DebugWillSync)); +#else // !FEATURE_CORECLR _ASSERTE(bit == TS_DebugSuspendPending || bit == (TS_DebugSuspendPending | TS_DebugWillSync) || bit == TS_UserSuspendPending); +#endif // FEATURE_CORECLR _ASSERTE(IsAtProcessExit() || ThreadStore::HoldingThreadStore()); @@ -7126,8 +7165,13 @@ void Thread::UnmarkForSuspension(ULONG mask) } CONTRACTL_END; +#ifdef FEATURE_CORECLR + // CoreCLR does not support user-requested thread suspension + _ASSERTE(mask == ~TS_DebugSuspendPending); +#else // !FEATURE_CORECLR _ASSERTE(mask == ~TS_DebugSuspendPending || mask == ~TS_UserSuspendPending); +#endif // FEATURE_CORECLR _ASSERTE(IsAtProcessExit() || ThreadStore::HoldingThreadStore()); From 8fca6ac536b7a9fc52ba00de46ad9a522d74a3ec Mon Sep 17 00:00:00 2001 From: Koundinya Veluri Date: Thu, 23 Mar 2017 13:12:57 -0700 Subject: [PATCH 2/5] Address some feedback --- src/debug/daccess/dacdbiimpl.cpp | 12 ---- src/gc/env/gcenv.ee.h | 2 +- src/gc/gc.cpp | 3 +- src/gc/gc.h | 5 -- src/gc/gccommon.cpp | 1 - src/gc/gcenv.ee.standalone.inl | 4 +- src/gc/gcinterface.h | 5 -- src/gc/sample/gcenv.ee.cpp | 2 +- src/vm/comsynchronizable.cpp | 15 ----- src/vm/debugdebugger.cpp | 59 ----------------- src/vm/eedbginterfaceimpl.cpp | 12 ---- src/vm/finalizerthread.cpp | 2 - src/vm/gcenv.ee.cpp | 7 +- src/vm/gcenv.ee.h | 2 +- src/vm/threads.cpp | 110 +++++++------------------------ src/vm/threads.h | 36 ++-------- src/vm/threadsuspend.cpp | 69 ------------------- 17 files changed, 39 insertions(+), 307 deletions(-) diff --git a/src/debug/daccess/dacdbiimpl.cpp b/src/debug/daccess/dacdbiimpl.cpp index eff9e02f9a49..74ba14f2eb74 100644 --- a/src/debug/daccess/dacdbiimpl.cpp +++ b/src/debug/daccess/dacdbiimpl.cpp @@ -5452,20 +5452,8 @@ CorDebugUserState DacDbiInterfaceImpl::GetPartialUserState(VMPTR_Thread vmThread result |= USER_WAIT_SLEEP_JOIN; } -#ifdef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension _ASSERTE(!(ts & Thread::TS_UserSuspendPending)); -#else // !FEATURE_CORECLR - // Don't report a SuspendRequested if the thread has actually Suspended. - if ((ts & Thread::TS_UserSuspendPending) && (ts & Thread::TS_SyncSuspended)) - { - result |= USER_SUSPENDED; - } - else if (ts & Thread::TS_UserSuspendPending) - { - result |= USER_SUSPEND_REQUESTED; - } -#endif // FEATURE_CORECLR if (pThread->IsThreadPoolThread()) { diff --git a/src/gc/env/gcenv.ee.h b/src/gc/env/gcenv.ee.h index 1f377bbe59ab..9f7f266a89f7 100644 --- a/src/gc/env/gcenv.ee.h +++ b/src/gc/env/gcenv.ee.h @@ -25,7 +25,7 @@ class GCToEEInterface // // start of GC call back - single threaded - static void GcStartWork(int condemned, int min_finalizing_gen, int max_gen); + static void GcStartWork(int condemned, int max_gen); //EE can perform post stack scanning action, while the // user threads are still suspended diff --git a/src/gc/gc.cpp b/src/gc/gc.cpp index c9f55b60f4db..11faa79e4283 100644 --- a/src/gc/gc.cpp +++ b/src/gc/gc.cpp @@ -16734,8 +16734,7 @@ int gc_heap::garbage_collect (int n) // Call the EE for start of GC work // just one thread for MP GC GCToEEInterface::GcStartWork (settings.condemned_generation, - min_finalizing_generation, - max_generation); + max_generation); // TODO: we could fire an ETW event to say this GC as a concurrent GC but later on due to not being able to // create threads or whatever, this could be a non concurrent GC. Maybe for concurrent GC we should fire diff --git a/src/gc/gc.h b/src/gc/gc.h index 62b8c4a42664..d521f93a9962 100644 --- a/src/gc/gc.h +++ b/src/gc/gc.h @@ -215,11 +215,6 @@ class IGCHeapInternal : public IGCHeap { virtual int GetHomeHeapNumber () = 0; virtual size_t GetPromotedBytes(int heap_index) = 0; - unsigned GetMinFinalizingGeneration() - { - return IGCHeap::minFinalizingGeneration; - } - unsigned GetMaxGeneration() { return IGCHeap::maxGeneration; diff --git a/src/gc/gccommon.cpp b/src/gc/gccommon.cpp index 4440396938b5..c1760515a69e 100644 --- a/src/gc/gccommon.cpp +++ b/src/gc/gccommon.cpp @@ -18,7 +18,6 @@ SVAL_IMPL_INIT(uint32_t,IGCHeap,gcHeapType,IGCHeap::GC_HEAP_INVALID); #endif // FEATURE_SVR_GC -const uint32_t IGCHeap::minFinalizingGeneration = 1; SVAL_IMPL_INIT(uint32_t,IGCHeap,maxGeneration,2); IGCHeapInternal* g_theGCHeap; diff --git a/src/gc/gcenv.ee.standalone.inl b/src/gc/gcenv.ee.standalone.inl index 5f064f1a4b2b..31f3d1d8daa9 100644 --- a/src/gc/gcenv.ee.standalone.inl +++ b/src/gc/gcenv.ee.standalone.inl @@ -62,10 +62,10 @@ ALWAYS_INLINE void GCToEEInterface::GcScanRoots(promote_func* fn, int condemned, g_theGCToCLR->GcScanRoots(fn, condemned, max_gen, sc); } -ALWAYS_INLINE void GCToEEInterface::GcStartWork(int condemned, int min_finalizing_gen, int max_gen) +ALWAYS_INLINE void GCToEEInterface::GcStartWork(int condemned, int max_gen) { assert(g_theGCToCLR != nullptr); - g_theGCToCLR->GcStartWork(condemned, int min_finalizing_gen, max_gen); + g_theGCToCLR->GcStartWork(condemned, max_gen); } ALWAYS_INLINE void GCToEEInterface::AfterGcScanRoots(int condemned, int max_gen, ScanContext* sc) diff --git a/src/gc/gcinterface.h b/src/gc/gcinterface.h index 60897c243975..f67aef711a20 100644 --- a/src/gc/gcinterface.h +++ b/src/gc/gcinterface.h @@ -163,7 +163,6 @@ struct segment_info // one for the object header, and one for the first field in the object. #define min_obj_size ((sizeof(uint8_t*) + sizeof(uintptr_t) + sizeof(size_t))) -#define min_finalizing_generation 1 #define max_generation 2 // The bit shift used to convert a memory address into an index into the @@ -527,9 +526,6 @@ class IGCHeap { // throughout the VM. virtual HRESULT GarbageCollect(int generation = -1, BOOL low_memory_p = FALSE, int mode = collection_blocking) = 0; - // Gets the smallest GC generation that runs finalizers. - virtual unsigned GetMinFinalizingGeneration() = 0; - // Gets the largest GC generation. Also used extensively throughout the VM. virtual unsigned GetMaxGeneration() = 0; @@ -731,7 +727,6 @@ class IGCHeap { SVAL_DECL(uint32_t, gcHeapType); #endif - static const uint32_t minFinalizingGeneration; SVAL_DECL(uint32_t, maxGeneration); }; diff --git a/src/gc/sample/gcenv.ee.cpp b/src/gc/sample/gcenv.ee.cpp index c788ab69e9a0..8403bba7d104 100644 --- a/src/gc/sample/gcenv.ee.cpp +++ b/src/gc/sample/gcenv.ee.cpp @@ -154,7 +154,7 @@ void GCToEEInterface::GcScanRoots(promote_func* fn, int condemned, int max_gen, // TODO: Implement - Scan stack roots on given thread } -void GCToEEInterface::GcStartWork(int condemned, int min_finalizing_gen, int max_gen) +void GCToEEInterface::GcStartWork(int condemned, int max_gen) { } diff --git a/src/vm/comsynchronizable.cpp b/src/vm/comsynchronizable.cpp index cf48fbb71ad7..95119adfe57b 100644 --- a/src/vm/comsynchronizable.cpp +++ b/src/vm/comsynchronizable.cpp @@ -926,23 +926,8 @@ FCIMPL1(INT32, ThreadNative::GetThreadState, ThreadBaseObject* pThisUNSAFE) if (state & Thread::TS_Interruptible) res |= ThreadWaitSleepJoin; -#ifdef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension _ASSERTE(!(state & Thread::TS_UserSuspendPending)); -#else // !FEATURE_CORECLR - // Don't report a SuspendRequested if the thread has actually Suspended. - if ((state & Thread::TS_UserSuspendPending) && - (state & Thread::TS_SyncSuspended) - ) - { - res |= ThreadSuspended; - } - else - if (state & Thread::TS_UserSuspendPending) - { - res |= ThreadSuspendRequested; - } -#endif // FEATURE_CORECLR HELPER_METHOD_POLL(); HELPER_METHOD_FRAME_END(); diff --git a/src/vm/debugdebugger.cpp b/src/vm/debugdebugger.cpp index a4544191752e..489c623df9a9 100644 --- a/src/vm/debugdebugger.cpp +++ b/src/vm/debugdebugger.cpp @@ -937,67 +937,8 @@ void DebugStackTrace::GetStackFramesHelper(Frame *pStartFrame, goto LSafeToTrace; } -#ifdef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension _ASSERTE(!(state & Thread::TS_UserSuspendPending)); -#else // !FEATURE_CORECLR - if (state & Thread::TS_UserSuspendPending) - { - if (state & Thread::TS_SyncSuspended) - { - goto LSafeToTrace; - } - -#ifndef DISABLE_THREADSUSPEND - // On Mac don't perform the optimization below, but rather wait for - // the suspendee to set the TS_SyncSuspended flag - - // The target thread is not actually suspended yet, but if it is - // in preemptive mode, then it is still safe to trace. Before we - // can look at another thread's GC mode, we have to suspend it: - // The target thread updates its GC mode flag with non-interlocked - // operations, and Thread::SuspendThread drains the CPU's store - // buffer (by virtue of calling GetThreadContext). - switch (pThread->SuspendThread()) - { - case Thread::STR_Success: - if (!pThread->PreemptiveGCDisabledOther()) - { - pThread->ResumeThread(); - goto LSafeToTrace; - } - - // Refuse to trace the stack. - // - // Note that there is a pretty large window in-between when the - // target thread sets the GC mode to cooperative, and when it - // actually sets the TS_SyncSuspended bit. In this window, we - // will refuse to take a stack trace even though it would be - // safe to do so. - pThread->ResumeThread(); - break; - case Thread::STR_Failure: - case Thread::STR_NoStressLog: - break; - case Thread::STR_UnstartedOrDead: - // We know the thread is not unstarted, because we checked for - // TS_Unstarted above. - _ASSERTE(!(state & Thread::TS_Unstarted)); - - // Since the thread is dead, it is safe to trace. - goto LSafeToTrace; - case Thread::STR_SwitchedOut: - if (!pThread->PreemptiveGCDisabledOther()) - { - goto LSafeToTrace; - } - break; - default: - UNREACHABLE(); - } -#endif // DISABLE_THREADSUSPEND - } -#endif // FEATURE_CORECLR COMPlusThrow(kThreadStateException, IDS_EE_THREAD_BAD_STATE); diff --git a/src/vm/eedbginterfaceimpl.cpp b/src/vm/eedbginterfaceimpl.cpp index c4ec9ed5f9cd..ede82c778003 100644 --- a/src/vm/eedbginterfaceimpl.cpp +++ b/src/vm/eedbginterfaceimpl.cpp @@ -1588,20 +1588,8 @@ CorDebugUserState EEDbgInterfaceImpl::GetPartialUserState(Thread *pThread) ret |= (unsigned)USER_WAIT_SLEEP_JOIN; } -#ifdef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension _ASSERTE(!(ts & Thread::TS_UserSuspendPending)); -#else // !FEATURE_CORECLR - // Don't report a SuspendRequested if the thread has actually Suspended. - if ( ((ts & Thread::TS_UserSuspendPending) && (ts & Thread::TS_SyncSuspended))) - { - ret |= (unsigned)USER_SUSPENDED; - } - else if (ts & Thread::TS_UserSuspendPending) - { - ret |= (unsigned)USER_SUSPEND_REQUESTED; - } -#endif // FEATURE_CORECLR LOG((LF_CORDB,LL_INFO1000, "EEDbgII::GUS: thread 0x%x (id:0x%x)" " userThreadState is 0x%x\n", pThread, pThread->GetThreadId(), ret)); diff --git a/src/vm/finalizerthread.cpp b/src/vm/finalizerthread.cpp index e799f8d4a759..d111eebd3c54 100644 --- a/src/vm/finalizerthread.cpp +++ b/src/vm/finalizerthread.cpp @@ -1345,10 +1345,8 @@ BOOL FinalizerThread::FinalizerThreadWatchDogHelper() } ULONGLONG dwCurTickCount = CLRGetTickCount64(); if (pThread && pThread->m_State & (Thread::TS_UserSuspendPending | Thread::TS_DebugSuspendPending)) { -#ifdef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension _ASSERTE(!(pThread->m_State & Thread::TS_UserSuspendPending)); -#endif // FEATURE_CORECLR dwBeginTickCount = dwCurTickCount; } if (dwCurTickCount - dwBeginTickCount >= maxTotalWait) diff --git a/src/vm/gcenv.ee.cpp b/src/vm/gcenv.ee.cpp index 15e6801e078e..6ca8835246eb 100644 --- a/src/vm/gcenv.ee.cpp +++ b/src/vm/gcenv.ee.cpp @@ -590,7 +590,7 @@ void GCToEEInterface::GcScanRoots(promote_func* fn, int condemned, int max_gen, } } -void GCToEEInterface::GcStartWork (int condemned, int min_finalizing_gen, int max_gen) +void GCToEEInterface::GcStartWork (int condemned, int max_gen) { CONTRACTL { @@ -627,10 +627,7 @@ void GCToEEInterface::GcStartWork (int condemned, int min_finalizing_gen, int ma RCWWalker::OnGCStarted(condemned); #endif // FEATURE_COMINTEROP - if (condemned >= min_finalizing_gen) - { - ThreadStore::s_pThreadStore->OnFinalizingGCStarted(); - } + ThreadStore::s_pThreadStore->OnGCStarted(condemned); } void GCToEEInterface::GcDone(int condemned) diff --git a/src/vm/gcenv.ee.h b/src/vm/gcenv.ee.h index 2569c16a7410..a7ab0b5ddae2 100644 --- a/src/vm/gcenv.ee.h +++ b/src/vm/gcenv.ee.h @@ -17,7 +17,7 @@ class GCToEEInterface : public IGCToCLR { void SuspendEE(SUSPEND_REASON reason); void RestartEE(bool bFinishedGC); void GcScanRoots(promote_func* fn, int condemned, int max_gen, ScanContext* sc); - void GcStartWork(int condemned, int min_finalizing_gen, int max_gen); + void GcStartWork(int condemned, int max_gen); void AfterGcScanRoots(int condemned, int max_gen, ScanContext* sc); void GcBeforeBGCSweepWork(); void GcDone(int condemned); diff --git a/src/vm/threads.cpp b/src/vm/threads.cpp index 4637d403b299..5d842d1bc751 100644 --- a/src/vm/threads.cpp +++ b/src/vm/threads.cpp @@ -2170,34 +2170,17 @@ BOOL Thread::AllocHandles() { WRAPPER_NO_CONTRACT; -#ifndef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension - _ASSERTE(!m_SafeEvent.IsValid()); - _ASSERTE(!m_UserSuspendEvent.IsValid()); -#endif // !FEATURE_CORECLR _ASSERTE(!m_DebugSuspendEvent.IsValid()); _ASSERTE(!m_EventWait.IsValid()); BOOL fOK = TRUE; EX_TRY { -#ifndef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension // create a manual reset event for getting the thread to a safe point - m_SafeEvent.CreateManualEvent(FALSE); - m_UserSuspendEvent.CreateManualEvent(FALSE); -#endif // !FEATURE_CORECLR m_DebugSuspendEvent.CreateManualEvent(FALSE); m_EventWait.CreateManualEvent(TRUE); } EX_CATCH { fOK = FALSE; -#ifndef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension - if (!m_SafeEvent.IsValid()) { - m_SafeEvent.CloseEvent(); - } - - if (!m_UserSuspendEvent.IsValid()) { - m_UserSuspendEvent.CloseEvent(); - } -#endif // !FEATURE_CORECLR if (!m_DebugSuspendEvent.IsValid()) { m_DebugSuspendEvent.CloseEvent(); @@ -2410,35 +2393,8 @@ BOOL Thread::HasStarted(BOOL bRequiresTSL) } #endif // PROFILING_SUPPORTED -#ifdef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension _ASSERTE(!(m_State & TS_SuspendUnstarted)); -#else // !FEATURE_CORECLR - // Is there a pending user suspension? - if (m_State & TS_SuspendUnstarted) - { - BOOL doSuspend = FALSE; - - { - ThreadStoreLockHolder TSLockHolder; - - // Perhaps we got resumed before it took effect? - if (m_State & TS_SuspendUnstarted) - { - FastInterlockAnd((ULONG *) &m_State, ~TS_SuspendUnstarted); - SetupForSuspension(TS_UserSuspendPending); - MarkForSuspension(TS_UserSuspendPending); - doSuspend = TRUE; - } - } - - if (doSuspend) - { - GCX_PREEMP(); - WaitSuspendEvents(); - } - } -#endif // FEATURE_CORECLR } return res; @@ -3075,16 +3031,6 @@ Thread::~Thread() CloseHandle(GetThreadHandle()); } -#ifndef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension - if (m_SafeEvent.IsValid()) - { - m_SafeEvent.CloseEvent(); - } - if (m_UserSuspendEvent.IsValid()) - { - m_UserSuspendEvent.CloseEvent(); - } -#endif // !FEATURE_CORECLR if (m_DebugSuspendEvent.IsValid()) { m_DebugSuspendEvent.CloseEvent(); @@ -3541,13 +3487,8 @@ void Thread::OnThreadTerminate(BOOL holdingLock) if (m_State & TS_DebugSuspendPending) UnmarkForSuspension(~TS_DebugSuspendPending); -#ifdef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension _ASSERTE(!(m_State & TS_UserSuspendPending)); -#else // !FEATURE_CORECLR - if (m_State & TS_UserSuspendPending) - UnmarkForSuspension(~TS_UserSuspendPending); -#endif // FEATURE_CORECLR if (CurrentThreadID == ThisThreadID && IsAbortRequested()) { @@ -5759,7 +5700,7 @@ ThreadStore::ThreadStore() m_PendingThreadCount(0), m_DeadThreadCount(0), m_DeadThreadCountForGCTrigger(0), - m_TriggerFinalizingGC(false), + m_TriggerGCForDeadThreads(false), m_GuidCreated(FALSE), m_HoldingThread(0) { @@ -6059,9 +6000,8 @@ void ThreadStore::IncrementDeadThreadCountForGCTrigger() } IGCHeap *gcHeap = GCHeapUtilities::GetGCHeap(); - SIZE_T millisecondsSinceLastFinalizingGC = - gcHeap->GetNow() - gcHeap->GetLastGCStartTime(gcHeap->GetMinFinalizingGeneration()); - if (millisecondsSinceLastFinalizingGC < s_DeadThreadGCTriggerPeriodMilliseconds) + SIZE_T millisecondsSinceLastGC = gcHeap->GetNow() - gcHeap->GetLastGCStartTime(1); + if (millisecondsSinceLastGC < s_DeadThreadGCTriggerPeriodMilliseconds) { return; } @@ -6072,11 +6012,11 @@ void ThreadStore::IncrementDeadThreadCountForGCTrigger() } // The dead thread count used for triggering a GC is tracked separately from the actual dead thread count so that a dead - // thread does not contribute to triggering a GC more than once. The count exceeds a certain threshold and a finalizing GC - // has not occurred in a certain duration, so reset the count and trigger a GC now to finalize unreachable dead threads. - // The GC is triggered on the finalizer thread since it's not safe to trigger it on DLL_THREAD_DETACH. + // thread does not contribute to triggering a GC more than once. The count exceeds a certain threshold and a GC has not + // occurred in a certain duration, so reset the count and trigger a GC now to finalize unreachable dead threads. The GC is + // triggered on the finalizer thread since it's not safe to trigger it on DLL_THREAD_DETACH. m_DeadThreadCountForGCTrigger = 0; - m_TriggerFinalizingGC = true; + m_TriggerGCForDeadThreads = true; FinalizerThread::EnableFinalization(); } @@ -6090,30 +6030,35 @@ void ThreadStore::DecrementDeadThreadCountForGCTrigger() } } -void ThreadStore::OnFinalizingGCStarted() +void ThreadStore::OnGCStarted(int generation) { - // A dead thread may contribute to triggering at most once. After a finalizing GC occurs, if some dead thread objects are - // still reachable due to references to the thread objects, they will not contribute to triggering a GC again. Synchronize - // the store with increment/decrement operations occurring on different threads, and make the change visible to other - // threads in order to prevent unnecessary GC triggers. + if (generation <= 0) + { + return; + } + + // A dead thread may contribute to triggering at most once. After a GC occurs, if some dead thread objects are still + // reachable due to references to the thread objects, they will not contribute to triggering a GC again. Synchronize the + // store with increment/decrement operations occurring on different threads, and make the change visible to other threads in + // order to prevent unnecessary GC triggers. FastInterlockExchange(&m_DeadThreadCountForGCTrigger, 0); } -bool ThreadStore::ShouldTriggerFinalizingGC() +bool ThreadStore::ShouldTriggerGCForDeadThreads() { - return m_TriggerFinalizingGC; + return m_TriggerGCForDeadThreads; } -void ThreadStore::TriggerFinalizingGCIfNecessary() +void ThreadStore::TriggerGCForDeadThreadsIfNecessary() { - if (!m_TriggerFinalizingGC) + if (!m_TriggerGCForDeadThreads) { return; } - m_TriggerFinalizingGC = false; + m_TriggerGCForDeadThreads = false; IGCHeap *gcHeap = GCHeapUtilities::GetGCHeap(); - gcHeap->GarbageCollect(gcHeap->GetMinFinalizingGeneration(), FALSE, collection_non_blocking); + gcHeap->GarbageCollect(1, FALSE, collection_non_blocking); } #endif // #ifndef DACCESS_COMPILE @@ -6333,13 +6278,8 @@ BOOL ThreadStore::DbgFindThread(Thread *target) if (cur->m_State & Thread::TS_DebugSuspendPending) cntReturn++; -#ifdef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension _ASSERTE(!(cur->m_State & Thread::TS_UserSuspendPending)); -#else // !FEATURE_CORECLR - if (cur->m_State & Thread::TS_UserSuspendPending) - cntReturn++; -#endif // FEATURE_CORECLR if (cur->m_TraceCallCount > 0) cntReturn++; @@ -8810,7 +8750,7 @@ BOOL Thread::HaveExtraWorkForFinalizer() || (m_DetachCount > 0) || AppDomain::HasWorkForFinalizerThread() || SystemDomain::System()->RequireAppDomainCleanup() - || ThreadStore::s_pThreadStore->ShouldTriggerFinalizingGC(); + || ThreadStore::s_pThreadStore->ShouldTriggerGCForDeadThreads(); } void Thread::DoExtraWorkForFinalizer() @@ -8868,7 +8808,7 @@ void Thread::DoExtraWorkForFinalizer() // If there were any TimerInfos waiting to be released, they'll get flushed now ThreadpoolMgr::FlushQueueOfTimerInfos(); - ThreadStore::s_pThreadStore->TriggerFinalizingGCIfNecessary(); + ThreadStore::s_pThreadStore->TriggerGCForDeadThreadsIfNecessary(); } diff --git a/src/vm/threads.h b/src/vm/threads.h index 020805b8e4ce..b558a758c363 100644 --- a/src/vm/threads.h +++ b/src/vm/threads.h @@ -3713,14 +3713,10 @@ class Thread: public IUnknown { WRAPPER_NO_CONTRACT; -#ifdef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension _ASSERTE(!(bit & TS_UserSuspendPending)); -#else // !FEATURE_CORECLR - if (bit & TS_UserSuspendPending) { - m_UserSuspendEvent.Reset(); - } -#endif // FEATURE_CORECLR + + if (bit & TS_DebugSuspendPending) { m_DebugSuspendEvent.Reset(); } @@ -3737,17 +3733,13 @@ class Thread: public IUnknown // ThreadState oldState = m_State; -#ifdef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension _ASSERTE(!(oldState & TS_UserSuspendPending)); -#endif // FEATURE_CORECLR while ((oldState & (TS_UserSuspendPending | TS_DebugSuspendPending)) == 0) { -#ifdef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension _ASSERTE(!(oldState & TS_UserSuspendPending)); -#endif // FEATURE_CORECLR // // Construct the destination state we desire - all suspension bits turned off. @@ -3767,14 +3759,8 @@ class Thread: public IUnknown oldState = m_State; } -#ifdef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension _ASSERTE(!(bit & TS_UserSuspendPending)); -#else // !FEATURE_CORECLR - if (bit & TS_UserSuspendPending) { - m_UserSuspendEvent.Set(); - } -#endif // FEATURE_CORECLR if (bit & TS_DebugSuspendPending) { m_DebugSuspendEvent.Set(); @@ -3782,12 +3768,6 @@ class Thread: public IUnknown } -#ifndef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension - // For getting a thread to a safe point. A client waits on the event, which is - // set by the thread when it reaches a safe spot. - void SetSafeEvent(); -#endif // !FEATURE_CORECLR - public: FORCEINLINE void UnhijackThreadNoAlloc() { @@ -3908,10 +3888,6 @@ class Thread: public IUnknown private: // For suspends: -#ifndef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension - CLREvent m_SafeEvent; - CLREvent m_UserSuspendEvent; -#endif // !FEATURE_CORECLR CLREvent m_DebugSuspendEvent; // For Object::Wait, Notify and NotifyAll, we use an Event inside the @@ -5542,7 +5518,7 @@ class ThreadStore LONG m_DeadThreadCount; LONG m_DeadThreadCountForGCTrigger; - bool m_TriggerFinalizingGC; + bool m_TriggerGCForDeadThreads; private: // Space for the lazily-created GUID. @@ -5637,9 +5613,9 @@ class ThreadStore void IncrementDeadThreadCountForGCTrigger(); void DecrementDeadThreadCountForGCTrigger(); public: - void OnFinalizingGCStarted(); - bool ShouldTriggerFinalizingGC(); - void TriggerFinalizingGCIfNecessary(); + void OnGCStarted(int generation); + bool ShouldTriggerGCForDeadThreads(); + void TriggerGCForDeadThreadsIfNecessary(); }; struct TSSuspendHelper { diff --git a/src/vm/threadsuspend.cpp b/src/vm/threadsuspend.cpp index cf7d15a91404..71409ceb5a71 100644 --- a/src/vm/threadsuspend.cpp +++ b/src/vm/threadsuspend.cpp @@ -1978,10 +1978,8 @@ Thread::UserAbort(ThreadAbortRequester requester, m_dwAbortPoint = 7; #endif -#ifdef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension _ASSERTE(!(m_State & TS_UserSuspendPending)); -#endif // FEATURE_CORECLR // // If it's stopped by the debugger, we don't want to throw an exception. @@ -3095,10 +3093,8 @@ void Thread::RareDisablePreemptiveGC() goto Exit; } -#ifdef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension _ASSERTE(!(m_State & TS_UserSuspendPending)); -#endif // !FEATURE_CORECLR // Note IsGCInProgress is also true for say Pause (anywhere SuspendEE happens) and GCThread is the // thread that did the Pause. While in Pause if another thread attempts Rev/Pinvoke it should get inside the following and @@ -3115,10 +3111,8 @@ void Thread::RareDisablePreemptiveGC() do { -#ifdef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension _ASSERTE(!(m_State & TS_UserSuspendPending)); -#endif // !FEATURE_CORECLR EnablePreemptiveGC(); @@ -3573,9 +3567,6 @@ void Thread::RareEnablePreemptiveGC() #endif // FEATURE_HIJACK // wake up any threads waiting to suspend us, like the GC thread. -#ifndef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension - SetSafeEvent(); -#endif // !FEATURE_CORECLR ThreadSuspend::g_pGCSuspendEvent->Set(); // for GC, the fact that we are leaving the EE means that it no longer needs to @@ -3583,10 +3574,8 @@ void Thread::RareEnablePreemptiveGC() // Give the debugger precedence over user suspensions: while (m_State & (TS_DebugSuspendPending | TS_UserSuspendPending)) { -#ifdef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension _ASSERTE(!(m_State & TS_UserSuspendPending)); -#endif // !FEATURE_CORECLR #ifdef DEBUGGING_SUPPORTED // We don't notify the debugger that this thread is now suspended. We'll just @@ -3925,15 +3914,6 @@ void __stdcall Thread::RedirectedHandledJITCase(RedirectReason reason) // Enable PGC before calling out to the client to allow runtime suspend to finish GCX_PREEMP_NO_DTOR(); -#ifndef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension - // @TODO: Is this necessary? Does debugger wait on the events, or does it just - // poll every so often? - // Notify the thread that is performing the suspension that this thread - // is now in PGC mode and that it can remove this thread from the list of - // threads it needs to wait for. - pThread->SetSafeEvent(); -#endif // !FEATURE_CORECLR - // Notify the interface of the pending suspension switch (reason) { case RedirectReason_GCSuspension: @@ -6040,19 +6020,6 @@ void Thread::SysResumeFromDebug(AppDomain *pAppDomain) LOG((LF_CORDB, LL_INFO1000, "RESUME: resume complete. Trap count: %d\n", g_TrapReturningThreads.Load())); } -#ifndef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension -void Thread::SetSafeEvent() -{ - CONTRACTL { - NOTHROW; - GC_NOTRIGGER; - } - CONTRACTL_END; - - m_SafeEvent.Set(); -} -#endif // !FEATURE_CORECLR - /* * * WaitSuspendEventsHelper @@ -6079,32 +6046,9 @@ BOOL Thread::WaitSuspendEventsHelper(void) EX_TRY { -#ifdef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension _ASSERTE(!(m_State & TS_UserSuspendPending)); -#else // !FEATURE_CORECLR - if (m_State & TS_UserSuspendPending) { - - ThreadState oldState = m_State; - - while (oldState & TS_UserSuspendPending) { - ThreadState newState = (ThreadState)(oldState | TS_SyncSuspended); - if (FastInterlockCompareExchange((LONG *)&m_State, newState, oldState) == (LONG)oldState) - { - result = m_UserSuspendEvent.Wait(INFINITE,FALSE); -#if _DEBUG - newState = m_State; - _ASSERTE(!(newState & TS_SyncSuspended) || (newState & TS_DebugSuspendPending)); -#endif - break; - } - - oldState = m_State; - } - - } else -#endif // FEATURE_CORECLR if (m_State & TS_DebugSuspendPending) { ThreadState oldState = m_State; @@ -6155,10 +6099,8 @@ void Thread::WaitSuspendEvents(BOOL fDoWait) ThreadState oldState = m_State; -#ifdef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension _ASSERTE(!(oldState & TS_UserSuspendPending)); -#endif // !FEATURE_CORECLR // // If all reasons to suspend are off, we think we can exit @@ -7139,15 +7081,9 @@ void Thread::MarkForSuspension(ULONG bit) } CONTRACTL_END; -#ifdef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension _ASSERTE(bit == TS_DebugSuspendPending || bit == (TS_DebugSuspendPending | TS_DebugWillSync)); -#else // !FEATURE_CORECLR - _ASSERTE(bit == TS_DebugSuspendPending || - bit == (TS_DebugSuspendPending | TS_DebugWillSync) || - bit == TS_UserSuspendPending); -#endif // FEATURE_CORECLR _ASSERTE(IsAtProcessExit() || ThreadStore::HoldingThreadStore()); @@ -7165,13 +7101,8 @@ void Thread::UnmarkForSuspension(ULONG mask) } CONTRACTL_END; -#ifdef FEATURE_CORECLR // CoreCLR does not support user-requested thread suspension _ASSERTE(mask == ~TS_DebugSuspendPending); -#else // !FEATURE_CORECLR - _ASSERTE(mask == ~TS_DebugSuspendPending || - mask == ~TS_UserSuspendPending); -#endif // FEATURE_CORECLR _ASSERTE(IsAtProcessExit() || ThreadStore::HoldingThreadStore()); From 81690dd27d86ebc0d48652c222ccaaaf9a047fa8 Mon Sep 17 00:00:00 2001 From: Koundinya Veluri Date: Fri, 24 Mar 2017 04:41:43 -0700 Subject: [PATCH 3/5] Trigger an appropriate generation of GC --- src/vm/gcenv.ee.cpp | 5 +- src/vm/threads.cpp | 149 ++++++++++++++++++++++++++++++++++++-------- src/vm/threads.h | 25 +++++++- 3 files changed, 151 insertions(+), 28 deletions(-) diff --git a/src/vm/gcenv.ee.cpp b/src/vm/gcenv.ee.cpp index 6ca8835246eb..2ce79bad3c28 100644 --- a/src/vm/gcenv.ee.cpp +++ b/src/vm/gcenv.ee.cpp @@ -627,7 +627,10 @@ void GCToEEInterface::GcStartWork (int condemned, int max_gen) RCWWalker::OnGCStarted(condemned); #endif // FEATURE_COMINTEROP - ThreadStore::s_pThreadStore->OnGCStarted(condemned); + if (condemned == max_gen) + { + ThreadStore::s_pThreadStore->OnMaxGenerationGCStarted(); + } } void GCToEEInterface::GcDone(int condemned) diff --git a/src/vm/threads.cpp b/src/vm/threads.cpp index 5d842d1bc751..5d637f9bd82c 100644 --- a/src/vm/threads.cpp +++ b/src/vm/threads.cpp @@ -1825,6 +1825,7 @@ Thread::Thread() #ifdef FEATURE_COMINTEROP m_fDisableComObjectEagerCleanup = false; #endif //FEATURE_COMINTEROP + m_fHasDeadThreadBeenConsideredForGCTrigger = false; m_Context = NULL; m_TraceCallCount = 0; m_ThrewControlForThread = 0; @@ -5700,7 +5701,7 @@ ThreadStore::ThreadStore() m_PendingThreadCount(0), m_DeadThreadCount(0), m_DeadThreadCountForGCTrigger(0), - m_TriggerGCForDeadThreads(false), + m_TriggerGCGenerationForDeadThreads(-1), m_GuidCreated(FALSE), m_HoldingThread(0) { @@ -5988,9 +5989,15 @@ DWORD ThreadStore::s_DeadThreadGCTriggerPeriodMilliseconds = 0; void ThreadStore::IncrementDeadThreadCountForGCTrigger() { + CONTRACTL { + NOTHROW; + GC_NOTRIGGER; + } + CONTRACTL_END; + // Although all increments and decrements are usually done inside a lock, that is not sufficient to synchronize with a // background GC thread resetting this value, hence the interlocked operation. Ignore overflow; overflow would likely never - // happen, the count is treated as unsigned, and nothing bad would happen if it were to overflow. + // occur, the count is treated as unsigned, and nothing bad would happen if it were to overflow. SIZE_T count = static_cast(FastInterlockIncrement(&m_DeadThreadCountForGCTrigger)); SIZE_T countThreshold = static_cast(s_DeadThreadCountThresholdForGCTrigger); @@ -6000,65 +6007,157 @@ void ThreadStore::IncrementDeadThreadCountForGCTrigger() } IGCHeap *gcHeap = GCHeapUtilities::GetGCHeap(); - SIZE_T millisecondsSinceLastGC = gcHeap->GetNow() - gcHeap->GetLastGCStartTime(1); - if (millisecondsSinceLastGC < s_DeadThreadGCTriggerPeriodMilliseconds) + if (gcHeap == nullptr) { return; } - if (!g_fEEStarted) + int gcMaxGeneration = static_cast(gcHeap->GetMaxGeneration()); + SIZE_T gcLastMilliseconds = gcHeap->GetLastGCStartTime(gcMaxGeneration); + SIZE_T gcNowMilliseconds = gcHeap->GetNow(); + if (gcNowMilliseconds - gcLastMilliseconds < s_DeadThreadGCTriggerPeriodMilliseconds) { return; } - // The dead thread count used for triggering a GC is tracked separately from the actual dead thread count so that a dead - // thread does not contribute to triggering a GC more than once. The count exceeds a certain threshold and a GC has not - // occurred in a certain duration, so reset the count and trigger a GC now to finalize unreachable dead threads. The GC is - // triggered on the finalizer thread since it's not safe to trigger it on DLL_THREAD_DETACH. + if (!g_fEEStarted) // required for FinalizerThread::EnableFinalization() below + { + return; + } + + int gcGenerationToTrigger = 0; + SIZE_T newDeadThreadGenerationCounts[3] = {0}; + int countedMaxGeneration = _countof(newDeadThreadGenerationCounts) - 1; + { + GCX_COOP(); + + // Determine the generation for which to trigger a GC. Iterate over all dead threads that have not yet been considered + // for triggering a GC and see how many are in which generations. + for (Thread *thread = ThreadStore::GetAllThreadList(NULL, Thread::TS_Dead, Thread::TS_Dead); + thread != nullptr; + thread = ThreadStore::GetAllThreadList(thread, Thread::TS_Dead, Thread::TS_Dead)) + { + if (thread->HasDeadThreadBeenConsideredForGCTrigger()) + { + continue; + } + + Object *exposedObject = OBJECTREFToObject(thread->GetExposedObjectRaw()); + if (exposedObject == nullptr) + { + continue; + } + + int exposedObjectGeneration = min(countedMaxGeneration, static_cast(gcHeap->WhichGeneration(exposedObject))); + SIZE_T newDeadThreadGenerationCount = ++newDeadThreadGenerationCounts[exposedObjectGeneration]; + if (exposedObjectGeneration > gcGenerationToTrigger && newDeadThreadGenerationCount >= countThreshold / 2) + { + gcGenerationToTrigger = exposedObjectGeneration; + if (gcGenerationToTrigger >= countedMaxGeneration) + { + break; + } + } + } + + // Make sure that enough time has elapsed since the last GC of the desired generation. Otherwise, the memory pressure + // would be sufficient to trigger GCs automatically. + gcLastMilliseconds = gcHeap->GetLastGCStartTime(gcGenerationToTrigger); + gcNowMilliseconds = gcHeap->GetNow(); + if (gcNowMilliseconds - gcLastMilliseconds < s_DeadThreadGCTriggerPeriodMilliseconds) + { + return; + } + + // For threads whose exposed objects are in the generation of GC that will be triggered or in a lower GC generation, + // mark them as having contributed to a GC trigger to prevent redundant GC triggers + for (Thread *thread = ThreadStore::GetAllThreadList(NULL, Thread::TS_Dead, Thread::TS_Dead); + thread != nullptr; + thread = ThreadStore::GetAllThreadList(thread, Thread::TS_Dead, Thread::TS_Dead)) + { + if (thread->HasDeadThreadBeenConsideredForGCTrigger()) + { + continue; + } + + Object *exposedObject = OBJECTREFToObject(thread->GetExposedObjectRaw()); + if (exposedObject == nullptr) + { + continue; + } + + if (gcGenerationToTrigger < countedMaxGeneration && + static_cast(gcHeap->WhichGeneration(exposedObject)) > gcGenerationToTrigger) + { + continue; + } + + thread->SetHasDeadThreadBeenConsideredForGCTrigger(); + } + } // GCX_COOP() + + if (gcGenerationToTrigger >= countedMaxGeneration) + { + gcGenerationToTrigger = gcMaxGeneration; + } + + // The GC is triggered on the finalizer thread since it's not safe to trigger it on DLL_THREAD_DETACH. Since there will be a + // delay before the dead thread count is updated, just clear the count and wait for it to reach the threshold again. m_DeadThreadCountForGCTrigger = 0; - m_TriggerGCForDeadThreads = true; + m_TriggerGCGenerationForDeadThreads = gcGenerationToTrigger; FinalizerThread::EnableFinalization(); } void ThreadStore::DecrementDeadThreadCountForGCTrigger() { + CONTRACTL { + NOTHROW; + GC_NOTRIGGER; + } + CONTRACTL_END; + // Although all increments and decrements are usually done inside a lock, that is not sufficient to synchronize with a // background GC thread resetting this value, hence the interlocked operation. if (FastInterlockDecrement(&m_DeadThreadCountForGCTrigger) < 0) { - FastInterlockExchange(&m_DeadThreadCountForGCTrigger, 0); + m_DeadThreadCountForGCTrigger = 0; } } -void ThreadStore::OnGCStarted(int generation) +void ThreadStore::OnMaxGenerationGCStarted() { - if (generation <= 0) - { - return; - } + LIMITED_METHOD_CONTRACT; - // A dead thread may contribute to triggering at most once. After a GC occurs, if some dead thread objects are still - // reachable due to references to the thread objects, they will not contribute to triggering a GC again. Synchronize the - // store with increment/decrement operations occurring on different threads, and make the change visible to other threads in - // order to prevent unnecessary GC triggers. + // A dead thread may contribute to triggering a GC at most once. After a max-generation GC occurs, if some dead thread + // objects are still reachable due to references to the thread objects, they will not contribute to triggering a GC again. + // Synchronize the store with increment/decrement operations occurring on different threads, and make the change visible to + // other threads in order to prevent unnecessary GC triggers. FastInterlockExchange(&m_DeadThreadCountForGCTrigger, 0); } bool ThreadStore::ShouldTriggerGCForDeadThreads() { - return m_TriggerGCForDeadThreads; + LIMITED_METHOD_CONTRACT; + + return m_TriggerGCGenerationForDeadThreads >= 0; } void ThreadStore::TriggerGCForDeadThreadsIfNecessary() { - if (!m_TriggerGCForDeadThreads) + CONTRACTL { + NOTHROW; + GC_TRIGGERS; + } + CONTRACTL_END; + + int gcGenerationToTrigger = m_TriggerGCGenerationForDeadThreads; + if (gcGenerationToTrigger < 0) { return; } - m_TriggerGCForDeadThreads = false; + m_TriggerGCGenerationForDeadThreads = -1; - IGCHeap *gcHeap = GCHeapUtilities::GetGCHeap(); - gcHeap->GarbageCollect(1, FALSE, collection_non_blocking); + GCHeapUtilities::GetGCHeap()->GarbageCollect(gcGenerationToTrigger, FALSE, collection_non_blocking); } #endif // #ifndef DACCESS_COMPILE diff --git a/src/vm/threads.h b/src/vm/threads.h index b558a758c363..bb63ed4d5c25 100644 --- a/src/vm/threads.h +++ b/src/vm/threads.h @@ -1450,6 +1450,24 @@ class Thread: public IUnknown } #endif //FEATURE_COMINTEROP +#ifndef DACCESS_COMPILE + bool HasDeadThreadBeenConsideredForGCTrigger() + { + LIMITED_METHOD_CONTRACT; + _ASSERTE(IsDead()); + + return m_fHasDeadThreadBeenConsideredForGCTrigger; + } + + void SetHasDeadThreadBeenConsideredForGCTrigger() + { + LIMITED_METHOD_CONTRACT; + _ASSERTE(IsDead()); + + m_fHasDeadThreadBeenConsideredForGCTrigger = true; + } +#endif // !DACCESS_COMPILE + // returns if there is some extra work for the finalizer thread. BOOL HaveExtraWorkForFinalizer(); @@ -5231,6 +5249,9 @@ class Thread: public IUnknown // Disables pumping and thread join in RCW creation bool m_fDisableComObjectEagerCleanup; + // See ThreadStore::TriggerGCForDeadThreadsIfNecessary() + bool m_fHasDeadThreadBeenConsideredForGCTrigger; + private: CLRRandom m_random; @@ -5518,7 +5539,7 @@ class ThreadStore LONG m_DeadThreadCount; LONG m_DeadThreadCountForGCTrigger; - bool m_TriggerGCForDeadThreads; + int m_TriggerGCGenerationForDeadThreads; private: // Space for the lazily-created GUID. @@ -5613,7 +5634,7 @@ class ThreadStore void IncrementDeadThreadCountForGCTrigger(); void DecrementDeadThreadCountForGCTrigger(); public: - void OnGCStarted(int generation); + void OnMaxGenerationGCStarted(); bool ShouldTriggerGCForDeadThreads(); void TriggerGCForDeadThreadsIfNecessary(); }; From dfcb32e1b0b518598d3e06cadb82470e85030df4 Mon Sep 17 00:00:00 2001 From: Koundinya Veluri Date: Fri, 24 Mar 2017 12:11:11 -0700 Subject: [PATCH 4/5] Move determination of GC generation to trigger to finalizer thread --- src/vm/threads.cpp | 139 ++++++++++++++++++++++++--------------------- src/vm/threads.h | 2 +- 2 files changed, 76 insertions(+), 65 deletions(-) diff --git a/src/vm/threads.cpp b/src/vm/threads.cpp index 5d637f9bd82c..29e603cc8322 100644 --- a/src/vm/threads.cpp +++ b/src/vm/threads.cpp @@ -5701,7 +5701,7 @@ ThreadStore::ThreadStore() m_PendingThreadCount(0), m_DeadThreadCount(0), m_DeadThreadCountForGCTrigger(0), - m_TriggerGCGenerationForDeadThreads(-1), + m_TriggerGCForDeadThreads(false), m_GuidCreated(FALSE), m_HoldingThread(0) { @@ -6012,8 +6012,7 @@ void ThreadStore::IncrementDeadThreadCountForGCTrigger() return; } - int gcMaxGeneration = static_cast(gcHeap->GetMaxGeneration()); - SIZE_T gcLastMilliseconds = gcHeap->GetLastGCStartTime(gcMaxGeneration); + SIZE_T gcLastMilliseconds = gcHeap->GetLastGCStartTime(gcHeap->GetMaxGeneration()); SIZE_T gcNowMilliseconds = gcHeap->GetNow(); if (gcNowMilliseconds - gcLastMilliseconds < s_DeadThreadGCTriggerPeriodMilliseconds) { @@ -6025,10 +6024,78 @@ void ThreadStore::IncrementDeadThreadCountForGCTrigger() return; } + // The GC is triggered on the finalizer thread since it's not safe to trigger it on DLL_THREAD_DETACH. + // TriggerGCForDeadThreadsIfNecessary() will determine which generation of GC to trigger, and may not actually trigger a GC. + // If a GC is triggered, since there would be a delay before the dead thread count is updated, clear the count and wait for + // it to reach the threshold again. If a GC would not be triggered, the count is still cleared here to prevent waking up the + // finalizer thread to do the work in TriggerGCForDeadThreadsIfNecessary() for every dead thread. + m_DeadThreadCountForGCTrigger = 0; + m_TriggerGCForDeadThreads = true; + FinalizerThread::EnableFinalization(); +} + +void ThreadStore::DecrementDeadThreadCountForGCTrigger() +{ + CONTRACTL { + NOTHROW; + GC_NOTRIGGER; + } + CONTRACTL_END; + + // Although all increments and decrements are usually done inside a lock, that is not sufficient to synchronize with a + // background GC thread resetting this value, hence the interlocked operation. + if (FastInterlockDecrement(&m_DeadThreadCountForGCTrigger) < 0) + { + m_DeadThreadCountForGCTrigger = 0; + } +} + +void ThreadStore::OnMaxGenerationGCStarted() +{ + LIMITED_METHOD_CONTRACT; + + // A dead thread may contribute to triggering a GC at most once. After a max-generation GC occurs, if some dead thread + // objects are still reachable due to references to the thread objects, they will not contribute to triggering a GC again. + // Synchronize the store with increment/decrement operations occurring on different threads, and make the change visible to + // other threads in order to prevent unnecessary GC triggers. + FastInterlockExchange(&m_DeadThreadCountForGCTrigger, 0); +} + +bool ThreadStore::ShouldTriggerGCForDeadThreads() +{ + LIMITED_METHOD_CONTRACT; + + return m_TriggerGCForDeadThreads; +} + +void ThreadStore::TriggerGCForDeadThreadsIfNecessary() +{ + CONTRACTL { + NOTHROW; + GC_TRIGGERS; + } + CONTRACTL_END; + + if (!m_TriggerGCForDeadThreads) + { + return; + } + m_TriggerGCForDeadThreads = false; + + if (g_fEEShutDown) + { + // Not safe to touch CLR state + return; + } + int gcGenerationToTrigger = 0; + IGCHeap *gcHeap = GCHeapUtilities::GetGCHeap(); + _ASSERTE(gcHeap != nullptr); + SIZE_T generationCountThreshold = static_cast(s_DeadThreadCountThresholdForGCTrigger) / 2; SIZE_T newDeadThreadGenerationCounts[3] = {0}; int countedMaxGeneration = _countof(newDeadThreadGenerationCounts) - 1; { + ThreadStoreLockHolder threadStoreLockHolder; GCX_COOP(); // Determine the generation for which to trigger a GC. Iterate over all dead threads that have not yet been considered @@ -6050,7 +6117,7 @@ void ThreadStore::IncrementDeadThreadCountForGCTrigger() int exposedObjectGeneration = min(countedMaxGeneration, static_cast(gcHeap->WhichGeneration(exposedObject))); SIZE_T newDeadThreadGenerationCount = ++newDeadThreadGenerationCounts[exposedObjectGeneration]; - if (exposedObjectGeneration > gcGenerationToTrigger && newDeadThreadGenerationCount >= countThreshold / 2) + if (exposedObjectGeneration > gcGenerationToTrigger && newDeadThreadGenerationCount >= generationCountThreshold) { gcGenerationToTrigger = exposedObjectGeneration; if (gcGenerationToTrigger >= countedMaxGeneration) @@ -6062,8 +6129,8 @@ void ThreadStore::IncrementDeadThreadCountForGCTrigger() // Make sure that enough time has elapsed since the last GC of the desired generation. Otherwise, the memory pressure // would be sufficient to trigger GCs automatically. - gcLastMilliseconds = gcHeap->GetLastGCStartTime(gcGenerationToTrigger); - gcNowMilliseconds = gcHeap->GetNow(); + SIZE_T gcLastMilliseconds = gcHeap->GetLastGCStartTime(gcGenerationToTrigger); + SIZE_T gcNowMilliseconds = gcHeap->GetNow(); if (gcNowMilliseconds - gcLastMilliseconds < s_DeadThreadGCTriggerPeriodMilliseconds) { return; @@ -6094,68 +6161,12 @@ void ThreadStore::IncrementDeadThreadCountForGCTrigger() thread->SetHasDeadThreadBeenConsideredForGCTrigger(); } - } // GCX_COOP() + } // ThreadStoreLockHolder, GCX_COOP() if (gcGenerationToTrigger >= countedMaxGeneration) { - gcGenerationToTrigger = gcMaxGeneration; - } - - // The GC is triggered on the finalizer thread since it's not safe to trigger it on DLL_THREAD_DETACH. Since there will be a - // delay before the dead thread count is updated, just clear the count and wait for it to reach the threshold again. - m_DeadThreadCountForGCTrigger = 0; - m_TriggerGCGenerationForDeadThreads = gcGenerationToTrigger; - FinalizerThread::EnableFinalization(); -} - -void ThreadStore::DecrementDeadThreadCountForGCTrigger() -{ - CONTRACTL { - NOTHROW; - GC_NOTRIGGER; - } - CONTRACTL_END; - - // Although all increments and decrements are usually done inside a lock, that is not sufficient to synchronize with a - // background GC thread resetting this value, hence the interlocked operation. - if (FastInterlockDecrement(&m_DeadThreadCountForGCTrigger) < 0) - { - m_DeadThreadCountForGCTrigger = 0; - } -} - -void ThreadStore::OnMaxGenerationGCStarted() -{ - LIMITED_METHOD_CONTRACT; - - // A dead thread may contribute to triggering a GC at most once. After a max-generation GC occurs, if some dead thread - // objects are still reachable due to references to the thread objects, they will not contribute to triggering a GC again. - // Synchronize the store with increment/decrement operations occurring on different threads, and make the change visible to - // other threads in order to prevent unnecessary GC triggers. - FastInterlockExchange(&m_DeadThreadCountForGCTrigger, 0); -} - -bool ThreadStore::ShouldTriggerGCForDeadThreads() -{ - LIMITED_METHOD_CONTRACT; - - return m_TriggerGCGenerationForDeadThreads >= 0; -} - -void ThreadStore::TriggerGCForDeadThreadsIfNecessary() -{ - CONTRACTL { - NOTHROW; - GC_TRIGGERS; - } - CONTRACTL_END; - - int gcGenerationToTrigger = m_TriggerGCGenerationForDeadThreads; - if (gcGenerationToTrigger < 0) - { - return; + gcGenerationToTrigger = gcHeap->GetMaxGeneration(); } - m_TriggerGCGenerationForDeadThreads = -1; GCHeapUtilities::GetGCHeap()->GarbageCollect(gcGenerationToTrigger, FALSE, collection_non_blocking); } diff --git a/src/vm/threads.h b/src/vm/threads.h index bb63ed4d5c25..f34066feb356 100644 --- a/src/vm/threads.h +++ b/src/vm/threads.h @@ -5539,7 +5539,7 @@ class ThreadStore LONG m_DeadThreadCount; LONG m_DeadThreadCountForGCTrigger; - int m_TriggerGCGenerationForDeadThreads; + bool m_TriggerGCForDeadThreads; private: // Space for the lazily-created GUID. From a2d50582ca526e61b042e2438cf60d262f0e9415 Mon Sep 17 00:00:00 2001 From: Koundinya Veluri Date: Fri, 24 Mar 2017 17:23:02 -0700 Subject: [PATCH 5/5] Address feedback and add sanity test --- src/vm/threads.cpp | 21 ++++----- .../threading/DeadThreads/DeadThreads.cs | 43 +++++++++++++++++++ .../threading/DeadThreads/DeadThreads.csproj | 32 ++++++++++++++ 3 files changed, 83 insertions(+), 13 deletions(-) create mode 100644 tests/src/baseservices/threading/DeadThreads/DeadThreads.cs create mode 100644 tests/src/baseservices/threading/DeadThreads/DeadThreads.csproj diff --git a/src/vm/threads.cpp b/src/vm/threads.cpp index 29e603cc8322..6d9d10b76181 100644 --- a/src/vm/threads.cpp +++ b/src/vm/threads.cpp @@ -6012,7 +6012,7 @@ void ThreadStore::IncrementDeadThreadCountForGCTrigger() return; } - SIZE_T gcLastMilliseconds = gcHeap->GetLastGCStartTime(gcHeap->GetMaxGeneration()); + SIZE_T gcLastMilliseconds = gcHeap->GetLastGCStartTime(max_generation); SIZE_T gcNowMilliseconds = gcHeap->GetNow(); if (gcNowMilliseconds - gcLastMilliseconds < s_DeadThreadGCTriggerPeriodMilliseconds) { @@ -6092,8 +6092,7 @@ void ThreadStore::TriggerGCForDeadThreadsIfNecessary() IGCHeap *gcHeap = GCHeapUtilities::GetGCHeap(); _ASSERTE(gcHeap != nullptr); SIZE_T generationCountThreshold = static_cast(s_DeadThreadCountThresholdForGCTrigger) / 2; - SIZE_T newDeadThreadGenerationCounts[3] = {0}; - int countedMaxGeneration = _countof(newDeadThreadGenerationCounts) - 1; + SIZE_T newDeadThreadGenerationCounts[max_generation + 1] = {0}; { ThreadStoreLockHolder threadStoreLockHolder; GCX_COOP(); @@ -6115,20 +6114,21 @@ void ThreadStore::TriggerGCForDeadThreadsIfNecessary() continue; } - int exposedObjectGeneration = min(countedMaxGeneration, static_cast(gcHeap->WhichGeneration(exposedObject))); + int exposedObjectGeneration = gcHeap->WhichGeneration(exposedObject); SIZE_T newDeadThreadGenerationCount = ++newDeadThreadGenerationCounts[exposedObjectGeneration]; if (exposedObjectGeneration > gcGenerationToTrigger && newDeadThreadGenerationCount >= generationCountThreshold) { gcGenerationToTrigger = exposedObjectGeneration; - if (gcGenerationToTrigger >= countedMaxGeneration) + if (gcGenerationToTrigger >= max_generation) { break; } } } - // Make sure that enough time has elapsed since the last GC of the desired generation. Otherwise, the memory pressure - // would be sufficient to trigger GCs automatically. + // Make sure that enough time has elapsed since the last GC of the desired generation. We don't want to trigger GCs + // based on this heuristic too often. Give it some time to let the memory pressure trigger GCs automatically, and only + // if it doesn't in the given time, this heuristic may kick in to trigger a GC. SIZE_T gcLastMilliseconds = gcHeap->GetLastGCStartTime(gcGenerationToTrigger); SIZE_T gcNowMilliseconds = gcHeap->GetNow(); if (gcNowMilliseconds - gcLastMilliseconds < s_DeadThreadGCTriggerPeriodMilliseconds) @@ -6153,7 +6153,7 @@ void ThreadStore::TriggerGCForDeadThreadsIfNecessary() continue; } - if (gcGenerationToTrigger < countedMaxGeneration && + if (gcGenerationToTrigger < max_generation && static_cast(gcHeap->WhichGeneration(exposedObject)) > gcGenerationToTrigger) { continue; @@ -6163,11 +6163,6 @@ void ThreadStore::TriggerGCForDeadThreadsIfNecessary() } } // ThreadStoreLockHolder, GCX_COOP() - if (gcGenerationToTrigger >= countedMaxGeneration) - { - gcGenerationToTrigger = gcHeap->GetMaxGeneration(); - } - GCHeapUtilities::GetGCHeap()->GarbageCollect(gcGenerationToTrigger, FALSE, collection_non_blocking); } diff --git a/tests/src/baseservices/threading/DeadThreads/DeadThreads.cs b/tests/src/baseservices/threading/DeadThreads/DeadThreads.cs new file mode 100644 index 000000000000..cc9f054dea63 --- /dev/null +++ b/tests/src/baseservices/threading/DeadThreads/DeadThreads.cs @@ -0,0 +1,43 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Runtime.CompilerServices; +using System.Threading; + +public class DeadThreads +{ + /// + /// A sanity test that exercises code paths relevant to the heuristic that triggers GCs based on dead thread count and time + /// elapsed since a previous GC. See https://github.com/dotnet/coreclr/pull/10413. + /// + /// This test suite runs with the following environment variables relevant to this test (see .csproj): + /// set COMPlus_Thread_DeadThreadCountThresholdForGCTrigger=8 + /// set COMPlus_Thread_DeadThreadGCTriggerPeriodMilliseconds=3e8 // 1000 + /// + private static void GCTriggerSanityTest() + { + var testDuration = TimeSpan.FromSeconds(8); + var startTime = DateTime.UtcNow; + do + { + StartNoOpThread(); + Thread.Sleep(1); + } while (DateTime.UtcNow - startTime < testDuration); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static void StartNoOpThread() + { + var t = new Thread(() => { }); + t.IsBackground = true; + t.Start(); + } + + public static int Main() + { + GCTriggerSanityTest(); + return 100; + } +} diff --git a/tests/src/baseservices/threading/DeadThreads/DeadThreads.csproj b/tests/src/baseservices/threading/DeadThreads/DeadThreads.csproj new file mode 100644 index 000000000000..b9c27581a927 --- /dev/null +++ b/tests/src/baseservices/threading/DeadThreads/DeadThreads.csproj @@ -0,0 +1,32 @@ + + + + + Debug + AnyCPU + DeadThreads + {649A239B-AB4B-4E20-BDCA-3BA736E8B790} + Exe + + + + + + + + + + + + + + +