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

Simplify MethodDesc::GetLoaderModule #106212

Merged
merged 1 commit into from
Aug 13, 2024

Conversation

davidwrighton
Copy link
Member

Turn MethodDesc::GetLoaderModule and MethodDesc::GetLoaderAllocator into O(1) operations

  • Today these are dependent on calling into ClassLoader::ComputerLoaderModule, which can be a very complex operation, and always requires walking all of the method's instantiation arguments.
  • This change makes puts a pointer to the Loader Module onto the MethodDescChunk and removes the need for running the potentially complex ComputeLoaderModule algorithm.

…tor` into O(1) operations

- Today these are dependent on calling into `ClassLoader::ComputerLoaderModule`, which can be a very complex operation, and always requires walking all of the method's instantiation arguments.
- This change makes puts a pointer to the Loader Module onto the `MethodDescChunk` and removes the need for running the potentially complex `ComputeLoaderModule` algorithm.
Copy link
Contributor

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

@lambdageek
Copy link
Member

Should we do this for ComputeModuleWorkerForFunctionPointer, too?

@davidwrighton
Copy link
Member Author

@lambdageek, yes, we should just add a field on FnPtrTypeDesc for the loader module. Function pointers are so rare, that adding a field won't cost us any significant amount of memory.

@davidwrighton
Copy link
Member Author

@lambdageek Although, I'd like to make that change as part of a separate PR, as this one I think is good enough for the MethodDesc scenario.

@davidwrighton davidwrighton marked this pull request as ready for review August 12, 2024 17:18
@lambdageek
Copy link
Member

Although, I'd like to make that change as part of a separate PR

Sure, sounds good

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

@davidwrighton davidwrighton merged commit d228238 into dotnet:main Aug 13, 2024
90 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 12, 2024
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