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 TypeReference.ToObject to managed (CoreCLR) #70055

Merged
merged 14 commits into from
Jun 2, 2022

Conversation

Sergio0694
Copy link
Contributor

@Sergio0694 Sergio0694 commented May 31, 2022

Follow up to #69940

Overview

This PR moves TypedReference.ToObject to managed in CoreCLR.
I've only added an FCall to handle a slow path that's only ever taken when T is a function pointer.
This is to avoid having to port a whole lot of additional VM code to C# as well.
All other paths, both for reference types and value types (using the new RuntimeHelpers.Box API) are managed now.

Benchmarks

Method Branch Mean Error StdDev Ratio
ToObject_String PR 1.944 ns 0.0268 ns 0.0250 ns 0.48
ToObject_String main 4.087 ns 0.0571 ns 0.0534 ns 1.00
ToObject_Int PR 41.241 ns 0.2446 ns 0.2042 ns 0.93
ToObject_Int main 44.195 ns 0.4417 ns 0.3916 ns 1.00
ToObject_FnPointer PR 43.501 ns 0.3535 ns 0.3133 ns 0.92
ToObject_FnPointer main 47.152 ns 0.5428 ns 0.5077 ns 1.00
Benchmark code (click to expand):
using System.Reflection;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Configs;
using BenchmarkDotNet.Running;

BenchmarkSwitcher.FromAssembly(Assembly.GetExecutingAssembly()).Run(args);

[DisassemblyDiagnoser]
[GroupBenchmarksBy(BenchmarkLogicalGroupRule.ByCategory)]
public unsafe class TypedReferenceBenchmark
{
    public string text = "Hello world";
    public int number = 42;
    public delegate*<int, float, string> pointer = (delegate*<int, float, string>)(void*)(nuint)0xDEADBEEF;

    [Benchmark]
    [BenchmarkCategory("REF_TYPE")]
    public object? ToObject_String()
    {
        return TypedReference.ToObject(__makeref(text));
    }

    [Benchmark]
    [BenchmarkCategory("VALUE_TYPE")]
    public object? ToObject_Int()
    {
        return TypedReference.ToObject(__makeref(number));
    }

    [Benchmark]
    [BenchmarkCategory("VALUE_TYPE")]
    public object? ToObject_FnPointer()
    {
        return TypedReference.ToObject(__makeref(pointer));
    }
}

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label May 31, 2022
@ghost
Copy link

ghost commented May 31, 2022

Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

Issue Details

Follow up to #69940

Overview

This PR moves TypedReference.ToObject to managed in CoreCLR.
I've only added an FCall to handle a slow path that's only ever taken when T is a function pointer.
This is to avoid having to port a whole lot of additional VM code to C# as well.
All other paths, both for reference types and value types (using the new RuntimeHelpers.Box API) are managed now.

API additions (CoreCLR-only)

namespace System
{
    public unsafe struct RuntimeTypeHandle
    {
        [MethodImpl(MethodImplOptions.InternalCall)]
        internal static extern unsafe MethodTable* GetElementTypeMethodTable(CorElementType elementType);
    }
}

namespace System.Runtime.CompilerServices
{
    internal unsafe struct TypeHandle
    {
        public bool IsNull { get; }

        public TypeDesc* AsTypeDesc();
        public MethodTable* GetMethodTable();
    }

    internal unsafe struct TypeDesc
    {
        private readonly uint TypeAndFlags;

        public MethodTable* GetMethodTable();
        public CorElementType GetInternalCorElementType();
    }

    internal unsafe struct ParamTypeDesc
    {
        public readonly uint TypeAndFlags;
        public readonly MethodTable* m_TemplateMT;
        public readonly TypeHandle m_Arg;
        public readonly void* m_hExposedClassObject;

        public MethodTable* GetTemplateMethodTableInternal();
    }
}

Benchmarks

Method Branch Mean Error StdDev Ratio
ToObject_String main 3.8621 ns 0.1293 ns 0.1770 ns 1.00
ToObject_String PR 0.9941 ns 0.0115 ns 0.0107 ns 0.25
ToObject_Int main 44.6062 ns 0.2902 ns 0.4432 ns 1.00
ToObject_Int PR 41.7305 ns 0.4657 ns 0.3889 ns 0.94
Benchmark code (click to expand):
using System.Reflection;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Configs;
using BenchmarkDotNet.Running;

BenchmarkSwitcher.FromAssembly(Assembly.GetExecutingAssembly()).Run(args);

[DisassemblyDiagnoser]
[GroupBenchmarksBy(BenchmarkLogicalGroupRule.ByCategory)]
public class ArrayGetValueBenchmark
{
    private string text = "Hello world";
    private int number = 42;

    [Benchmark]
    [BenchmarkCategory("REF_TYPE")]
    public object? ToObject_String()
    {
        return TypedReference.ToObject(__makeref(text));
    }

    [Benchmark]
    [BenchmarkCategory("VALUE_TYPE")]
    public object? ToObject_Int()
    {
        return TypedReference.ToObject(__makeref(number));
    }
}
Author: Sergio0694
Assignees: -
Labels:

area-System.Reflection

Milestone: -

@Sergio0694 Sergio0694 force-pushed the managed-typed-reference-to-object branch from 871e46c to 2e8db94 Compare June 1, 2022 11:58
@jkotas
Copy link
Member

jkotas commented Jun 1, 2022

The new tests are failing on Mono. Open an issue on it and disable the tests against it.

@Sergio0694
Copy link
Contributor Author

Looks like the tests are not failing, but just can't even compile on Mono 🤔

"/tmp/helix/working/A9980937/p/build/apple/AppleApp.targets(104,5): error : Precompiling failed for /tmp/helix/working/A9980937/w/B32B097C/e/publish/System.Runtime.Tests.dll. [/private/tmp/helix/working/A9980937/w/B32B097C/e/publish/ProxyProjectForAOTOnHelix.proj]
/tmp/helix/working/A9980937/p/build/apple/AppleApp.targets(104,5): error : Mono Ahead of Time compiler - compiling assembly /private/tmp/helix/working/A9980937/w/B32B097C/e/publish/System.Runtime.Tests.dll [/private/tmp/helix/working/A9980937/w/B32B097C/e/publish/ProxyProjectForAOTOnHelix.proj]
/tmp/helix/working/A9980937/p/build/apple/AppleApp.targets(104,5): error : AOTID 2E205542-C4E2-0761-A6DA-F8F7672DD714 [/private/tmp/helix/working/A9980937/w/B32B097C/e/publish/ProxyProjectForAOTOnHelix.proj]
/tmp/helix/working/A9980937/p/build/apple/AppleApp.targets(104,5): error : * Assertion at /Users/runner/work/1/s/src/mono/mono/mini/aot-compiler.c:3406, condition `m_class_get_rank (klass) > 0' not met [/private/tmp/helix/working/A9980937/w/B32B097C/e/publish/ProxyProjectForAOTOnHelix.proj]"

So [SkipOnMono] didn't do the trick. I'm not seeing eg. CORECLR defined in the tests, is there a recommended way or a specific compile constant to just remove some tests entirely on a specific runtime so that they won't even be compiled?

@jkotas
Copy link
Member

jkotas commented Jun 1, 2022

[ActiveIssue("https://github.com/dotnet/runtime/issues/...", TestRuntimes.Mono)]

@jkotas
Copy link
Member

jkotas commented Jun 1, 2022

If it is causing the Mono AOT compiler the crash, we do not have a good way to deal with it today. I guess you can comment out the test for now.

@Sergio0694
Copy link
Contributor Author

Ah, gotcha. Will do that, thank you! 🙂

@jkotas
Copy link
Member

jkotas commented Jun 2, 2022

Failures are known issues #69125 and #69192

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!

@jkotas jkotas merged commit 59a3093 into dotnet:main Jun 2, 2022
@Sergio0694 Sergio0694 deleted the managed-typed-reference-to-object branch June 2, 2022 15:09
@ghost ghost locked as resolved and limited conversation to collaborators Jul 2, 2022
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.

4 participants