address feedback

dotnet · Apr 17, 2023 · 78efc23 · 78efc23
1 parent 63b9a3a
commit 78efc23
Show file tree

Hide file tree

Showing 4 changed files with 78 additions and 5 deletions.
diff --git a/src/coreclr/vm/codeversion.cpp b/src/coreclr/vm/codeversion.cpp
@@ -194,11 +194,13 @@ void NativeCodeVersionNode::SetGCCoverageInfo(PTR_GCCoverageInfo gcCover)
 
 NativeCodeVersion::NativeCodeVersion(PTR_NativeCodeVersionNode pVersionNode) :
     m_storageKind(pVersionNode != NULL ? StorageKind::Explicit : StorageKind::Unknown),
+    m_flags(None),
     m_pVersionNode(pVersionNode)
 {}
 
 NativeCodeVersion::NativeCodeVersion(PTR_MethodDesc pMethod) :
-    m_storageKind(pMethod != NULL ? StorageKind::Synthetic : StorageKind::Unknown)
+    m_storageKind(pMethod != NULL ? StorageKind::Synthetic : StorageKind::Unknown),
+    m_flags(None)
 {
     LIMITED_METHOD_DAC_CONTRACT;
     m_synthetic.m_pMethodDesc = pMethod;
@@ -283,6 +285,30 @@ BOOL NativeCodeVersion::IsActiveChildVersion() const
     }
 }
 
+void NativeCodeVersion::SetShouldSkipInstrumentation()
+{
+    LIMITED_METHOD_CONTRACT;
+    m_flags = (NativeCodeVersionFlag)(m_flags | ShouldSkipInstrumentationFlag);
+}
+
+bool NativeCodeVersion::ShouldSkipInstrumentation() const
+{
+    LIMITED_METHOD_CONTRACT;
+    return m_flags & ShouldSkipInstrumentationFlag;
+}
+
+void NativeCodeVersion::SetInstrumented()
+{
+    LIMITED_METHOD_CONTRACT;
+    m_flags = (NativeCodeVersionFlag)(m_flags | InstrumentedFlag);
+}
+
+bool NativeCodeVersion::IsInstrumented() const
+{
+    LIMITED_METHOD_CONTRACT;
+    return m_flags & InstrumentedFlag;
+}
+
 PTR_MethodDescVersioningState NativeCodeVersion::GetMethodDescVersioningState() const
 {
     LIMITED_METHOD_DAC_CONTRACT;

diff --git a/src/coreclr/vm/codeversion.h b/src/coreclr/vm/codeversion.h
@@ -106,8 +106,21 @@ class NativeCodeVersion
     PTR_NativeCodeVersionNode AsNode() const;
 #endif
 
+    void SetShouldSkipInstrumentation();
+    bool ShouldSkipInstrumentation() const;
+
+    void SetInstrumented();
+    bool IsInstrumented() const;
+
 private:
 
+    enum NativeCodeVersionFlag : UINT16
+    {
+        None = 0,
+        ShouldSkipInstrumentationFlag = 1,
+        InstrumentedFlag = 2,
+    };
+
 #ifndef FEATURE_CODE_VERSIONING
     PTR_MethodDesc m_pMethodDesc;
 #else // FEATURE_CODE_VERSIONING
@@ -122,14 +135,15 @@ class NativeCodeVersion
     BOOL IsActiveChildVersion() const;
     PTR_MethodDescVersioningState GetMethodDescVersioningState() const;
 
-    enum StorageKind
+    enum StorageKind : UINT16
     {
         Unknown,
         Explicit,
         Synthetic
     };
 
     StorageKind m_storageKind;
+    NativeCodeVersionFlag m_flags;
     union
     {
         PTR_NativeCodeVersionNode m_pVersionNode;
@@ -694,7 +708,7 @@ class CodeVersionManager
 
 inline NativeCodeVersion::NativeCodeVersion()
 #ifdef FEATURE_CODE_VERSIONING
-    : m_storageKind(StorageKind::Unknown), m_pVersionNode(PTR_NULL)
+    : m_storageKind(StorageKind::Unknown), m_flags(None), m_pVersionNode(PTR_NULL)
 #else
     : m_pMethodDesc(PTR_NULL)
 #endif
@@ -707,7 +721,7 @@ inline NativeCodeVersion::NativeCodeVersion()
 
 inline NativeCodeVersion::NativeCodeVersion(const NativeCodeVersion & rhs)
 #ifdef FEATURE_CODE_VERSIONING
-    : m_storageKind(rhs.m_storageKind), m_pVersionNode(rhs.m_pVersionNode)
+    : m_storageKind(rhs.m_storageKind), m_flags(None), m_pVersionNode(rhs.m_pVersionNode)
 #else
     : m_pMethodDesc(rhs.m_pMethodDesc)
 #endif

diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp
@@ -12129,6 +12129,25 @@ HRESULT CEEJitInfo::allocPgoInstrumentationBySchema(
         codeSize = m_ILHeader->GetCodeSize();
     }
 
+    if (m_pMethodBeingCompiled->IsVersionable())
+    {
+        CodeVersionManager* pCodeVersionManager = m_pMethodBeingCompiled->GetCodeVersionManager();
+        CodeVersionManager::LockHolder codeVersioningLockHolder;
+        ILCodeVersion ilVersion = pCodeVersionManager->GetActiveILCodeVersion(m_pMethodBeingCompiled);
+        NativeCodeVersion currentVersion = ilVersion.GetActiveNativeCodeVersion(m_pMethodBeingCompiled);
+
+        if ((currentVersion.GetOptimizationTier() == NativeCodeVersion::OptimizationTier::OptimizationTier0) ||
+            (currentVersion.GetOptimizationTier() == NativeCodeVersion::OptimizationTier::OptimizationTier1))
+        {
+            // Current tier is not marked as instrumented, but it requested the schemas so it means it doesn't
+            // need to be re-instrumented in future and can be promoted straight to Tier1 if hot enough.
+            currentVersion.SetShouldSkipInstrumentation();
+        }
+
+        // Leave a note that this method is properly instrumented
+        currentVersion.SetInstrumented();
+    }
+
 #ifdef FEATURE_PGO
     hr = PgoManager::allocPgoInstrumentationBySchema(m_pMethodBeingCompiled, pSchema, countSchemaItems, pInstrumentationData);
 #else

diff --git a/src/coreclr/vm/tieredcompilation.cpp b/src/coreclr/vm/tieredcompilation.cpp
@@ -284,10 +284,16 @@ void TieredCompilationManager::AsyncPromoteToTier1(
 #ifdef FEATURE_PGO
     if (g_pConfig->TieredPGO())
     {
+        NativeCodeVersion::OptimizationTier currentTier = currentNativeCodeVersion.GetOptimizationTier();
         if (currentNativeCodeVersion.GetOptimizationTier() == NativeCodeVersion::OptimizationTier0 &&
             g_pConfig->TieredPGO_InstrumentOnlyHotCode())
         {
-            if (ExecutionManager::IsReadyToRunCode(currentNativeCodeVersion.GetNativeCode()))
+            if (currentNativeCodeVersion.ShouldSkipInstrumentation())
+            {
+                // Skip instrumentation on explicit request
+                nextTier = NativeCodeVersion::OptimizationTier1;
+            }
+            else if (ExecutionManager::IsReadyToRunCode(currentNativeCodeVersion.GetNativeCode()))
             {
                 // We definitely don't want to use unoptimized instrumentation tier for hot R2R:
                 // 1) It will produce a lot of new compilations for small methods which were inlined in R2R
@@ -314,6 +320,14 @@ void TieredCompilationManager::AsyncPromoteToTier1(
                 // made it to Tier1-OSR.
             }
         }
+        else if (currentTier == NativeCodeVersion::OptimizationTier1Instrumented &&
+            !currentNativeCodeVersion.IsInstrumented())
+        {
+            // JIT didn't ask for PGO schemes (too simple to instrument?) and the current tier has optimizations
+            // enabled - it means it's already the final tier, no need to promote further. Call counting stubs
+            // are also expected to be removed by this point.
+            return;
+        }
     }
 #endif