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

Remove Helper Method Frames (HMF) from Reflection #110211

Merged
merged 9 commits into from
Dec 3, 2024

Conversation

AaronRobinsonMSFT
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT commented Nov 27, 2024

Convert RuntimeMethodHandle::ReboxToNullable() to managed.
Convert RuntimeMethodHandle::ReboxFromNullable() to managed.
Convert RuntimeMethodHandle::InvokeMethod() to QCall.

Copy link
Contributor

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

@am11
Copy link
Member

am11 commented Nov 27, 2024

This one looks related:

  Starting:    System.Runtime.ReflectionInvokeInterpreted.Tests (parallel test collections = on [2 threads], stop on fail = off)
    System.Reflection.Tests.InvokeInterpretedTests.VerifyInvokeIsUsingInterpreter_Method [FAIL]
      Assert.Contains() Failure: Sub-string not found
      String:    "System.Exception: Here\n   at System.Refle"···
      Not found: "System.RuntimeMethodHandle.InvokeMethod"
      Stack Trace:
        /_/src/libraries/Common/tests/System/Reflection/InvokeInterpretedTests.cs(19,0): at System.Reflection.Tests.InvokeInterpretedTests.VerifyInvokeIsUsingInterpreter_Method()
        /_/src/coreclr/System.Private.CoreLib/src/System/Reflection/MethodBaseInvoker.CoreCLR.cs(36,0): at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
        /_/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBaseInvoker.cs(57,0): at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
    System.Reflection.Tests.InvokeInterpretedTests.VerifyInvokeIsUsingInterpreter_Constructor [FAIL]
      Assert.Contains() Failure: Sub-string not found
      String:    "System.Exception: Here\n   at System.Refle"···
      Not found: "System.RuntimeMethodHandle.InvokeMethod"
      Stack Trace:
        /_/src/libraries/Common/tests/System/Reflection/InvokeInterpretedTests.cs(32,0): at System.Reflection.Tests.InvokeInterpretedTests.VerifyInvokeIsUsingInterpreter_Constructor()
        /_/src/coreclr/System.Private.CoreLib/src/System/Reflection/MethodBaseInvoker.CoreCLR.cs(36,0): at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
        /_/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBaseInvoker.cs(57,0): at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
  Finished:    System.Runtime.ReflectionInvokeInterpreted.Tests

@AaronRobinsonMSFT
Copy link
Member Author

@jkotas @steveharter I'm not sure the InvokeInterpretedTests suite should be validating stack frame names of non-user code. This fails when converting the FCall into a QCall, even with DebuggerStepThrough and DebuggerHidden applied. I don't think the helper method should factor into the test. Removing it makes the test pass locally.

Any concern with me changing the test?

@AaronRobinsonMSFT
Copy link
Member Author

See fa675ce for update to the interpreter test.

@AaronRobinsonMSFT
Copy link
Member Author

Upon further reflection it appears the JIT is now inlining function calls and the behavior matches Mono. Removed the special casing of Mono/CoreCLR and the tests all pass.

Copy link
Member

@steveharter steveharter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this noticeably affect performance?

src/coreclr/vm/reflectioninvocation.cpp Show resolved Hide resolved
@steveharter
Copy link
Member

Any concern with me changing the test?

No, go ahead and remove or modify that test.

@AaronRobinsonMSFT
Copy link
Member Author

Failure seems to be #106094

@jkotas
Copy link
Member

jkotas commented Dec 2, 2024

@EgorBot -windows_intel

using System;
using BenchmarkDotNet.Attributes;

public class Bench
{
    Action a = () => { };
    Action b = () => { };

    [Benchmark]
    public Action Combine() => a + b;
}

@jkotas
Copy link
Member

jkotas commented Dec 2, 2024

I am seeing about 30% regression in a simple delegate combine microbenchmark locally. Let's see whether egorbot confirms it. Looks like I was wrong about AllocateNoChecks optimization usefulness. However, I still see about 15% regression even with the last commit deleted, so some of that regression comes from the generalization and passing extra args.

I think the alloc helper needs some perf tweaks to avoid perf regressions in delegate operations. Alternatively, reverting the delegate changes and leaving the allocation helper unification as a problem for future would be fine with me too.

@jkotas
Copy link
Member

jkotas commented Dec 2, 2024

Egorbot measured 1.26x slowdown: EgorBot/runtime-utils#198 (comment)

@AaronRobinsonMSFT
Copy link
Member Author

@EgorBot -windows_intel

using System;
using BenchmarkDotNet.Attributes;

public class Bench
{
    Action a = () => { };
    Action b = () => { };

    [Benchmark]
    public Action Combine() => a + b;
}

@AaronRobinsonMSFT
Copy link
Member Author

I am seeing about 30% regression in a simple delegate combine microbenchmark locally. Let's see whether egorbot confirms it. Looks like I was wrong about AllocateNoChecks optimization usefulness. However, I still see about 15% regression even with the last commit deleted, so some of that regression comes from the generalization and passing extra args.

I think the alloc helper needs some perf tweaks to avoid perf regressions in delegate operations. Alternatively, reverting the delegate changes and leaving the allocation helper unification as a problem for future would be fine with me too.

@jkotas Putting back in the no checks allocation function reduces the regression to noise (< 1 %) - EgorBot/runtime-utils#199

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. Thanks!

@AaronRobinsonMSFT AaronRobinsonMSFT merged commit d592d11 into dotnet:main Dec 3, 2024
125 checks passed
@AaronRobinsonMSFT AaronRobinsonMSFT deleted the hmf_remove06 branch December 3, 2024 21:45
eduardo-vp pushed a commit to eduardo-vp/runtime that referenced this pull request Dec 5, 2024
Create general purpose RuntimeTypeHandle.InternalAlloc() and RuntimeTypeHandle.InternalAllocNoChecks().
Convert RuntimeMethodHandle::ReboxToNullable() to managed.
Convert RuntimeMethodHandle::ReboxFromNullable() to managed.
Convert RuntimeMethodHandle::InvokeMethod() to QCall.
mikelle-rogers pushed a commit to mikelle-rogers/runtime that referenced this pull request Dec 10, 2024
Create general purpose RuntimeTypeHandle.InternalAlloc() and RuntimeTypeHandle.InternalAllocNoChecks().
Convert RuntimeMethodHandle::ReboxToNullable() to managed.
Convert RuntimeMethodHandle::ReboxFromNullable() to managed.
Convert RuntimeMethodHandle::InvokeMethod() to QCall.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants