Skip to content

Replace CodeManState with cache in EECodeInfo for x86. #114170

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 9, 2025

Conversation

filipnavara
Copy link
Member

@filipnavara filipnavara commented Apr 2, 2025

On all other platforms CodeManState was unused and taking space in StackFrameIterator.

Optimize some code paths to cache GC info early or decode the method size without decoding the whole table.

@ghost ghost added the area-VM-coreclr label Apr 2, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Apr 2, 2025
Copy link
Contributor

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

@filipnavara
Copy link
Member Author

filipnavara commented Apr 2, 2025

This is not an universal win.

It makes StackFrameIterator on x86 slightly larger because it contains two EECodeInfo classes. It may also increase stack usage in places where EECodeInfo is used but most of those seem to access the GC info at some point anyway so embedding the buffer for the cache doesn't make things worse.

On other platforms it saves an unused space. There seem to be no downsides.

It gets rid of one x86 quirk and makes it easier to reuse the cache in FEATURE_EH_FUNCLETS builds, currently to very small benefit (about 5% speed up of stack iterators).

Thoughts?

(I am aware of the build issues due to mismatching offsets; not worth resolving if this whole direction gets abandoned)

@filipnavara filipnavara requested a review from jkotas April 2, 2025 20:30
@jkotas
Copy link
Member

jkotas commented Apr 3, 2025

LGTM! I like that it gets rid of the unused cache on !x86.

@filipnavara
Copy link
Member Author

LGTM! I like that it gets rid of the unused cache on !x86.

Thanks for having a look. I made some progress on this locally and it helped me improve the performance on x86+funclet platforms. I'll clean up the PR soon.

@filipnavara filipnavara changed the title RFC: Replace CodeManState with cache in EECodeInfo for x86. Replace CodeManState with cache in EECodeInfo for x86. Apr 8, 2025
On all other platforms CodeManState was unused and taking space in
StackFrameIterator.

Optimize some code paths to cache GC info early or decode the method
size without decoding the whole table.

Move PCTAddr (Eip pointer) into KNONVOLATILE_CONTEXT_POINTERS_EX to
avoid recomputing it when switching between callee and caller contexts.
@filipnavara filipnavara marked this pull request as ready for review April 8, 2025 14:16
Comment on lines +138 to 143
inline TADDR GetRegdisplayPCTAddr(REGDISPLAY *display)
{
return display->PCTAddr;
}

inline void SetRegdisplayPCTAddr(REGDISPLAY *display, TADDR addr)
Copy link
Member Author

Choose a reason for hiding this comment

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

I initially tried to optimize how the PCTAddr is stored for callee context to avoid recomputation. Unfortunately it results in some bugs (eg. CopyRegDisplay doesn't transfer the information correctly, IsInCalleesFrames depends on quirks in the initial state, etc.) and I opted not to pursue this optimization at this time. I left the helper methods in place to make it easier to address this in future and to create a central place where we can _ASSERTE on invalid values.

…e and thus avoid repeated GetGCInfoToken() calls
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. Thank you!

@janvorli Could you please take a look as well?

Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@janvorli
Copy link
Member

janvorli commented Apr 9, 2025

/ba-g the test failure has been happening consistently on all runs since yesterday. #114426

@janvorli janvorli merged commit 2b922d0 into dotnet:main Apr 9, 2025
96 of 98 checks passed
@filipnavara filipnavara deleted the no-codemanstate branch April 9, 2025 11:55
@github-actions github-actions bot locked and limited conversation to collaborators May 10, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-VM-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants