From 2a033d4d51ff634c525a9dee36fc607e53340759 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Wed, 19 Jun 2024 15:50:46 -0700 Subject: [PATCH 01/11] do not do just gen2 when GCSTRESS_INSTR_JIT is set --- src/coreclr/gc/gc.cpp | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/src/coreclr/gc/gc.cpp b/src/coreclr/gc/gc.cpp index 69cb6c2c668cb..6032ac3579e05 100644 --- a/src/coreclr/gc/gc.cpp +++ b/src/coreclr/gc/gc.cpp @@ -49310,7 +49310,29 @@ bool GCHeap::StressHeap(gc_alloc_context * context) } Interlocked::Decrement(&OneAtATime); #endif // !MULTIPLE_HEAPS - if (IsConcurrentGCEnabled()) + + if (g_pConfig->GetGCStressLevel() & EEConfig::GCSTRESS_INSTR_JIT) + { + // When GCSTRESS_INSTR_JIT is set we see lots of GCs - on every GC-eligible instruction. + // We do not want all these GC to be gen2 because: + // - doing only or mostly gen2 is very expensive in this mode + // - doing only or mostly gen2 prevents coverage of generation-aware behaviors + // - the main value of this stress mode is to catch stack scanning issues at various/rare locations + // in the code and gen2 is not needed for that. + + int rgen = StressRNG(100); + + // gen0:gen1:gen2 distribution: 80:15:5 + if (rgen >= 95) + rgen = 2; + else if (rgen >= 80) + rgen = 1; + else + rgen = 0; + + GarbageCollectTry (rgen, FALSE, collection_gcstress); + } + else if (IsConcurrentGCEnabled()) { int rgen = StressRNG(10); @@ -49319,7 +49341,7 @@ bool GCHeap::StressHeap(gc_alloc_context * context) rgen = 2; else if (rgen >= 4) rgen = 1; - else + else rgen = 0; GarbageCollectTry (rgen, FALSE, collection_gcstress); From 9e60f364ae5258b86f490fd023180af5e45bd89e Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Wed, 19 Jun 2024 16:54:14 -0700 Subject: [PATCH 02/11] cleanups and disable GC stress while WFPF is in progress --- src/coreclr/vm/finalizerthread.cpp | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/coreclr/vm/finalizerthread.cpp b/src/coreclr/vm/finalizerthread.cpp index b3d16dbf37dc3..f898bed3ce2d8 100644 --- a/src/coreclr/vm/finalizerthread.cpp +++ b/src/coreclr/vm/finalizerthread.cpp @@ -569,6 +569,16 @@ void FinalizerThread::FinalizerThreadWait() EnableFinalization(); - hEventFinalizerDone->Wait(INFINITE,TRUE); + // Under GC stress the finalizer queue may never go empty as frequent + // GCs will keep filling up the queue with items. + // We will disable GC stress to make sure the current thread is not permanently blocked on that. + GCStressPolicy::InhibitHolder iholder; + + //---------------------------------------------------- + // Do appropriate wait and pump messages if necessary + //---------------------------------------------------- + + DWORD status = hEventFinalizerDone->Wait(INFINITE,TRUE); + _ASSERTE(status == WAIT_OBJECT_0); } } From ca127248a3be668afe101befff99d2edfe1942ed Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Thu, 20 Jun 2024 06:58:09 -0700 Subject: [PATCH 03/11] Do not GC stress in preemptive mode --- src/coreclr/vm/gccover.cpp | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/src/coreclr/vm/gccover.cpp b/src/coreclr/vm/gccover.cpp index a0a6c2f1576db..b7ae97613d507 100644 --- a/src/coreclr/vm/gccover.cpp +++ b/src/coreclr/vm/gccover.cpp @@ -1417,16 +1417,9 @@ BOOL OnGcCoverageInterrupt(PCONTEXT regs) RemoveGcCoverageInterrupt(instrPtr, savedInstrPtr, gcCover, offset); return TRUE; } - - // If the thread is in preemptive mode then we must be in a - // PInvoke stub, a method that has an inline PInvoke frame, - // or be in a reverse PInvoke stub that's about to return. - // - // The PInvoke cases should should properly report GC refs if we - // trigger GC here. But a reverse PInvoke stub may over-report - // leading to spurious failures, as we would not normally report - // anything for this method at this point. - if (!pThread->PreemptiveGCDisabled() && pMD->HasUnmanagedCallersOnlyAttribute()) + + // The thread is in preemptive mode. Normally, it should not be able to trigger GC. + if (!pThread->PreemptiveGCDisabled()) { RemoveGcCoverageInterrupt(instrPtr, savedInstrPtr, gcCover, offset); return TRUE; From 9cd4655f92dbe528aba56774714b589cb502f040 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Sun, 23 Jun 2024 15:08:19 -0700 Subject: [PATCH 04/11] higher ratio of gen0 --- src/coreclr/gc/gc.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/gc/gc.cpp b/src/coreclr/gc/gc.cpp index 6032ac3579e05..a9242a804555e 100644 --- a/src/coreclr/gc/gc.cpp +++ b/src/coreclr/gc/gc.cpp @@ -49322,10 +49322,10 @@ bool GCHeap::StressHeap(gc_alloc_context * context) int rgen = StressRNG(100); - // gen0:gen1:gen2 distribution: 80:15:5 - if (rgen >= 95) + // gen0:gen1:gen2 distribution: 90:8:2 + if (rgen >= 98) rgen = 2; - else if (rgen >= 80) + else if (rgen >= 90) rgen = 1; else rgen = 0; From 99161b846f0fab518b9894f24c5442e4c115f432 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Wed, 26 Jun 2024 13:31:31 -0700 Subject: [PATCH 05/11] fix test --- .../baseservices/RuntimeConfiguration/TestConfigTester.csproj | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/tests/baseservices/RuntimeConfiguration/TestConfigTester.csproj b/src/tests/baseservices/RuntimeConfiguration/TestConfigTester.csproj index cd6cb09525e45..48615f4ec48a9 100644 --- a/src/tests/baseservices/RuntimeConfiguration/TestConfigTester.csproj +++ b/src/tests/baseservices/RuntimeConfiguration/TestConfigTester.csproj @@ -1,5 +1,7 @@ + + true true true From 06a9087ddfc0db9ceb47088d3c7c9783569ef51a Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Tue, 2 Jul 2024 13:41:04 -0700 Subject: [PATCH 06/11] make s_nGcStressDisabled sequentially consistent for multiuthreaded use --- src/coreclr/vm/eeconfig.cpp | 2 +- src/coreclr/vm/gcstress.h | 24 +++++++++--------------- 2 files changed, 10 insertions(+), 16 deletions(-) diff --git a/src/coreclr/vm/eeconfig.cpp b/src/coreclr/vm/eeconfig.cpp index 29017c18d3c74..7280a9d441cec 100644 --- a/src/coreclr/vm/eeconfig.cpp +++ b/src/coreclr/vm/eeconfig.cpp @@ -27,7 +27,7 @@ using namespace clr; // is relied on by the EH code and the JIT code (for handling patched // managed code, and GC stress exception) after GC stress is dynamically // turned off. -Volatile GCStressPolicy::InhibitHolder::s_nGcStressDisabled = 0; +int GCStressPolicy::InhibitHolder::s_nGcStressDisabled = 0; #endif // STRESS_HEAP /**************************************************************/ diff --git a/src/coreclr/vm/gcstress.h b/src/coreclr/vm/gcstress.h index 8fe0b962007fe..9102e14578b55 100644 --- a/src/coreclr/vm/gcstress.h +++ b/src/coreclr/vm/gcstress.h @@ -72,26 +72,17 @@ namespace GCStressPolicy private: // This static controls whether GC stress may induce GCs. EEConfig::GetGCStressLevel() still // controls when GCs may occur. - static Volatile s_nGcStressDisabled; - - bool m_bAcquired; + static int s_nGcStressDisabled; public: InhibitHolder() - { LIMITED_METHOD_CONTRACT; ++s_nGcStressDisabled; m_bAcquired = true; } + { LIMITED_METHOD_CONTRACT; GlobalDisable(); } ~InhibitHolder() { LIMITED_METHOD_CONTRACT; Release(); } void Release() - { - LIMITED_METHOD_CONTRACT; - if (m_bAcquired) - { - --s_nGcStressDisabled; - m_bAcquired = false; - } - } + { LIMITED_METHOD_CONTRACT; GlobalEnable();} friend bool IsEnabled(); friend void GlobalDisable(); @@ -99,13 +90,16 @@ namespace GCStressPolicy } UNUSED_ATTR; FORCEINLINE bool IsEnabled() - { return InhibitHolder::s_nGcStressDisabled == 0U; } + { return Interlocked::ExchangeAdd(&InhibitHolder::s_nGcStressDisabled, 0) == 0; } FORCEINLINE void GlobalDisable() - { ++InhibitHolder::s_nGcStressDisabled; } + { Interlocked::Increment(&InhibitHolder::s_nGcStressDisabled); } FORCEINLINE void GlobalEnable() - { --InhibitHolder::s_nGcStressDisabled; } + { + int newVal = Interlocked::Decrement(&InhibitHolder::s_nGcStressDisabled); + _ASSERTE(newVal >= 0); + } #else // STRESS_HEAP From 207d72ba716c2e4a26932016f2bda8e4411958cf Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Tue, 2 Jul 2024 14:03:12 -0700 Subject: [PATCH 07/11] just use a compiler fence, to not raise questions if ordering is important --- src/coreclr/vm/gcstress.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/gcstress.h b/src/coreclr/vm/gcstress.h index 9102e14578b55..8a638e149c3dd 100644 --- a/src/coreclr/vm/gcstress.h +++ b/src/coreclr/vm/gcstress.h @@ -90,7 +90,7 @@ namespace GCStressPolicy } UNUSED_ATTR; FORCEINLINE bool IsEnabled() - { return Interlocked::ExchangeAdd(&InhibitHolder::s_nGcStressDisabled, 0) == 0; } + { return VolatileLoadWithoutBarrier(&InhibitHolder::s_nGcStressDisabled) == 0; } FORCEINLINE void GlobalDisable() { Interlocked::Increment(&InhibitHolder::s_nGcStressDisabled); } From e8a2e9e2208d89f9bd47f667f36574b7a96f5c8c Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Tue, 2 Jul 2024 16:46:09 -0700 Subject: [PATCH 08/11] make GCC happy and bring back m_bAcquired --- src/coreclr/vm/gcstress.h | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/src/coreclr/vm/gcstress.h b/src/coreclr/vm/gcstress.h index 8a638e149c3dd..a6e7dfc2a9e2f 100644 --- a/src/coreclr/vm/gcstress.h +++ b/src/coreclr/vm/gcstress.h @@ -73,16 +73,33 @@ namespace GCStressPolicy // This static controls whether GC stress may induce GCs. EEConfig::GetGCStressLevel() still // controls when GCs may occur. static int s_nGcStressDisabled; + bool m_bAcquired; + + FORCEINLINE static void Disable() + { Interlocked::Increment(&InhibitHolder::s_nGcStressDisabled); } + + FORCEINLINE static void Enable() + { + int newVal = Interlocked::Decrement(&InhibitHolder::s_nGcStressDisabled); + _ASSERTE(newVal >= 0); + } public: InhibitHolder() - { LIMITED_METHOD_CONTRACT; GlobalDisable(); } + { LIMITED_METHOD_CONTRACT; Disable(); m_bAcquired = true;} ~InhibitHolder() { LIMITED_METHOD_CONTRACT; Release(); } void Release() - { LIMITED_METHOD_CONTRACT; GlobalEnable();} + { + LIMITED_METHOD_CONTRACT; + if (m_bAcquired) + { + Enable(); + m_bAcquired = false; + } + } friend bool IsEnabled(); friend void GlobalDisable(); @@ -93,13 +110,10 @@ namespace GCStressPolicy { return VolatileLoadWithoutBarrier(&InhibitHolder::s_nGcStressDisabled) == 0; } FORCEINLINE void GlobalDisable() - { Interlocked::Increment(&InhibitHolder::s_nGcStressDisabled); } + { InhibitHolder::Disable(); } FORCEINLINE void GlobalEnable() - { - int newVal = Interlocked::Decrement(&InhibitHolder::s_nGcStressDisabled); - _ASSERTE(newVal >= 0); - } + { InhibitHolder::Enable(); } #else // STRESS_HEAP From 3b7cac25583fc237eefbf4ed294a44a44694ee94 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Tue, 2 Jul 2024 17:56:07 -0700 Subject: [PATCH 09/11] workaround an unused variable warning --- src/coreclr/vm/gcstress.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/coreclr/vm/gcstress.h b/src/coreclr/vm/gcstress.h index a6e7dfc2a9e2f..5033a4032c83b 100644 --- a/src/coreclr/vm/gcstress.h +++ b/src/coreclr/vm/gcstress.h @@ -82,6 +82,7 @@ namespace GCStressPolicy { int newVal = Interlocked::Decrement(&InhibitHolder::s_nGcStressDisabled); _ASSERTE(newVal >= 0); + __UNUSED(newVal); } public: From db84183b576896fda111f752988a9556bc9151ae Mon Sep 17 00:00:00 2001 From: Vladimir Sadov Date: Tue, 2 Jul 2024 18:18:07 -0700 Subject: [PATCH 10/11] Apply suggestions from code review Co-authored-by: Jan Kotas --- src/coreclr/vm/gcstress.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/vm/gcstress.h b/src/coreclr/vm/gcstress.h index 5033a4032c83b..23b11d9989fcf 100644 --- a/src/coreclr/vm/gcstress.h +++ b/src/coreclr/vm/gcstress.h @@ -80,9 +80,9 @@ namespace GCStressPolicy FORCEINLINE static void Enable() { - int newVal = Interlocked::Decrement(&InhibitHolder::s_nGcStressDisabled); + int newVal; + newVal = Interlocked::Decrement(&InhibitHolder::s_nGcStressDisabled); _ASSERTE(newVal >= 0); - __UNUSED(newVal); } public: From f542fd4e1ed1c0d0458119b6e2fb5ea1ea3756c8 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Tue, 2 Jul 2024 18:27:27 -0700 Subject: [PATCH 11/11] use more compiler-happy pattern when asserting on return value. --- src/coreclr/vm/finalizerthread.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/vm/finalizerthread.cpp b/src/coreclr/vm/finalizerthread.cpp index f898bed3ce2d8..e543a3c60c346 100644 --- a/src/coreclr/vm/finalizerthread.cpp +++ b/src/coreclr/vm/finalizerthread.cpp @@ -578,7 +578,8 @@ void FinalizerThread::FinalizerThreadWait() // Do appropriate wait and pump messages if necessary //---------------------------------------------------- - DWORD status = hEventFinalizerDone->Wait(INFINITE,TRUE); + DWORD status; + status = hEventFinalizerDone->Wait(INFINITE,TRUE); _ASSERTE(status == WAIT_OBJECT_0); } }