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

How about MethodBase.GetCurrentMethod() performance #85

Open
jerviscui opened this issue Jun 10, 2021 · 11 comments
Open

How about MethodBase.GetCurrentMethod() performance #85

jerviscui opened this issue Jun 10, 2021 · 11 comments

Comments

@jerviscui
Copy link

I see the weaved dll.

MethodExecutionArgs __var_ = new MethodExecutionArgs();
__var_.Arguments = new object[0];
__var_.Method = MethodBase.GetCurrentMethod();
__var_.Instance = this;
__var_3.AttributeTargetMemberAttributes = new TransactionScopeAttribute();
__var_3.OnEntry(__var_);

__var_.Method use the static method MethodBase.GetCurrentMethod() get current method, this method performance is bad.

Are there any other implementations? Just like typeof(Type).GetMethod() or typeof(this).GetMethod()?

@Ralf1108
Copy link
Collaborator

When profiling these approaches in this system I got:

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

namespace MyBenchmarks
{
    public class GetMethodInfo
    {
        public static MethodBase Cache;
        
        [Benchmark]
        public MethodBase GetCurrentMethod() => MethodBase.GetCurrentMethod();

        [Benchmark]
        public MethodBase TypeOfGetMethod() => typeof(GetMethodInfo).GetMethod(nameof(TypeOfGetMethod)); // have to add parameter types find unique method

        [Benchmark]
        public MethodBase ThisGetTypeGetMethod() => GetType().GetMethod(nameof(ThisGetTypeGetMethod)); // have to add parameter types find unique method

        [Benchmark]
        public MethodBase GetMethodFromHandle()
        {
            if (Cache == null)
                Cache = MethodBase.GetCurrentMethod();

            return MethodBase.GetMethodFromHandle(Cache.MethodHandle, typeof(GetMethodInfo).TypeHandle);
        }
    }

    public class Program
    {
        public static void Main(string[] args)
        {
            var summary = BenchmarkRunner.Run<GetMethodInfo>();
        }
    }
}

// * Summary *

BenchmarkDotNet=v0.13.0, OS=Windows 10.0.18363.1440 (1909/November2019Update/19H2)
Intel Core i7-6700K CPU 4.00GHz (Skylake), 1 CPU, 8 logical and 4 physical cores
[Host] : .NET Framework 4.8 (4.8.4300.0), X86 LegacyJIT
DefaultJob : .NET Framework 4.8 (4.8.4300.0), X86 LegacyJIT

Method Mean Error StdDev
GetCurrentMethod 798.22 ns 15.051 ns 13.343 ns
TypeOfGetMethod 54.08 ns 0.176 ns 0.156 ns
ThisGetTypeGetMethod 55.32 ns 0.293 ns 0.260 ns
GetMethodFromHandle 166.38 ns 1.340 ns 1.119 ns

So you are correct there yould be an improvement.
Do you have a concrete use case where this performance penalty hurts the most?
And we would also be happy to accept a PR :-)

@jerviscui
Copy link
Author

Thank you for your contributions very much!
I'm just starting to learn IL weave.
Hope I can help.

@buvar
Copy link

buvar commented Sep 17, 2021

Any solution for that? I just found the problem yesterday when benchmarking method call with and without this addin. Ten million calls without addin took 12ms, with almost 9000ms. When I added MethodBase.GetCurrentMethod to my "pure" method, the time was similiar to the time with addin. For me would be enough to be able prevent setting MethodExecutionArgs.Method at all as I can handle it myself in Attribute implementation (when needed).

@Ralf1108
Copy link
Collaborator

Ralf1108 commented Oct 18, 2021

I managed to implement storing the method info in static fields to avoid using "Method.GetCurrentMethod".
With this change to overhead of an aspect invocation shrinks from

841,12ns - 56,83ns = 784,29ns (v2.0.139)
to
81,63ns - 56,76ns = 24,87 ns (v2.0.141)

This is a speedup of 30x for aspect method invocation overhead or an reduction of 97% :-)

Here are the benchmark numbers:

v2.0.139

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.19041.1083 (2004/May2020Update/20H1)
Intel Core i7-6700K CPU 4.00GHz (Skylake), 1 CPU, 8 logical and 4 physical cores
.NET SDK=5.0.204
[Host] : .NET 5.0.7 (5.0.721.25508), X64 RyuJIT
DefaultJob : .NET 5.0.7 (5.0.721.25508), X64 RyuJIT

Method Mean Error StdDev
MethodCallWithAspect 841.12 ns 6.404 ns 5.000 ns
MethodCallWithoutAspect 56.83 ns 0.337 ns 0.299 ns

v2.0.141

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.19041.1083 (2004/May2020Update/20H1)
Intel Core i7-6700K CPU 4.00GHz (Skylake), 1 CPU, 8 logical and 4 physical cores
.NET SDK=5.0.204
[Host] : .NET 5.0.7 (5.0.721.25508), X64 RyuJIT
DefaultJob : .NET 5.0.7 (5.0.721.25508), X64 RyuJIT

Method Mean Error StdDev
MethodCallWithAspect 81.63 ns 0.864 ns 0.808 ns
MethodCallWithoutAspect 56.76 ns 0.296 ns 0.277 ns

@Ralf1108
Copy link
Collaborator

The benchmark project is here project

@Ralf1108
Copy link
Collaborator

@jerviscui can you verify the improved performance?

@Ralf1108
Copy link
Collaborator

Currently this optimization is only supported for closed types.
For open types (generics) we can not fetch the method information on compile time because the generics are compiled during runtime.
Maybe we find a workaround for this.

@Ralf1108
Copy link
Collaborator

Support for open generics was implemented in v2.0.144

Overhead for open generic methods is bigger because they can not efficently cached at compile time like closed generic or non-generic methods.
Timings are:

// * Summary *

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.19041.1083 (2004/May2020Update/20H1)
Intel Core i7-6700K CPU 4.00GHz (Skylake), 1 CPU, 8 logical and 4 physical cores
.NET SDK=5.0.204
[Host] : .NET 5.0.7 (5.0.721.25508), X64 RyuJIT
DefaultJob : .NET 5.0.7 (5.0.721.25508), X64 RyuJIT

Method Mean Error StdDev
CallWithoutAspect 133.7 ns 0.15 ns 0.14 ns
CallWithAspect 168.1 ns 1.81 ns 1.60 ns
OpenGenericCallWithoutAspect 136.8 ns 0.21 ns 0.18 ns
OpenGenericCallWithAspect 250.4 ns 1.36 ns 1.27 ns

@jerviscui
Copy link
Author

You are amazing. 👍

I've been so busy with work and family lately.
I'll learn from your code.

@buvar
Copy link

buvar commented Oct 21, 2021

Looks great, thank you, I'll give it a try! Anyway, I still think that for special cases (eg. generics now) it would be fine if calling GetCurrentMethod could be switched off explicitly by decorator/attribute parameter...

@Ralf1108
Copy link
Collaborator

Looks great, thank you, I'll give it a try! Anyway, I still think that for special cases (eg. generics now) it would be fine if calling GetCurrentMethod could be switched off explicitly by decorator/attribute parameter...

So you mean that you want to explicitly disable the method info because it is not used in the aspect itself?
This could also be discovered automatically when inspecting the aspect mehtod body during weaving 👍
Sounds interesting and could further increase aspect invocation performance!

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

No branches or pull requests

3 participants