Add IL Emit support for MethodInfo.Invoke() and friends #69575

steveharter · 2022-05-19T20:10:55Z

Brings back the original commit from #67917 plus a new commit to prevent a missing stack frame that blocked SDK integration thus causing the original commit to be reverted.

Testing performed with these changes:

Ran Microsoft.NET.Sdk.Razor.Tests in the SDK's integration tests locally which were previously failing in some cases due to chained constructors calling Assembly.GetCallingAssembly` to find the calling test class assembly.
Ran System.Diagnostics.StackTrace.Tests with COMPlus_JitStress=1 plus forcing emit on every invoke. This was also previously failing.
Added a new reflection test that previously failed.

The JIT will no longer inline the target method into the generated IL. A future commit will likely switch the implementation from using call and newobj opcodes to using function pointers (calli opcode plus a call to allocate for constructors). Doing that and passing the method pointer into the generated method will also prevent the JIT from inlining the target method, so in effect this PR prevents a temporary breaking change until we switch to calli.

Fixes #69251

ghost · 2022-05-19T20:11:04Z

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

Issue Details

Brings back the original commit from #67917 plus a new commit to prevent a missing stack frame that blocked SDK integration thus causing the original commit to be reverted.

Author:	steveharter
Assignees:	steveharter
Labels:	`area-System.Reflection`
Milestone:	7.0.0

steveharter · 2022-05-23T13:25:42Z

src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs

@@ -29,7 +29,8 @@ public static unsafe InvokeFunc CreateInvokeDelegate(MethodBase method)
                InvokeStubPrefix + declaringTypeName + method.Name,
                returnType: typeof(object),
                delegateParameters,
-                restrictedSkipVisibility: true);
+                typeof(object).Module, // Use system module to identify our DynamicMethods.


This helps address #69251 by assigning the system module. We could also create our own module with a special name and compare that, which would reduce all chances of naming collision but would require a new module\alloc.

jkotas

Thanks

jakobbotsch · 2022-05-23T18:20:31Z

src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs

+            il.Emit(OpCodes.Call, Methods.NextCallReturnAddress()); // For CallStack reasons, don't inline target method.
+            il.Emit(OpCodes.Pop);


Out of curiosity, did you check codegen/your benchmarks when this is used? Just to verify that there is no significant perf impact by using this (beyond what is expected by not inlining the target call).

FWIW, in the current JIT I believe this will actually end up suppressing inlining for the remainder of the IL it sees. That shouldn't be too bad for common cases although I can see that it may have some impact on pointer/by-ref returns below.

FWIW, in the current JIT I believe this will actually end up suppressing inlining for the remainder of the IL it sees

For my edification, what is "this" in the above statement?

System.StubHelpers.StubHelpers.NextCallReturnAddress() is an intrinsic used to implement tailcalls. It has the side effect of guaranteeing that the next call-producing IL instruction will not be inlined by the JIT, so I suggested to @steveharter to use it in #69154.

Cool, thanks.

did you check codegen/your benchmarks when this is used

There was perhaps a slight regression, but still within the margin of error. I'm not too concerned since we'd want to switch to use calli anyway and remove the call to the intrisic.

Just doing a single run and picking the most canonical benchmark:

previous | Method | Job | Toolchain | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Gen 0 | Allocated | Alloc Ratio | |----------------------------------------------------------------------------- |----------- |----------------------- |-----------:|-----------:|-----------:|-----------:|-----------:|-----------:|------:|--------:|-------:|----------:|------------:| | StaticMethod4_int_string_struct_class | Job-AEFDZT | \main\corerun.exe | 200.804 ns | 3.3102 ns | 2.7642 ns | 200.939 ns | 194.044 ns | 205.150 ns | 3.22 | 0.07 | 0.0146 | 160 B | 1.00 | | StaticMethod4_int_string_struct_class | Job-VAFQFP | \newinvoke\corerun.exe | 62.374 ns | 0.8406 ns | 0.7452 ns | 62.538 ns | 61.110 ns | 63.808 ns | 1.00 | 0.00 | 0.0151 | 160 B | 1.00 current | Method | Job | Toolchain | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Gen 0 | Allocated | Alloc Ratio | |----------------------------------------------------------------------------- |----------- |------------------------ |-----------:|----------:|----------:|-----------:|-----------:|-----------:|------:|--------:|-------:|----------:|------------:| | StaticMethod4_int_string_struct_class | Job-WQJSNV | \main\corerun.exe | 200.456 ns | 1.4665 ns | 1.3717 ns | 200.110 ns | 197.585 ns | 202.696 ns | 3.12 | 0.03 | 0.0145 | 160 B | 1.00 | | StaticMethod4_int_string_struct_class | Job-IAUHXD | \newinvoke3\corerun.exe | 64.245 ns | 0.4365 ns | 0.3869 ns | 64.281 ns | 63.756 ns | 65.193 ns | 1.00 | 0.00 | 0.0151 | 160 B |

it went from a ratio of 3.22 to 3.12

ghost · 2022-05-24T16:31:01Z

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

ericstj · 2022-05-24T16:33:19Z

Let's consider this "breaking" as a functional breaking change. That way we can get some docs that let folks know about the types of behavior changes they might see as a result of this change. -- Rereading your mitigations above, feel free to remove the breaking change tag if you feel that this is less breaking in its current form.

AndyAyersMS · 2022-05-27T02:11:41Z

Improvements seen on ARM64 perf.

Also seems like the auto-filer may be repeatedly filing for the same issue. cc @DrewScoggins

steveharter · 2022-06-09T15:50:53Z

@sebastienros have you seen any TechEmpower changes from this? Basically scenarios that use reflection to invoke members. Thanks

sebastienros · 2022-06-09T15:55:44Z

@steveharter Nothing visible on the charts or caught by the bot. In the future ping me when the PR is still open and if you have a local build I can show you how to check if there is an impact.

ericstj · 2022-06-09T18:50:32Z

@sebastienros do you have any other ASP.NET benchmarks that we could check? I would hope that tech-empower doesn't have reflection invoke on the hot path but perhaps other ASP.NET scenarios do.

sebastienros · 2022-06-09T18:53:48Z

@ericstj I looked at MVC scenarios actually because this is where we should see reflection the most. And nothing special shows up around the date it was merged. There is an MVC page in the dashboard with different scenarios all using MVC.

steveharter · 2022-09-26T19:59:19Z

Removing "breaking change" tags since:

The "tailcall" issue was fixed without a breaking change. Originally, the thinking was that a breaking change may be necessary where the breaking change would have been to assume an invoked method may be inlined thus causing Assembly.GetCallingAssembly() to behave differently. This PR preserved the functionality of Assembly.GetCallingAssembly() without a breaking change.
Although stack frames may be different now within an invoked method, that does not really raise to the level of a breaking change. See also Consider hiding stack frames when using Invoke #68923 for future changes here where we may try to hide more reflection frames.
Exception breaking changes due to the previous refactoring is documented in [Breaking change]: exceptions thrown by reflection Invoke() APIs have changed docs#29199

Original commit

8acdb4e

steveharter added the area-System.Reflection label May 19, 2022

steveharter added this to the 7.0.0 milestone May 19, 2022

steveharter self-assigned this May 19, 2022

steveharter force-pushed the EmitInvoke2 branch 2 times, most recently from c6f05f8 to f383835 Compare May 20, 2022 20:28

runfoapp bot mentioned this pull request May 20, 2022

Failing test system.tests.gcextendedtests.getallocatedbytesforcurrentthread #65183

Closed

steveharter force-pushed the EmitInvoke2 branch 3 times, most recently from 4d52247 to 8899aa0 Compare May 21, 2022 14:57

Avoid missing stack frame by preventing inlined target method

2cb349e

steveharter force-pushed the EmitInvoke2 branch from 8899aa0 to 2cb349e Compare May 22, 2022 23:24

steveharter requested review from jkotas, AaronRobinsonMSFT and buyaa-n May 23, 2022 13:20

steveharter marked this pull request as ready for review May 23, 2022 13:20

steveharter requested a review from marek-safar as a code owner May 23, 2022 13:20

steveharter commented May 23, 2022

View reviewed changes

jkotas approved these changes May 23, 2022

View reviewed changes

steveharter merged commit 51df356 into dotnet:main May 23, 2022

steveharter deleted the EmitInvoke2 branch May 23, 2022 17:51

jakobbotsch reviewed May 23, 2022

View reviewed changes

jakobbotsch mentioned this pull request May 23, 2022

libraries-jitstress failure in System.Diagnostics.Tests.StackFrameTests.Ctor_FNeedFileInfo #69154

Closed

steveharter mentioned this pull request May 24, 2022

Remove dependency on Assembly.GetCallingAssembly dotnet/sdk#25494

Closed

ericstj added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label May 24, 2022

ghost added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label May 24, 2022

jonathanpeppers mentioned this pull request Jun 23, 2022

RegexOptions.Compiled impacts Android startup #71007

Closed

ghost locked as resolved and limited conversation to collaborators Jul 9, 2022

steveharter removed needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. labels Sep 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add IL Emit support for MethodInfo.Invoke() and friends #69575

Add IL Emit support for MethodInfo.Invoke() and friends #69575

steveharter commented May 19, 2022 •

edited

Loading

ghost commented May 19, 2022

steveharter May 23, 2022

jkotas left a comment

jakobbotsch May 23, 2022 •

edited

Loading

stephentoub May 23, 2022

jakobbotsch May 23, 2022

stephentoub May 23, 2022

steveharter May 24, 2022 •

edited

Loading

ghost commented May 24, 2022

ericstj commented May 24, 2022 •

edited

Loading

AndyAyersMS commented May 27, 2022 •

edited

Loading

steveharter commented Jun 9, 2022

sebastienros commented Jun 9, 2022 •

edited

Loading

ericstj commented Jun 9, 2022

sebastienros commented Jun 9, 2022

steveharter commented Sep 26, 2022 •

edited

Loading

		il.Emit(OpCodes.Call, Methods.NextCallReturnAddress()); // For CallStack reasons, don't inline target method.
		il.Emit(OpCodes.Pop);

Add IL Emit support for MethodInfo.Invoke() and friends #69575

Add IL Emit support for MethodInfo.Invoke() and friends #69575

Conversation

steveharter commented May 19, 2022 • edited Loading

ghost commented May 19, 2022

steveharter May 23, 2022

Choose a reason for hiding this comment

jkotas left a comment

Choose a reason for hiding this comment

jakobbotsch May 23, 2022 • edited Loading

Choose a reason for hiding this comment

stephentoub May 23, 2022

Choose a reason for hiding this comment

jakobbotsch May 23, 2022

Choose a reason for hiding this comment

stephentoub May 23, 2022

Choose a reason for hiding this comment

steveharter May 24, 2022 • edited Loading

Choose a reason for hiding this comment

ghost commented May 24, 2022

ericstj commented May 24, 2022 • edited Loading

AndyAyersMS commented May 27, 2022 • edited Loading

steveharter commented Jun 9, 2022

sebastienros commented Jun 9, 2022 • edited Loading

ericstj commented Jun 9, 2022

sebastienros commented Jun 9, 2022

steveharter commented Sep 26, 2022 • edited Loading

steveharter commented May 19, 2022 •

edited

Loading

jakobbotsch May 23, 2022 •

edited

Loading

steveharter May 24, 2022 •

edited

Loading

ericstj commented May 24, 2022 •

edited

Loading

AndyAyersMS commented May 27, 2022 •

edited

Loading

sebastienros commented Jun 9, 2022 •

edited

Loading

steveharter commented Sep 26, 2022 •

edited

Loading