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

Move DispatchMap pointer to MethodTable #85698

Merged
merged 1 commit into from
May 3, 2023

Conversation

MichalStrehovsky
Copy link
Member

Sending this for consideration. The old approach also had an advantage. Wouldn't be the end of the world if we keep that.

Before this PR, accessing dispatch map involved:

  • Reading optional fields to find the field with the right tag
  • The optional field contained an integer index into a table
  • The index was used to index into a dispatch map table to find a pointer to the actual dispatch map
  • We then followed the pointer to get to the dispatch map.

The advantage of this scheme is smaller working set (MethodTable is smaller), but this assumes the MethodTable has other optional fields (because we still need a pointer to the optional fields). Turns out most MethodTables only need optional fields pointer because of the dispatch map and if we move them to MethodTable, we no longer need an optional field pointer.

This PR simply moves the dispatch map pointer to MethodTable.

I'm seeing another 15 kB saving on BasicMinimalApi. Plus the code looks simpler.

Cc @dotnet/ilc-contrib

Sending this for consideration. The old approach also had an advantage. Wouldn't be the end of the world if we keep that.

Before this PR, accessing dispatch map involved:
* Reading optional fields to find the field with the right tag
* The optional field contained an integer index into a table
* The index was used to index into a dispatch map table to find a pointer to the actual dispatch map
* We then followed the pointer to get to the dispatch map.

The advantage of this scheme is smaller working set (MethodTable is smaller), but this assumes the MethodTable has other optional fields (because we still need a pointer to the optional fields). Turns out most MethodTables only need optional fields pointer because of the dispatch map and if we move them to MethodTable, we no longer need an optional field pointer.

This PR simply moves the dispatch map pointer to MethodTable.

I'm seeing another 15 kB saving on BasicMinimalApi. Plus the code looks simpler.
@ghost
Copy link

ghost commented May 3, 2023

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

Sending this for consideration. The old approach also had an advantage. Wouldn't be the end of the world if we keep that.

Before this PR, accessing dispatch map involved:

  • Reading optional fields to find the field with the right tag
  • The optional field contained an integer index into a table
  • The index was used to index into a dispatch map table to find a pointer to the actual dispatch map
  • We then followed the pointer to get to the dispatch map.

The advantage of this scheme is smaller working set (MethodTable is smaller), but this assumes the MethodTable has other optional fields (because we still need a pointer to the optional fields). Turns out most MethodTables only need optional fields pointer because of the dispatch map and if we move them to MethodTable, we no longer need an optional field pointer.

This PR simply moves the dispatch map pointer to MethodTable.

I'm seeing another 15 kB saving on BasicMinimalApi. Plus the code looks simpler.

Cc @dotnet/ilc-contrib

Author: MichalStrehovsky
Assignees: -
Labels:

area-NativeAOT-coreclr

Milestone: -

@ghost ghost assigned MichalStrehovsky May 3, 2023
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.

Nice!

@jkotas jkotas merged commit b412b6b into dotnet:main May 3, 2023
@MichalStrehovsky MichalStrehovsky deleted the dispatchmapt branch May 3, 2023 20:57
@ghost ghost locked as resolved and limited conversation to collaborators Jun 3, 2023
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.

2 participants