Skip to content

Add an IsFuncletCache to the EECodeInfo #114580

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

Merged
merged 3 commits into from
Apr 15, 2025

Conversation

davidwrighton
Copy link
Member

  • Do a cheap computation in EECodeInfo::Init time to fill it in if there aren't any funclets in the method
  • This avoids the fairly complex IsFunclet logic on the JitManager, as well as the virtual call to get there in some cases.

This should improve the performance of any EH stack trace operations that repeatedly call IsFunclet, especially if many of the methods on the stack are simple and do not have any EH constructs in them.

In a simple benchmark of EH throw performance, when running in the Windows X86 Funclets mode, the performance improves by about 15%.

…t EECodeInfo::Init time to fill it in if there aren't any funclets in the method

- This avoids the fairly complex IsFunclet logic on the JitManager, as well as the virtual call to get there in some cases.
@Copilot Copilot AI review requested due to automatic review settings April 11, 2025 23:21
@ghost ghost added the area-VM-coreclr label Apr 11, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Copy link
Contributor

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

Comment on lines 300 to 305
IsFuncletCache ComputeInitialIsFuncletCacheValue(PCODE codePtr)
{
if (GetNumberOfUnwindInfos() == 1)
return IsFuncletCache::IsNotFunclet; // Optimize the scenario where there are not any funclets in a method
return IsFuncletCache::NotSet;
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
IsFuncletCache ComputeInitialIsFuncletCacheValue(PCODE codePtr)
{
if (GetNumberOfUnwindInfos() == 1)
return IsFuncletCache::IsNotFunclet; // Optimize the scenario where there are not any funclets in a method
return IsFuncletCache::NotSet;
}
bool MayHaveFunclets()
{
return GetNumberOfUnwindInfos() > 1;
}

It would be nice if IsFuncletCache is kept as a private detail of EECodeInfo.

BOOL IsFunclet()
{
WRAPPER_NO_CONTRACT;
if (m_isFuncletCache == IsFuncletCache::NotSet)
Copy link
Member

Choose a reason for hiding this comment

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

This looks very similar to the lazy initialization used for GetFunctionEntry() that is also initialized eagerly in some cases and lazily in other cases. It would be nice if the two look exactly the same:

  • Move the implementation of IsFunclet to .cpp file next to GetFunctionEntry
  • Rename method on the code manager interface to LazyIsFunclet

@@ -4294,6 +4294,7 @@ BOOL EECodeGenManager::JitCodeToMethodInfoWorker(
#ifdef FEATURE_EH_FUNCLETS
// Computed lazily by code:EEJitManager::LazyGetFunctionEntry
pCodeInfo->m_pFunctionEntry = NULL;
pCodeInfo->m_isFuncletCache = pCHdr->ComputeInitialIsFuncletCacheValue(currentPC);
Copy link
Member

@jkotas jkotas Apr 12, 2025

Choose a reason for hiding this comment

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

Suggested change
pCodeInfo->m_isFuncletCache = pCHdr->ComputeInitialIsFuncletCacheValue(currentPC);
if (pCHdr->MayHaveFunclets())
{
// Computed lazily by code:EEJitManager::LazyGetFunctionEntry
pCodeInfo->m_pFunctionEntry = NULL;
pCodeInfo->m_isFuncletCache = IsFuncletCache::NotSet;
}
else
{
pCodeInfo->m_pFunctionEntry = pCHdr->GetUnwindInfo(0);
pCodeInfo->m_isFuncletCache = IsFuncletCache::IsNotFunclet;
}

We may be able to do even more cheap caching for the common case here.

Copy link
Member

Choose a reason for hiding this comment

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

Nit: Typo - s/(0)(/(0).

I already had this optimization locally and it's useful.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

LGTM!

@jkotas jkotas merged commit 9fd6a57 into dotnet:main Apr 15, 2025
94 of 97 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators May 16, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants