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 #109996

Merged
merged 12 commits into from
Nov 25, 2024

Conversation

AaronRobinsonMSFT
Copy link
Member

Convert RuntimeMethodHandle.GetDeclaringType() to managed and QCall.
Convert RuntimeTypeHandle.GetElementType() to managed.
Correct nullability in signature and usage.
Convert RuntimeTypeHandle.GetDeclaringType() to managed and QCalls.
Remove GetRuntimeTypeHelper().

@AaronRobinsonMSFT
Copy link
Member Author

@EgorBot -intel -arm64 -perf

using System;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;

BenchmarkRunner.Run<Bench>(args: args);

public class Bench
{
    struct S;

    [Benchmark]
    public Type Nested() => typeof(S).DeclaringType;

    [Benchmark]
    public Type NotNested() => typeof(Bench).DeclaringType;

    [Benchmark]
    public Type RefElementType() => typeof(Bench[]).GetElementType();

    [Benchmark]
    public Type ValueElementType() => typeof(S[]).GetElementType();
}

@AaronRobinsonMSFT
Copy link
Member Author

@EgorBot -x64 -arm64 -profiler

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;

public class Bench
{
    struct S;

    [Benchmark]
    public Type Nested() => typeof(S).DeclaringType;

    [Benchmark]
    public Type NotNested() => typeof(Bench).DeclaringType;

    [Benchmark]
    public Type RefElementType() => typeof(Bench[]).GetElementType();

    [Benchmark]
    public Type ValueElementType() => typeof(S[]).GetElementType();
}

@EgorBo
Copy link
Member

EgorBo commented Nov 20, 2024

@AaronRobinsonMSFT the bot complained that -profiler flag should be used when there are not more than 3 [Benchmark] attributes. I think I should improve error reporting 🙂

@EgorBo
Copy link
Member

EgorBo commented Nov 20, 2024

@EgorBot -x64 -arm64

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;

public class Bench
{
    struct S;

    [Benchmark]
    public Type Nested() => typeof(S).DeclaringType;

    [Benchmark]
    public Type NotNested() => typeof(Bench).DeclaringType;

    [Benchmark]
    public Type RefElementType() => typeof(Bench[]).GetElementType();

    [Benchmark]
    public Type ValueElementType() => typeof(S[]).GetElementType();
}

@AaronRobinsonMSFT
Copy link
Member Author

@EgorBot -x64 -arm64 -profiler

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;

public class Bench
{
    struct S;

    [Benchmark]
    public Type RefElementType() => typeof(Bench[]).GetElementType();

    [Benchmark]
    public Type ValueElementType() => typeof(S[]).GetElementType();
}

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.

Thank you!

@AaronRobinsonMSFT
Copy link
Member Author

@jkotas Looks like removing that IsArray check from GetDeclaringType() is causing linker legs to fail. Any thoughts?

I'll investigate the x86 leg tomorrow.

@jkotas
Copy link
Member

jkotas commented Nov 25, 2024

I'll investigate the x86 leg tomorrow.

This is failing in all PRs: #110127

@jkotas
Copy link
Member

jkotas commented Nov 25, 2024

Looks like removing that IsArray check from GetDeclaringType() is causing linker legs to fail. Any thoughts?

The failures are caused by the linker assuming that instances of RuntimeType and RuntimeAssembly types are never allocated. The linker is replacing checks like if (assembly is RuntimeAssembly) by if (false) that is causing inscrutable failures.

The RuntimeTypeHandle.GetRuntimeTypeFromHandle call under the IsArray check is what makes the linker think that RuntimeType may be allocated. When the call is deleted as side-effect of deleting the IsArray check, the things fall apart. Of course, depending on a random call in the middle of reflection stack from keeping RuntimeType type alive is super fragile.

The problem is caused by the fact that the runtime allocates RuntimeType instances without running constructor. It is ok since the constructor would be no-op anyway, but it is confusing the linker. I think the proper fix is to add an entry for RuntimeType and RuntimeAssembly constructors under #if FOR_ILLINK to corelib.h to signal to the linker that the VM creates instances of these types. Once you do that, the linker tests should be passing with the IsArray check deleted.

@AaronRobinsonMSFT
Copy link
Member Author

/ba-g "Failure is dotnet/dnceng#3879. Not waiting for timeout."

@AaronRobinsonMSFT AaronRobinsonMSFT merged commit aaa7ba8 into dotnet:main Nov 25, 2024
140 of 142 checks passed
@AaronRobinsonMSFT AaronRobinsonMSFT deleted the hmf_remove05 branch November 25, 2024 23:52
mikelle-rogers pushed a commit to mikelle-rogers/runtime that referenced this pull request Dec 10, 2024
Convert RuntimeMethodHandle.GetDeclaringType() to managed and QCall.
Convert RuntimeTypeHandle.GetElementType() to managed.
Correct nullability in signature and usage.
Convert RuntimeTypeHandle.GetDeclaringType() to managed and QCalls.
Remove GetRuntimeTypeHelper().
Root RuntimeAssembly and RuntimeType constructors.
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.

4 participants