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

Investigate benefits of using UnsafeAccessorAttribute in EF Core #32656

Closed
ilmax opened this issue Dec 21, 2023 · 3 comments
Closed

Investigate benefits of using UnsafeAccessorAttribute in EF Core #32656

ilmax opened this issue Dec 21, 2023 · 3 comments

Comments

@ilmax
Copy link
Contributor

ilmax commented Dec 21, 2023

Since I couldn't find an issue tracking this, I thought about opening one.

Now in net8.0 there's this new attribute that promises to make reflection fast so I was wondering if this can be used to replace some part of EF that uses reflection nowadays. I was thinking about the part that sets and reads values on the current entity (i.e. ClrPropertySetter & Friends).

I made a simple benchmark to check how faster UnsafeAccessorAttribute is compared to reflection and this is the result on my machine:

// * Summary *

BenchmarkDotNet v0.13.9+228a464e8be6c580ad9408e98f18813f6407fb5a, Windows 11 (10.0.22621.2715/22H2/2022Update/SunValley2)
11th Gen Intel Core i7-1165G7 2.80GHz, 1 CPU, 8 logical and 4 physical cores
.NET SDK 8.0.100
  [Host]   : .NET 8.0.0 (8.0.23.53103), X64 RyuJIT AVX2
  .NET 8.0 : .NET 8.0.0 (8.0.23.53103), X64 RyuJIT AVX2

Job=.NET 8.0  Runtime=.NET 8.0

| Method                          | Mean       | Error     | StdDev    | Median     | Ratio | RatioSD |
|-------------------------------- |-----------:|----------:|----------:|-----------:|------:|--------:|
| GetProperty_Reflection          | 39.8169 ns | 0.9062 ns | 2.6292 ns | 39.6347 ns | 3.388 |    0.30 |
| GetProperty_ReflectionWithCache | 11.9320 ns | 0.2669 ns | 0.6646 ns | 11.8401 ns | 1.000 |    0.00 |
| GetProperty_UnsafeAccessor      |  0.0436 ns | 0.0228 ns | 0.0500 ns |  0.0306 ns | 0.004 |    0.00 |
| GetProperty_DirectAccess        |  0.0144 ns | 0.0142 ns | 0.0300 ns |  0.0000 ns | 0.001 |    0.00 |

// * Warnings *
MultimodalDistribution
  GetPropertyBenchmarks.GetProperty_ReflectionWithCache: .NET 8.0 -> It seems that the distribution can have several modes (mValue = 2.95)
ZeroMeasurement
  GetPropertyBenchmarks.GetProperty_UnsafeAccessor: .NET 8.0 -> The method duration is indistinguishable from the empty method duration
  GetPropertyBenchmarks.GetProperty_DirectAccess: .NET 8.0   -> The method duration is indistinguishable from the empty method duration


benchmark code
[SimpleJob(BenchmarkDotNet.Jobs.RuntimeMoniker.Net80)]
public class GetPropertyBenchmarks
{
    static readonly Foo _instance = new();

    [UnsafeAccessor(UnsafeAccessorKind.Method, Name = "get_PublicProperty")]
    extern static int GetPublicProperty(Foo @this);

    static readonly MethodInfo _publicProperty = typeof(Foo)
        .GetProperty(nameof(Foo.PublicProperty), BindingFlags.Public | BindingFlags.Instance)
        .GetGetMethod();

    [Benchmark]
    public int GetProperty_Reflection() => (int)typeof(Foo)
        .GetProperty(nameof(Foo.PublicProperty), BindingFlags.Public | BindingFlags.Instance)
        .GetGetMethod()
        .Invoke(_instance, []);

    [Benchmark(Baseline = true)]
    public int GetProperty_ReflectionWithCache() => (int)_publicProperty.Invoke(_instance, []);

    [Benchmark]
    public int GetProperty_UnsafeAccessor() => GetPublicProperty(_instance);

    [Benchmark]
    public int GetProperty_DirectAccess() => _instance.PublicProperty;
}


public class Foo
{
    private int _privateField;
    public int PublicProperty { get; set; } = 42;
}

UnsafeAccessorAttribute requires to define a extern static method with an attribute that defines the target (Field/Method/Constructor) and the name. In order to be used, it requires some sort of code generation.

I was thinking that this may fit well with the Compiled Model functionality where a faster alternative of the classes used to get and set entity values can be created.

This hypothetical class can be generated during model compilation and can wrap an actual entity and can make getting/setting properties/fields faster than the cached reflection based approach.

Please let me know what you think about this and, if this is a duplicate issue, please close it.

@roji
Copy link
Member

roji commented Dec 21, 2023

@ilmax we're well aware of UnsafeAccessorAttribute (EF is one of the reasons it was introduced in the first place). The main idea is to use this in generated C# code to for precompiled queries (#25009), which will be a necessary step for supporting NativeAOT (but not only).

@ilmax
Copy link
Contributor Author

ilmax commented Dec 21, 2023

@roji I was sure something that big wouldn't go unnoticed by the EF team 🙂

I'm super curious to see what the next version will bring on the table then!

@roji
Copy link
Member

roji commented Dec 21, 2023

Duplicate of #25009

@roji roji marked this as a duplicate of #25009 Dec 21, 2023
@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Jan 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants