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.

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 delay call-counting-installation because of R2R'd code #77635

Open
EgorBo opened this issue Oct 29, 2022 · 4 comments
Open

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

EgorBo opened this issue Oct 29, 2022 · 4 comments

Comments

@EgorBo
Copy link
Member

EgorBo commented Oct 29, 2022

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 #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 #70941 (comment)

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Oct 29, 2022
@EgorBo EgorBo self-assigned this Oct 29, 2022
@EgorBo EgorBo removed the untriaged New issue has not been triaged by the area owner label Oct 29, 2022
@EgorBo EgorBo added this to the 8.0.0 milestone Oct 29, 2022
@davidwrighton
Copy link
Member

#79021 has fixed the reason not to do this

@mangod9
Copy link
Member

mangod9 commented Jul 3, 2023

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

@EgorBo
Copy link
Member Author

EgorBo commented Jul 3, 2023

@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 EgorBo modified the milestones: 8.0.0, Future Jul 3, 2023
@EgorBo
Copy link
Member Author

EgorBo commented Jul 3, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants