-
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
Change some delegate FCalls to managed #70000
Conversation
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
Tagging subscribers to this area: @dotnet/area-system-runtime Issue DetailsBenchmark: [UnmanagedCallersOnly]
static void X() { }
private readonly Action _fnptr1 = Marshal.GetDelegateForFunctionPointer<Action>((nint)(delegate* unmanaged<void>)(&X));
private readonly Action _fnptr2 = Marshal.GetDelegateForFunctionPointer<Action>((nint)(delegate* unmanaged<void>)(&X));
private readonly Action _action1 = () => Console.WriteLine("A");
private readonly Action _action2 = () => Console.WriteLine("B");
private readonly Func<int> _actionInt1 = Func;
private readonly Func<int> _actionInt2 = Func;
public static int Func() => 42;
[Benchmark]
public bool UnmanagedEquals() => _fnptr1 == _fnptr2;
[Benchmark]
public Action Combine() => _action1 + _action2;
[Benchmark]
public bool ManagedEquals() => _actionInt1 == _actionInt2;
[Benchmark]
public bool ManagedNotEquals() => _action1 == _action2;
[Benchmark]
public bool TypeMismatch() => _action1.Equals(_actionInt1);
|
src/coreclr/vm/comutilnative.cpp
Outdated
FCIMPL2(FC_BOOL_RET, MethodTableNative::IsEquivalentTo, MethodTable* mta, MethodTable* mtb) | ||
{ | ||
FCALL_CONTRACT; | ||
FC_RETURN_BOOL(mta->IsEquivalentTo(mtb)); |
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.
The contract of IsEquivalentTo_Worker
is GC_TRIGGER, so a helper method frame is still required?
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.
Perhaps GCX_COOP is better suited for these MethodTable helpers? AFAIK, converting these FCalls to QCall (or pure managed) is a general goodness, e.g.
runtime/src/coreclr/vm/qcallentrypoints.cpp
Line 110 in e67e4da
DllImportEntry(RuntimeMethodHandle_GetTypicalMethodDefinition) |
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.
Yes, it would be better to change this to QCall while you are on it.
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.
How to pass MethodTable
in QCall then? Since it's a pointer into unmanaged runtime, it should be safe to directly pass the pointer?
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.
Yes, pass it as pointer
The test failures are #64544 + Helix infrastructure intermittent failure. |
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.
Thank you!
Benchmark: