search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.29k stars 4.74k forks source link

Don't delay call-counting-installation because of R2R'd code #77635

Open EgorBo opened 2 years ago

EgorBo commented 2 years ago

When we re-jit a R2R'd code we hit HandleCallCountingForFirstCall where we set m_tier1CallCountingCandidateMethodRecentlyRecorded = true to delay call-counting installation further, see here.

It seems to me that we should not do that when we already have R2R'd version installed, a proper fix is

if (!ExecutionManager::IsReadyToRunCode(pMethodDesc->GetNativeCode()))
{
    // Delay call counting for currently recoded methods further
    m_tier1CallCountingCandidateMethodRecentlyRecorded = true;
}

However, ExecutionManager::IsReadyToRunCode is likely not cheap due to https://github.com/dotnet/runtime/issues/8393 issue 😞

The problem, it should help with, is: when we start a large app we constantly compile something and VM simply doesn't have a window to install call counting stubs for methods, even extremely hot ones - they might stuck un-counted in Tier0 for seconds, see https://github.com/dotnet/runtime/pull/70941#issuecomment-1284348407

davidwrighton commented 1 year ago

79021 has fixed the reason not to do this

mangod9 commented 1 year ago

@EgorBo, this issue is tagged for 8. Assume its not a large change to make it in?

EgorBo commented 1 year ago

@EgorBo, this issue is tagged for 8. Assume its not a large change to make it in?

The fix is more or less simple but I wasn't able yet to proove it improves anything yet, need to try to some big 1P project

EgorBo commented 1 year ago

I moved it to Future but as a best-effort to make it to 8.0 if we find a proof it works