From 78efc2365e6ee68b41beb24bc654e65d4ca15c7f Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 18 Apr 2023 01:30:07 +0200 Subject: [PATCH 1/2] address feedback --- src/coreclr/vm/codeversion.cpp | 28 +++++++++++++++++++++++++++- src/coreclr/vm/codeversion.h | 20 +++++++++++++++++--- src/coreclr/vm/jitinterface.cpp | 19 +++++++++++++++++++ src/coreclr/vm/tieredcompilation.cpp | 16 +++++++++++++++- 4 files changed, 78 insertions(+), 5 deletions(-) diff --git a/src/coreclr/vm/codeversion.cpp b/src/coreclr/vm/codeversion.cpp index 6a5db21db37f6..5553ce0718e4e 100644 --- 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 index 66de4ba27257a..3133b8b620e4f 100644 --- 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,7 +135,7 @@ class NativeCodeVersion BOOL IsActiveChildVersion() const; PTR_MethodDescVersioningState GetMethodDescVersioningState() const; - enum StorageKind + enum StorageKind : UINT16 { Unknown, Explicit, @@ -130,6 +143,7 @@ class NativeCodeVersion }; 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 index 8c4e44ef7eac5..f5ac8e4ac034d 100644 --- 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 index c08e945ccab00..4f0a472f47811 100644 --- 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 From 46a343eb3aba57770256a8a18445e89087876e31 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 19 Apr 2023 11:47:53 +0200 Subject: [PATCH 2/2] Cache GetOptimizationTier call --- src/coreclr/vm/jitinterface.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index 1b046c56cc38f..650eb1eac477e 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -12138,8 +12138,9 @@ HRESULT CEEJitInfo::allocPgoInstrumentationBySchema( ILCodeVersion ilVersion = pCodeVersionManager->GetActiveILCodeVersion(m_pMethodBeingCompiled); NativeCodeVersion currentVersion = ilVersion.GetActiveNativeCodeVersion(m_pMethodBeingCompiled); - if ((currentVersion.GetOptimizationTier() == NativeCodeVersion::OptimizationTier::OptimizationTier0) || - (currentVersion.GetOptimizationTier() == NativeCodeVersion::OptimizationTier::OptimizationTier1)) + NativeCodeVersion::OptimizationTier currentOptTier = currentVersion.GetOptimizationTier(); + if ((currentOptTier == NativeCodeVersion::OptimizationTier::OptimizationTier0) || + (currentOptTier == 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.