-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Add IL Emit support for MethodInfo.Invoke() and friends #69575
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
skipVisibility: true); | ||
|
||
ILGenerator il = dm.GetILGenerator(); | ||
|
||
|
@@ -64,6 +65,11 @@ public static unsafe InvokeFunc CreateInvokeDelegate(MethodBase method) | |
} | ||
|
||
// Invoke the method. | ||
#if !MONO | ||
il.Emit(OpCodes.Call, Methods.NextCallReturnAddress()); // For CallStack reasons, don't inline target method. | ||
il.Emit(OpCodes.Pop); | ||
Comment on lines
+69
to
+70
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For my edification, what is "this" in the above statement? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool, thanks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 Just doing a single run and picking the most canonical benchmark:
it went from a ratio of |
||
#endif | ||
|
||
if (emitNew) | ||
{ | ||
il.Emit(OpCodes.Newobj, (ConstructorInfo)method); | ||
|
@@ -182,6 +188,13 @@ public static MethodInfo Pointer_Box() => | |
public static MethodInfo Type_GetTypeFromHandle() => | ||
s_Type_GetTypeFromHandle ?? | ||
(s_Type_GetTypeFromHandle = typeof(Type).GetMethod(nameof(Type.GetTypeFromHandle), new[] { typeof(RuntimeTypeHandle) })!); | ||
|
||
#if !MONO | ||
private static MethodInfo? s_NextCallReturnAddress; | ||
public static MethodInfo NextCallReturnAddress() => | ||
s_NextCallReturnAddress ?? | ||
(s_NextCallReturnAddress = typeof(System.StubHelpers.StubHelpers).GetMethod(nameof(System.StubHelpers.StubHelpers.NextCallReturnAddress), BindingFlags.NonPublic | BindingFlags.Static)!); | ||
#endif | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System.Runtime.CompilerServices; | ||
|
||
namespace System.Reflection.TestAssembly | ||
{ | ||
public sealed class ClassToInvoke | ||
{ | ||
[MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
public static Assembly CallMe_AggressiveInlining() | ||
{ | ||
return CallMeActual(); | ||
} | ||
|
||
[MethodImpl(MethodImplOptions.NoInlining)] | ||
private static Assembly CallMeActual() | ||
{ | ||
return Assembly.GetCallingAssembly(); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,10 @@ | ||
<Project Sdk="Microsoft.NET.Sdk"> | ||
<Project Sdk="Microsoft.NET.Sdk"> | ||
<PropertyGroup> | ||
<AutoGenerateBindingRedirects>true</AutoGenerateBindingRedirects> | ||
<TargetFramework>netstandard2.0</TargetFramework> | ||
</PropertyGroup> | ||
<ItemGroup> | ||
<Compile Include="ClassToInvoke.cs" /> | ||
<Compile Include="TestAssembly.cs" /> | ||
</ItemGroup> | ||
</Project> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.