Skip to content

Conversation

@AustinWise
Copy link
Contributor

This has a similar structure to FailFast as converted in #98908.

Contributes to #95695

This has a similar structure to FailFast as converted in dotnet#98908.

Contributes to dotnet#95695
@ghost ghost added the area-VM-coreclr label Mar 16, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Mar 16, 2024
Co-authored-by: Jan Kotas <[email protected]>
@jkotas
Copy link
Member

jkotas commented Mar 16, 2024

I see about 6% perf regression in MethodBase.GetCurrentMethod with this change (Win x64).

using System.Reflection;
using System.Diagnostics;

for(;;)
{
    Stopwatch sw = new Stopwatch();
    sw.Start();
    for (int i = 0; i < 1000000; i++) MethodBase.GetCurrentMethod();
    Console.WriteLine(sw.ElapsedMilliseconds.ToString());
}

The implementation of MethodBase.GetCurrentMethod comes with a lot of unnecessary ceremony. Eliminating the ceremony should fix the regression - it can be just:

        public static MethodBase? GetCurrentMethod()
        {
            StackCrawlMark stackMark = StackCrawlMark.LookForMyCaller;
            RuntimeMethodHandleInternal methodHandle = InternalGetCurrentMethod(new StackCrawlMarkHandle(ref stackMark));
            return methodHandle.IsNullHandle() ? null : RuntimeType.GetMethodBase(null, methodHandle);
        }

@AustinWise
Copy link
Contributor Author

I was able to reproduce the regression in a benchmark. Interestingly, it appears that something between .NET 9 Preview 2 and the merge base sped things up:

dotnet/runtime Mean Error StdDev
.NET 9 Preview 2 698.8 ns 2.27 ns 2.12 ns
This PR 639.9 ns 3.62 ns 3.38 ns
Merge Base 571.2 ns 2.98 ns 2.79 ns

Anyways, I'll mark this as draft while I fix the regression.

@AustinWise AustinWise marked this pull request as draft March 17, 2024 17:28
Co-authored-by: Jan Kotas <[email protected]>
@AustinWise AustinWise marked this pull request as ready for review March 17, 2024 18:42
@AustinWise
Copy link
Contributor Author

AustinWise commented Mar 17, 2024

@jkotas Thanks for the tip, this PR is now a performance improvement. Benchmark results of MethodBase.GetCurrentMethod:

code Mean Error StdDev
this PR 480.5 ns 6.14 ns 5.13 ns
merge base 571.2 ns 2.98 ns 2.79 ns
.NET 9 preview 2 698.8 ns 2.27 ns 2.12 ns

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 0935105 into dotnet:main Mar 17, 2024
@AustinWise AustinWise deleted the austin/GetCurrentMethod branch March 17, 2024 22:47
@github-actions github-actions bot locked and limited conversation to collaborators Apr 17, 2024
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.

2 participants