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

Look up Queryable operator MethodInfos without MakeGenericMethod #79717

Merged
merged 1 commit into from
Dec 16, 2022

Conversation

roji
Copy link
Member

@roji roji commented Dec 15, 2022

Replaces MethodInfo lookup for Queryable LINQ operators to stop using MakeGenericMethod, making them NativeAOT-compatible (and slightly faster).

The current code caches the generic method definition and calls MakeGenericMethod every time:

[RequiresDynamicCode("Calls System.Reflection.MethodInfo.MakeGenericMethod(params Type[])")]
public static MethodInfo Where_TSource_2(Type TSource) =>
    (s_Where_TSource_2 ??= new Func<IQueryable<object>, Expression<Func<object, bool>>, IQueryable<object>>(Queryable.Where).GetMethodInfo().GetGenericMethodDefinition())
    .MakeGenericMethod(TSource);

This PR replaces that with the following:

new Func<IQueryable<TSource>, Expression<Func<TSource, bool>>, IQueryable<TSource>>(Where).Method

There's no more caching, but the new implementation is slightly faster than MakeGenericMethod:

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
ViaMethodGenericMethodValue 395.5 ns 1.21 ns 0.95 ns 0.0463 - - 200 B
ViaMethodGenericMethodReference 388.1 ns 6.57 ns 5.49 ns 0.0463 - - 200 B
ViaFuncValue 326.8 ns 5.27 ns 4.68 ns 0.0610 - - 264 B
ViaFuncReference 325.6 ns 2.48 ns 2.20 ns 0.0610 - - 264 B

Note the increase in allocations though.

Benchmark code
BenchmarkRunner.Run<Benchmark>();

[MemoryDiagnoser]
public class Benchmark
{
    private static readonly MethodInfo GenericMethodDefinition
        = new Func<IQueryable<object>, Expression<Func<object, bool>>, IQueryable<object>>(Queryable.Where)
            .GetMethodInfo().GetGenericMethodDefinition();

    [Benchmark]
    public MethodInfo ViaMethodGenericMethodValue()
        => GenericMethodDefinition.MakeGenericMethod(typeof(int));

    [Benchmark]
    public MethodInfo ViaMethodGenericMethodReference()
        => GenericMethodDefinition.MakeGenericMethod(typeof(string));

    [Benchmark]
    public MethodInfo ViaFuncValue()
        => new Func<IQueryable<int>, Expression<Func<int, bool>>, IQueryable<int>>(Queryable.Where).Method;

    [Benchmark]
    public MethodInfo ViaFuncReference()
        => new Func<IQueryable<string>, Expression<Func<string, bool>>, IQueryable<string>>(Queryable.Where).Method;
}

Closes #79199

/cc @jkotas @vitek-karas @ajcvickers

For NativeAOT compatibility, better speed.

Closes dotnet#79199
@ghost
Copy link

ghost commented Dec 15, 2022

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

Issue Details

Replaces MethodInfo lookup for Queryable LINQ operators to stop using MakeGenericMethod, making them NativeAOT-compatible (and slightly faster).

The current code caches the generic method definition and calls MakeGenericMethod every time:

[RequiresDynamicCode("Calls System.Reflection.MethodInfo.MakeGenericMethod(params Type[])")]
public static MethodInfo Where_TSource_2(Type TSource) =>
    (s_Where_TSource_2 ??= new Func<IQueryable<object>, Expression<Func<object, bool>>, IQueryable<object>>(Queryable.Where).GetMethodInfo().GetGenericMethodDefinition())
    .MakeGenericMethod(TSource);

This PR replaces that with the following:

new Func<IQueryable<TSource>, Expression<Func<TSource, bool>>, IQueryable<TSource>>(Where).Method

There's no more caching, but the new implementation is slightly faster than MakeGenericMethod:

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
ViaMethodGenericMethodValue 395.5 ns 1.21 ns 0.95 ns 0.0463 - - 200 B
ViaMethodGenericMethodReference 388.1 ns 6.57 ns 5.49 ns 0.0463 - - 200 B
ViaFuncValue 326.8 ns 5.27 ns 4.68 ns 0.0610 - - 264 B
ViaFuncReference 325.6 ns 2.48 ns 2.20 ns 0.0610 - - 264 B

Note the increase in allocations though.

Benchmark code
BenchmarkRunner.Run<Benchmark>();

[MemoryDiagnoser]
public class Benchmark
{
    private static readonly MethodInfo GenericMethodDefinition
        = new Func<IQueryable<object>, Expression<Func<object, bool>>, IQueryable<object>>(Queryable.Where)
            .GetMethodInfo().GetGenericMethodDefinition();

    [Benchmark]
    public MethodInfo ViaMethodGenericMethodValue()
        => GenericMethodDefinition.MakeGenericMethod(typeof(int));

    [Benchmark]
    public MethodInfo ViaMethodGenericMethodReference()
        => GenericMethodDefinition.MakeGenericMethod(typeof(string));

    [Benchmark]
    public MethodInfo ViaFuncValue()
        => new Func<IQueryable<int>, Expression<Func<int, bool>>, IQueryable<int>>(Queryable.Where).Method;

    [Benchmark]
    public MethodInfo ViaFuncReference()
        => new Func<IQueryable<string>, Expression<Func<string, bool>>, IQueryable<string>>(Queryable.Where).Method;
}

Closes #79199

/cc @jkotas @vitek-karas @ajcvickers

Author: roji
Assignees: -
Labels:

area-System.Linq

Milestone: -

@jkotas
Copy link
Member

jkotas commented Dec 15, 2022

cc @MichalStrehovsky

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. Thank you!

@MichalStrehovsky
Copy link
Member

Nice!

Cc @jaredpar if there's an issue where we track use cases for a methodof operator. The ones I could find were all closed.

@roji roji merged commit 76e2be8 into dotnet:main Dec 16, 2022
@roji roji deleted the QueryableOperators branch December 16, 2022 09:18
@jaredpar
Copy link
Member

@MichalStrehovsky I don't believe there is an active issue around this. There was a lot of discussion around this around the time that nameof was introduced. The proposed addition was called infoof rather than methodof (allowed for identifying all members, not just methods). It was cut though due to lack of need.

This has come up several times in the following years though. The design is fairly well understood but there has never been the need to push it over the top. I think the way to move this forward is to outline the benefit this would have over the current solution (send it to me). I can add in the other scenarios I'm aware of and try to get an issue filed.

@ghost ghost locked as resolved and limited conversation to collaborators Jan 26, 2023
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.

Make LINQ providers (Queryable) compatible with NativeAOT
4 participants