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

Remove Helper Method Frames (HMF) from Reflection #110627

Merged
merged 22 commits into from
Dec 17, 2024

Conversation

AaronRobinsonMSFT
Copy link
Member

Convert Signature.GetSignature() to QCall.
Rename Signature.GetSignature() to Signature.Init().
Remove HMF from Signature.GetTypeParameterOffset().
Remove HMF from Signature.GetCallingConventionFromFunctionPointerAtOffset().
Convert Signature.GetCustomModifiersAtOffset() to QCall.
Convert RuntimeTypeHandle.GetDeclaringMethodForGenericParameter() to QCall.
Rename RuntimeTypeHandle.GetDeclaringMethod() to RuntimeTypeHandle.GetDeclaringMethodForGenericParameter().
Convert RuntimeTypeHandle.CanCastTo() to slow/fast paths using FCall/QCall.
Remove HMF from RuntimeMethodHandle::GetMethodDef().
Convert RuntimeMethodHandle.GetStubIfNeeded() to fast/slow paths using FCall/QCall.
Convert RuntimeTypeHandle.SatisfiesConstraints() to QCall.
Convert RuntimeMethodHandle.GetMethodBody() to QCall.

Copy link
Contributor

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

@am11
Copy link
Member

am11 commented Dec 12, 2024

S.D.Process test failure is unrelated #110643 (intermittently failing on other PRs as well).

@jkotas jkotas requested a review from Copilot December 14, 2024 14:06
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 otherwise. Thanks!

@AaronRobinsonMSFT
Copy link
Member Author

@jkotas I did some consolidation around TypeHandle.CanCastTo, please take another look.

@jkotas
Copy link
Member

jkotas commented Dec 16, 2024

I did some consolidation around TypeHandle.CanCastTo, please take another look.

I think we can go a bit further and merge the QCALL for the regular no-cache version and reflection no-cache version by moving the Nullable special case to the managed side. It should make the casting impl details more centralized. I left a suggestion for what it may look like.

@AaronRobinsonMSFT
Copy link
Member Author

I did some consolidation around TypeHandle.CanCastTo, please take another look.

I think we can go a bit further and merge the QCALL for the regular no-cache version and reflection no-cache version by moving the Nullable special case to the managed side. It should make the casting impl details more centralized. I left a suggestion for what it may look like.

Good call, simplified a lot.

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
Copy link
Member

jkotas commented Dec 17, 2024

@EgorBot -windows_intel

using System;
using BenchmarkDotNet.Attributes;

public class Bench
{
    Type fromType = typeof(object);
    Type toType = typeof(string);

    [Benchmark]
    public bool IsAssignableFrom() => toType.IsAssignableFrom(fromType);
}

@AaronRobinsonMSFT
Copy link
Member Author

Seems the change is net zero for IsAssignableFrom - EgorBot/runtime-utils#205 (comment).

@AaronRobinsonMSFT AaronRobinsonMSFT merged commit e78ee77 into dotnet:main Dec 17, 2024
139 of 141 checks passed
@AaronRobinsonMSFT AaronRobinsonMSFT deleted the hmf_remove09 branch December 17, 2024 03:09
@jkotas
Copy link
Member

jkotas commented Dec 17, 2024

@EgorBot -windows_intel

using System;
using BenchmarkDotNet.Attributes;

public class Bench
{
    Type fromType = typeof(string);
    Type toType = typeof(object);

    [Benchmark]
    public bool IsAssignableFrom() => toType.IsAssignableFrom(fromType);
}

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

Successfully merging this pull request may close these issues.

3 participants