Don't re-use callcountingstubs for now

dotnet · EgorBo · Oct 26, 2022 · Jun 23, 2022 · Jun 23, 2022 · Jun 24, 2022
commit 41733fede75f7c0b0cf9015a6842a4cb2d5221c5
diff --git a/src/coreclr/inc/clrconfigvalues.h b/src/coreclr/inc/clrconfigvalues.h
@@ -594,7 +594,6 @@ RETAIL_CONFIG_DWORD_INFO(INTERNAL_TC_DelaySingleProcMultiplier, W("TC_DelaySingl
 RETAIL_CONFIG_DWORD_INFO(INTERNAL_TC_CallCounting, W("TC_CallCounting"), 1, "Enabled by default (only activates when TieredCompilation is also enabled). If disabled immediately backpatches prestub, and likely prevents any promotion to higher tiers")
 RETAIL_CONFIG_DWORD_INFO(INTERNAL_TC_UseCallCountingStubs, W("TC_UseCallCountingStubs"), 1, "Uses call counting stubs for faster call counting.")
 RETAIL_CONFIG_DWORD_INFO(INTERNAL_TC_DeleteCallCountingStubsAfter, W("TC_DeleteCallCountingStubsAfter"), 0, "Deletes call counting stubs after this many have completed. Zero to disable deleting.")
-
 #undef TC_BackgroundWorkerTimeoutMs
 #undef TC_CallCountThreshold
 #undef TC_CallCountingDelayMs

diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h
@@ -9101,15 +9101,6 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
         }
 #endif
 
-        bool IsInstrumented()
-        {
-            return jitFlags->IsSet(JitFlags::JIT_FLAG_BBINSTR);
-        }
-        bool IsInstrumentedOptimized()
-        {
-            return IsInstrumented() && jitFlags->IsSet(JitFlags::JIT_FLAG_TIER1);
-        }
-
         // true if we should use the PINVOKE_{BEGIN,END} helpers instead of generating
         // PInvoke transitions inline. Normally used by R2R, but also used when generating a reverse pinvoke frame, as
         // the current logic for frame setup initializes and pushes

diff --git a/src/coreclr/jit/fgprofile.cpp b/src/coreclr/jit/fgprofile.cpp
@@ -383,8 +383,7 @@ void BlockCountInstrumentor::Prepare(bool preImport)
     //
     // If we see any, we need to adjust our instrumentation pattern.
     //
-    if ((m_comp->opts.IsOSR() || m_comp->opts.IsInstrumentedOptimized()) &&
-        ((m_comp->optMethodFlags & OMF_HAS_TAILCALL_SUCCESSOR) != 0))
+    if (m_comp->opts.IsOSR() && ((m_comp->optMethodFlags & OMF_HAS_TAILCALL_SUCCESSOR) != 0))
     {
         JITDUMP("OSR + PGO + potential tail call --- preparing to relocate block probes\n");
 
@@ -1888,8 +1887,8 @@ PhaseStatus Compiler::fgPrepareToInstrumentMethod()
         (JitConfig.TC_PartialCompilation() > 0);
     const bool prejit               = opts.jitFlags->IsSet(JitFlags::JIT_FLAG_PREJIT);
     const bool tier0WithPatchpoints = opts.jitFlags->IsSet(JitFlags::JIT_FLAG_TIER0) && mayHavePatchpoints;
-    const bool useEdgeProfiles =
-        (JitConfig.JitEdgeProfiling() > 0) && !prejit && !tier0WithPatchpoints && !opts.IsInstrumentedOptimized();
+    const bool osrMethod            = opts.IsOSR();
+    const bool useEdgeProfiles = (JitConfig.JitEdgeProfiling() > 0) && !prejit && !tier0WithPatchpoints && !osrMethod;
 
     if (useEdgeProfiles)
     {
@@ -1900,7 +1899,7 @@ PhaseStatus Compiler::fgPrepareToInstrumentMethod()
         JITDUMP("Using block profiling, because %s\n",
                 (JitConfig.JitEdgeProfiling() == 0)
                     ? "edge profiles disabled"
-                    : prejit ? "prejitting" : opts.IsInstrumentedOptimized() ? "tier1" : "tier0 with patchpoints");
+                    : prejit ? "prejitting" : osrMethod ? "OSR" : "tier0 with patchpoints");
 
         fgCountInstrumentor = new (this, CMK_Pgo) BlockCountInstrumentor(this);
     }

diff --git a/src/coreclr/vm/callcounting.cpp b/src/coreclr/vm/callcounting.cpp
@@ -147,18 +147,13 @@ FORCEINLINE void CallCountingManager::CallCountingInfo::SetStage(Stage stage)
             --s_activeCallCountingStubCount;
             break;
 
-        case Stage::PendingReset:
-            _ASSERT(m_stage >= Stage::PendingCompletion);
-            --s_activeCallCountingStubCount;
-            break;
-
         case Stage::StubMayBeActive:
             _ASSERTE(m_callCountingStub != nullptr);
             FALLTHROUGH;
 
         case Stage::PendingCompletion:
-            _ASSERTE(m_stage == Stage::StubIsNotActive || m_stage == Stage::StubMayBeActive || m_stage == Stage::PendingReset);
-            if ((m_stage == Stage::StubIsNotActive || m_stage == Stage::PendingReset) && m_callCountingStub != nullptr)
+            _ASSERTE(m_stage == Stage::StubIsNotActive || m_stage == Stage::StubMayBeActive);
+            if (m_stage == Stage::StubIsNotActive && m_callCountingStub != nullptr)
             {
                 ++s_activeCallCountingStubCount;
             }
@@ -183,13 +178,6 @@ FORCEINLINE void CallCountingManager::CallCountingInfo::SetStage(Stage stage)
 
     m_stage = stage;
 }
-
-void CallCountingManager::CallCountingInfo::ReuseForNewVersion(NativeCodeVersion version)
-{
-    WRAPPER_NO_CONTRACT;
-    m_codeVersion = version;
-    SetStage(Stage::PendingReset);
-}
 #endif
 
 ////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
@@ -551,28 +539,6 @@ void CallCountingManager::DisableCallCounting(NativeCodeVersion codeVersion)
     callCountingInfoHolder.SuppressRelease();
 }
 
-void CallCountingManager::ReuseStubForNewVersion(NativeCodeVersion oldVersion, NativeCodeVersion newVersion)
-{
-    CONTRACTL
-    {
-        THROWS;
-        GC_NOTRIGGER;
-        MODE_ANY;
-    }
-    CONTRACTL_END;
-
-    CodeVersionManager::LockHolder codeVersioningLockHolder;
-    CallCountingInfo* callCountingInfo = m_callCountingInfoByCodeVersionHash.Lookup(oldVersion);
-
-    // No problem if it's null, it's just an optimization to allocate less call counting stubs
-    if (callCountingInfo != nullptr)
-    {
-        m_callCountingInfoByCodeVersionHash.Remove(oldVersion);
-        callCountingInfo->ReuseForNewVersion(newVersion);
-        m_callCountingInfoByCodeVersionHash.Add(callCountingInfo);
-    }
-}
-
 // Returns true if the code entry point was updated to reflect the active code version, false otherwise. In normal paths, the
 // code entry point is not updated only when the use of call counting stubs is disabled, as in that case returning to the
 // prestub is necessary for further call counting. On exception, the code entry point may or may not have been updated and it's
@@ -652,19 +618,6 @@ bool CallCountingManager::SetCodeEntryPoint(
 
             do
             {
-                if (callCountingStage == CallCountingInfo::Stage::PendingReset)
-                {
-                    // Consider using a different threshold here
-                    CallCount callCountThreshold = g_pConfig->TieredCompilation_CallCountThreshold();
-                    _ASSERTE(callCountThreshold != 0);
-
-                    CallCountingStub *stub = (CallCountingStub*)callCountingInfo->GetCallCountingStub();
-                    // Patch existing CallCountingStub to look at new target
-                    stub->SetTargetForMethod(activeCodeVersion.GetNativeCode());
-                    *callCountingInfo->GetRemainingCallCountCell() = callCountThreshold;
-                    break;
-                }
-
                 if (!wasMethodCalled)
                 {
                     break;
@@ -844,7 +797,7 @@ PCODE CallCountingManager::OnCallCountThresholdReached(TransitionBlock *transiti
             }
 
             CallCountingInfo::Stage callCountingStage = callCountingInfo->GetStage();
-            if (callCountingStage >= CallCountingInfo::Stage::PendingReset)
+            if (callCountingStage >= CallCountingInfo::Stage::PendingCompletion)
             {
                 break;
             }
@@ -977,11 +930,7 @@ void CallCountingManager::CompleteCallCounting()
                     methodDesc->ResetCodeEntryPoint();
                 } while (false);
 
-                // stub was about to be completed but we decided to re-use it
-                if (callCountingInfo->GetStage() != CallCountingInfo::Stage::PendingReset)
-                {
-                    callCountingInfo->SetStage(CallCountingInfo::Stage::Complete);
-                }
+                callCountingInfo->SetStage(CallCountingInfo::Stage::Complete);
             }
             EX_CATCH
             {

diff --git a/src/coreclr/vm/callcounting.h b/src/coreclr/vm/callcounting.h
@@ -150,7 +150,6 @@ class CallCountingStub
 
     PTR_CallCount GetRemainingCallCountCell() const;
     PCODE GetTargetForMethod() const;
-    void SetTargetForMethod(PCODE code);
 
 protected:
 
@@ -183,9 +182,6 @@ class CallCountingManager
             // Stub may be called, don't know if it's actually active (changes to code versions, etc.)
             StubMayBeActive,
 
-            // Stub is being considered to re-use
-            PendingReset,
-
             // Stub may be active, call counting complete, not yet promoted
             PendingCompletion,
 
@@ -197,7 +193,7 @@ class CallCountingManager
         };
 
     private:
-        NativeCodeVersion m_codeVersion;
+        const NativeCodeVersion m_codeVersion;
         const CallCountingStub *m_callCountingStub;
         CallCount m_remainingCallCount;
         Stage m_stage;
@@ -423,12 +419,6 @@ inline PCODE CallCountingStub::GetTargetForMethod() const
     return GetData()->TargetForMethod;
 }
 
-inline void CallCountingStub::SetTargetForMethod(PCODE code)
-{
-    WRAPPER_NO_CONTRACT;
-    InterlockedExchangeT<PCODE>(&GetData()->TargetForMethod, code);
-}
-
 ////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
 // CallCountingManager::CallCountingStubManager