Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't re-instrument methods which don't need that #84772

Closed
wants to merge 3 commits into from
Closed

Don't re-instrument methods which don't need that #84772

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
78efc23
address feedback
EgorBo Apr 17, 2023
53aa380
Merge branch 'main' of github.com:dotnet/runtime into fix-redundant-i…
EgorBo Apr 19, 2023
46a343e
Cache GetOptimizationTier call
EgorBo Apr 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion src/coreclr/vm/codeversion.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
20 changes: 17 additions & 3 deletions src/coreclr/vm/codeversion.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest using an enum class to avoid polluting the parent namespace, though this may need to be moved based on my other comment.

{
None = 0,
ShouldSkipInstrumentationFlag = 1,
InstrumentedFlag = 2,
};

#ifndef FEATURE_CODE_VERSIONING
PTR_MethodDesc m_pMethodDesc;
#else // FEATURE_CODE_VERSIONING
Expand All @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
20 changes: 20 additions & 0 deletions src/coreclr/vm/jitinterface.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12131,6 +12131,26 @@ 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The active code version may have changed meanwhile, so it may not represent the current code version being jitted. Could use something like this instead:

runtime/src/coreclr/vm/jitinterface.cpp

Line 6681 in 7b662a7

PrepareCodeConfig *config = GetThread()->GetCurrentPrepareCodeConfig();

Along with config->GetCodeVersion().


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.
currentVersion.SetShouldSkipInstrumentation();
}

// Leave a note that this method is properly instrumented
currentVersion.SetInstrumented();
Copy link
Member

@kouvel kouvel Apr 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NativeCodeVersion is a simple wrapper around either a MethodDesc or a NativeCodeVersionNode, and is typically passed by value. The default native code version doesn't have a node and just wraps a MethodDesc. So, it's not a good candidate for persistent storage of this type of info. This change to the code version would just modify the local copy and would not be persisted anywhere.

AFAIK there isn't a clear place currently where such info could be stored. CallCountingInfo may be a candidate, for instance see CallCountingManager::DisableCallCounting() and CallCountingInfo::CreateWithCallCountingDisabled(), which are used to indicate that a code version should not be tiered. However, currently it's not set up to store extra info aside from call counting, for example CallCountingManager::SetCodeEntryPoint() does something different if the CallCountingInfo already exists as opposed to when it's being created. It may be possible to enable precreating a CallCountingInfo for a code version by introducing a new CallCountingInfo::Stage something like NotStarted to indicate that call counting has not started, and to modify SetCodeEntryPoint() to do something similar to before in that case. If the CallCountingInfo already exists, it could be used as persistent storage, as the CallCountingManager has a hash table mapping a code version to a CallCountingInfo.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's better to rollback to occupying a spare bit in MethodDesc m_flag2 then? This change is a sort of an optimization, not correctness. The problem with CallCountingStubs that they're created assyncronlsy like you pointed out

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The info appears to be specific to a code version rather than a method desc, so I'm not sure the method desc would be an appropriate place for it

}

#ifdef FEATURE_PGO
hr = PgoManager::allocPgoInstrumentationBySchema(m_pMethodBeingCompiled, pSchema, countSchemaItems, pInstrumentationData);
#else
Expand Down
16 changes: 15 additions & 1 deletion src/coreclr/vm/tieredcompilation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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

Expand Down