Instantiating stub for devirtualized default interface method on a generic type #43668
Conversation
Let me know if you have trouble extracting the failing tests from the CI. E.g. here is one: I don't have pointers for you - I've decided the CoreCLR type system is not something I want to be involved in after finishing the default interface methods feature and haven't looked back since. |
Thank you! I've managed to dig through pipelines but I'm struggling to reproduce that failure. ... and it passes 🤯 I see there is setup being done during pipeline builds:
I can't find that file in my repo/artifacts. Could there be some crucial setup steps like setting |
:) Try COMPlus_TieredCompilation=0 |
|
Thank you @MichalStrehovsky ❤️ now I'm able to reproduce and have couple of failing tests under /regeressions dir. And all that issues have linked PR's submitted by you. Reading through related discussions I can feel your PTSD
|
There was a problem hiding this comment.
I'm done with general logic changes. The gist is:
resolveVirtualMethodHelper now returns wrapped MethodDesc for devirtualized default interface method.
interface I<T>
{
string DefaultTypeOf() => typeof(T).Name;
}
class C : I<string>
{
}
public static class Program
{
[MethodImpl(MethodImplOptions.AggressiveOptimization)]
static int Main()
{
var c = new C();
var dcs = ((I<string>)c).DefaultTypeOf();
if (dcs != "String") return 200;
return 100;
}
}For the program above we will get I'1[[System.String]][System.String].DefaultTypeOf from pDerivedMT->GetMethodDescForInterfaceMethod(TypeHandle(pOwnerMT), pBaseMD, FALSE); Which is already an instantiating stub!
Then we going to unwrap it into I'1[[System.__Canon]][System.String].DefaultTypeOf, tell to caller that *requiresInstMethodTableArg = true and do our business in impDevirtualizeCall:
Get the instantiation param from context: GenTree* instParam = gtNewIconEmbClsHndNode(exactClassHandle);. Context here is I<string> part of ((I<string>)c).DefaultTypeOf();. Pass that param, so we will end with:
[000010] I-C-G------- * CALL nullcheck ref I`1[__Canon][System.__Canon].DefaultTypeOf (exactContextHnd=0x00007FF95AE1FE50)
[000009] ------------ this in rcx +--* LCL_VAR ref V00 loc0
[000011] H----------- arg1 \--* CNS_INT(h) long 0x7ff95ae40020 class
^-- I`1[System.String] type handle
and the following assembly would be
IN0002: 00000F call CORINFO_HELP_NEWSFAST
IN0003: 000014 mov rsi, rax
IN0004: 000017 mov rcx, rsi
IN0005: 00001A call System.Object:.ctor():this
IN0006: 00001F mov rcx, rsi
IN0007: 000022 mov rdx, 0x7FF95AE40020 # I`1[System.String] type handle
IN0008: 00002C call I`1[__Canon][System.__Canon]:DefaultTypeOf():System.String:this
src/coreclr/src/tools/Common/JitInterface/ThunkGenerator/ThunkInput.txt
Outdated
Show resolved
Hide resolved
src/coreclr/src/vm/method.hpp
Outdated
| @@ -581,7 +581,7 @@ class MethodDesc | |||
| // | |||
| // RequiresInstMethodDescArg() | |||
| // The method is itself generic and is shared between generic | |||
| // instantiations but is not itself generic. Furthermore | |||
| // instantiations but is not itself generic. WTF LOL. Furthermore | |||
There was a problem hiding this comment.
I forgot to change wording before pushing.
I think we should rephrase that
The method is itself generic and is shared between generic instantiations but is not itself generic
src/coreclr/src/vm/jitinterface.cpp
Outdated
| if (!pDerivedMT->CanCastToInterface(pBaseMT)) | ||
| { | ||
| return nullptr; | ||
| } |
There was a problem hiding this comment.
I've delayed this check because it forbids several devirt oportunities:
interface I<T>
{
string DefaultTypeOf() => typeof(T).Name;
}
class C : I<string>
{
}
public static class Program
{
[MethodImpl(MethodImplOptions.AggressiveOptimization)]
static int Main()
{
var c = new C();
var dcs = ((I<string>)c).DefaultTypeOf();
if (dcs != "String") return 200;
return 100;
}
}The check above makes us return nullptr for ((I<string>)c).DefaultTypeOf(); Since it takes !pTargetMT->HasVariance() branch and we fail to comapre I<String> with I<__Canon> .
So this change should allow us to devirtualize cases like class C<T> : I<T> and class C : I<string>.
By the way cases like class C : I<int> are fine since we comparing I<Int32> with I<Int32>
|
I wonder what could be the reason behind inliner failure |
|
BTW all failures in checked builds are happening here runtime/src/coreclr/src/vm/methodtable.cpp Line 2119 in f5c17f1 This is because of this reordering EDIT: It appears I've messed with conditionals. Fixed in 61a7aab |
|
Given the current state of what can be done with devirtualization in the JIT I worry that directly calling an instantiating stub may not provide much of a performance win. We'll need at least some level of performance benchmarking to indicate how much improvement this can provide. |
|
@davidwrighton I've named this PR in consistent way with the problem described in #9588 After spending some time on this issue it appears the title does not describe preciselly what have been done in this PR. Let me briefly explain my understaing of "instantiating/unboxing stub" first: this is purely VM concept, there is no common asm counterpart of this on the compiler side, like the ones we call "PrecodeFixup", "PreStub" etc. It is the duty of the one who generates the code to emit all necessary instructions once they have been told by VM side that I would really love to hear your criticism on stated above. For performance sake
interface I<T>
{
string DefaultTypeOf() => typeof(T).Name;
}
class C : I<string>
{
}
public static class Program
{
[MethodImpl(MethodImplOptions.AggressiveOptimization)]
static int Main()
{
I<string> c = new C();
var dcs = c.DefaultTypeOf();
if (dcs != "String") return 200;
return 100;
}
}when jitted relevant parts of Main would be: 00007FFEFA6AC33A call 00007FFEFA690088 # C.ctor()
00007FFEFA6AC33F mov rcx,rsi
00007FFEFA6AC342 mov rdx,7FFEFAA10860h # I`1[[System.String]] type handle
00007FFEFA6AC34C call 00007FFEFA6ABDB8 # I`1[__Canon][System.__Canon]:DefaultTypeOf()
00007FFEFA6AC351 mov rcx,rax
00007FFEFA6AC354 mov rdx,1DD3FD031A8h
00007FFEFA6AC35E mov rdx,qword ptr [rdx]
00007FFEFA6AC361 call 00007FFEFA695978 # String equals
00007FFEFA6AC366 test eax,eax
00007FFEFA6AC368 je 00007FFEFA6AC375
00007FFEFA6AC36A mov eax,0C8h
00007FFEFA6AC36F add rsp,20h
00007FFEFA6AC373 pop rsi
00007FFEFA6AC374 ret
00007FFEFA6ABDB8 call PrecodeFixupThunk (07FFF594EEAD0h) and after jitting of 00007FFEFA6ABDB8 jmp 00007FFEFA6AC3D0 If we replace interface with class with virtual method like that: class I<T>
{
public virtual string DefaultTypeOf() => typeof(T).Name;
}the only difference (in case of devirt) would be the absence of type handle store in rdx (we didn't tell compiler to emit it, because methodtable of 00007FFEFA68C36F call qword ptr [7FFEFA9F0730h] For me this is clearly better than VSD in non-devirtualized interface case and just one In given example we were not able to inline due to limitations mentioned in #38477 But if we try something like that: interface I<T>
{
T GetAt(int i, T[] tx) => tx[i];
}
class C : I<string>
{
}
public static class Program
{
private static string[] tx = new string[] { "test" };
[MethodImpl(MethodImplOptions.AggressiveOptimization)]
static int Main()
{
I<string> c = new C();
var dcs = c.GetAt(0, new string[] { "test" });
if (dcs != "test") return 200;
return 100;
}
}we will get inlined code IN0001: 000005 mov rcx, 0x7FFEFA9F0858
IN0002: 00000F call CORINFO_HELP_NEWSFAST
IN0003: 000014 mov rcx, rax
IN0004: 000017 call System.Object:.ctor():this
IN0005: 00001C mov rcx, 0x7FFEFA9618E0
IN0006: 000026 mov edx, 1
IN0007: 00002B call CORINFO_HELP_NEWARR_1_OBJ
IN0008: 000030 lea rcx, bword ptr [rax+16]
IN0009: 000034 mov rdx, 0x284C25431A8
IN000a: 00003E mov rsi, gword ptr [rdx]
IN000b: 000041 mov rdx, rsi
IN000c: 000044 call CORINFO_HELP_ASSIGN_REF
IN000d: 000049 mov rcx, rsi
IN000e: 00004C mov rdx, rsi
IN000f: 00004F call System.String:op_Inequality(System.String,System.String):bool
IN0010: 000054 test eax, eax
IN0011: 000056 je SHORT G_M24375_IG05I'm totally ok if you still not convinced: would synthetic BDN harness be suffictient to demonstrate improvement or do I need to hack something like stuff described here then? |
|
Some asm diff stats:
IN0008: 000000 sub rsp, 40
IN0009: 000004 xor rax, rax
IN000a: 000006 mov qword ptr [V00 rsp+20H], rax
G_M29659_IG02: ; offs=00000BH, size=0029H, bbWeight=1 PerfScore 6.75, gcrefRegs=00000000 {}, byrefRegs=00000000 {}, byref
IN0001: 00000B mov rcx, 0x13F900031A8 # "IFrobber<T>:Frob"
IN0002: 000015 mov rcx, gword ptr [rcx]
IN0003: 000018 call System.Console:WriteLine(System.String)
IN0004: 00001D mov rcx, 0x13F900031B0 # "IRobber<T>:Frob"
IN0005: 000027 mov rcx, gword ptr [rcx]
IN0006: 00002A call System.Console:WriteLine(System.String)
IN0007: 00002F mov eax, 100 # 34 + 66
G_M29659_IG03: ; offs=000034H, size=0005H, bbWeight=1 PerfScore 1.25, epilog, nogc, extend
IN000b: 000034 add rsp, 40
IN000c: 000038 ret
IN0005: 000000 sub rsp, 56
IN0006: 000004 xor rax, rax
IN0007: 000006 mov qword ptr [V05 rsp+28H], rax
IN0008: 00000B mov qword ptr [V05+0x8 rsp+30H], rax
G_M27646_IG02: ; offs=000010H, size=0010H, bbWeight=1 PerfScore 2.00, gcrefRegs=00000000 {}, byrefRegs=00000000 {}, byref
IN0001: 000010 lea rcx, bword ptr [V05 rsp+28H]
IN0002: 000015 mov edx, 42
IN0003: 00001A call IM`1[Int32][System.Int32]:DefaultM(int):System.Threading.Tasks.ValueTask
IN0004: 00001F nop
G_M27646_IG03: ; offs=000020H, size=0005H, bbWeight=1 PerfScore 1.25, epilog, nogc, extend
IN0009: 000020 add rsp, 56
IN000a: 000024 retEDIT: to have stats reported by jit-diff properly, it has to be patched to move coreclr binaries around, see dotnet/jitutils#296 |
|
@Rattenkrieg I would prefer some level of actual benchmarking, but the analysis you have provided is sufficient to convince me that there is value here, and this is worth implementing. I'm currently very focused on finishing up some details around crossgen2 compilation and that will occupy me for a few more days before I can deeply analyze the correctness of this change, and probably write a few extra test cases to cover possible problems I see. (Primarily around cases where the type implements multiple generic variants of the same interface.). In addition, I'd like to see @AndyAyersMS or someone he designates take a look at the jit/jitinterface api side of this change. With this work, we are very close to supporting devirtualization of generic virtual methods, which would also be interesting, and it might be best to make the jit interface api reflect that possibility as well. |
|
This is going to take a bit of time to sort through. But some quick initial thoughts:
|
|
@AndyAyersMS I'll check params passing solution for jit-diff -fAll these improvements are due to #43668 (comment) |
No rush needed! I've pushed some tests I used for debugging, most of them are primitive since I was primarily interested in straightforward asm for analysis. But there are few testing "cases where the type implements multiple generic variants of the same interface". I don't propose to merge those, just added it for reference. Now I'm going to work on benchmarks, mostly for methods were reported as changed by jit-diff. |
Rattenkrieg
force-pushed
the
default-interface-method-virtualization
branch
from
c751e71 to
f145018
Compare
|
Here are benchmarks made from pushed tests. Ran with resultsBenchmarkDotNet=v0.12.1.1448-nightly, OS=fedora 33
Intel Core i7-8700 CPU 3.20GHz (Coffee Lake), 1 CPU, 12 logical and 6 physical cores
.NET SDK=6.0.100-alpha.1.20553.9
[Host] : .NET 6.0.0 (6.0.20.55204), X64 RyuJIT
Job-UOOGDK : .NET 6.0 (42.42.42.42424), X64 RyuJIT
Job-ENJNBQ : .NET 6.0 (42.42.42.42424), X64 RyuJIT
PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:DebugType=portable IterationTime=250.0000 ms
MaxIterationCount=20 MinIterationCount=15 WarmupCount=1
|
| @@ -1289,7 +1289,7 @@ void MethodContext::recTryResolveToken(CORINFO_RESOLVED_TOKEN* pResolvedToken, b | |||
|
|
|||
| TryResolveTokenValue value; | |||
|
|
|||
| value.tokenOut = SpmiRecordsHelper::StoreAgnostic_CORINFO_RESOLVED_TOKENout(pResolvedToken, ResolveToken); | |||
| value.tokenOut = SpmiRecordsHelper::StoreAgnostic_CORINFO_RESOLVED_TOKENout(pResolvedToken, TryResolveToken); // copypaste artifacts? | |||
There was a problem hiding this comment.
Be advised, although I'm not 100% sure, same below.
There was a problem hiding this comment.
Yeah, it looks like the wrong map is getting used here. Suspect the keys are conformable and don't collide so we never noticed.
cc @sandreenko
There was a problem hiding this comment.
Agree, thanks for catching this.
|
I don't know the reason behind timeouts in few tests, have everything passing on my machine. Besides that I think that I'm done with implementation. |
Format is failing on CISame on my machineWindows |
Rattenkrieg
force-pushed
the
default-interface-method-virtualization
branch
from
5fe23e4 to
e91c55b
Compare
|
@davidwrighton @AndyAyersMS PTAL Initially I thought that my solution is incomplete wrt generic methods on non-generic interfaces but tests demonstrated that we never touch devirtualization for such cases. Consider: interface I
{
string DefaultTypeOf<T>() => typeof(T).Name;
}
class C : I
{
}
public static class Program
{
[MethodImpl(MethodImplOptions.AggressiveOptimization)]
static int Main()
{
I c = new C();
var dcs = c.DefaultTypeOf();
if (dcs != "Object") return 200;
return 100;
}
}produces IN0001: 000008 mov rdi, 0x12293E518
IN0002: 000012 call CORINFO_HELP_NEWSFAST
IN0003: 000017 mov rbx, rax
IN0004: 00001A mov rdi, rbx
IN0005: 00001D call System.Object:.ctor():this
IN0006: 000022 mov rdi, rbx
IN0007: 000025 mov rsi, 0x12293E3C8
IN0008: 00002F mov rdx, 0x12293E718
IN0009: 000039 call CORINFO_HELP_VIRTUAL_FUNC_PTR
IN000a: 00003E mov rdi, rbx
IN000b: 000041 call rax
IN000c: 000043 mov rdi, rax
IN000d: 000046 mov rsi, 0x19B2371F0
IN000e: 000050 mov rsi, gword ptr [rsi]
IN000f: 000053 call System.String:op_Inequality(System.String,System.String):bool
IN0010: 000058 test eax, eax
IN0011: 00005A je SHORT G_M24375_IG05If we remove DIM and let class implement interface asm stays the same. However if we remove interface, method inlines perfectly: class C
{
public string DefaultTypeOf<T>() => typeof(T).Name;
}IN0001: 000004 mov rdi, 0x11CFFE3F8
IN0002: 00000E call CORINFO_HELP_NEWSFAST
IN0003: 000013 mov rdi, rax
IN0004: 000016 call System.Object:.ctor():this
IN0005: 00001B mov rdi, 0x11CDA4388
IN0006: 000025 call CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPE
IN0007: 00002A mov rdi, rax
IN0008: 00002D mov rax, 0x11CC5A5F0
IN0009: 000037 call gword ptr [rax]System.RuntimeType:get_Name():System.String:this
IN000a: 000039 mov rdi, rax
IN000b: 00003C mov rsi, 0x19D8FD1F0
IN000c: 000046 mov rsi, gword ptr [rsi]
IN000d: 000049 call System.String:op_Inequality(System.String,System.String):bool
IN000e: 00004E test eax, eax
IN000f: 000050 je SHORT G_M24375_IG05But only for one generic param: class C
{
public string DefaultTypeOf<T1, T2>() => typeof(T1).Name + typeof(T2).Name;
}IN0001: 000008 mov rdi, 0x10C68E3F8
IN0002: 000012 call CORINFO_HELP_NEWSFAST
IN0003: 000017 mov rbx, rax
IN0004: 00001A mov rdi, rbx
IN0005: 00001D call System.Object:.ctor():this
IN0006: 000022 mov rdi, rbx
IN0007: 000025 mov rsi, 0x10C68E608
IN0008: 00002F call C:DefaultTypeOf():System.String:this
IN0009: 000034 mov rdi, rax
IN000a: 000037 mov rsi, 0x194F911F0
IN000b: 000041 mov rsi, gword ptr [rsi]
IN000c: 000044 call System.String:op_Inequality(System.String,System.String):bool
IN000d: 000049 test eax, eax
IN000e: 00004B je SHORT G_M24375_IG05Have we ever considered optimizing such case? |
I believe all generic virtual methods are handled via the I had been thinking it wouldn't be worth addressing until after the jit is able propagate methods into |
|
Hey @davidwrighton is there still an interest from dotnet team/you in this PR? |
| public static class Program | ||
| { | ||
| [MethodImpl(MethodImplOptions.AggressiveOptimization)] | ||
| static int Main() |
There was a problem hiding this comment.
We should find a way to test this without AggressiveOptimization. Currently that will disable the Crossgen compiler and so we won't test the behavior in ahead of time compile scenarios.
| @@ -0,0 +1,31 @@ | |||
| using System; | |||
There was a problem hiding this comment.
u [](start = 0, length = 1)
All of the test files need the copyright attribution comment.
| @@ -2094,6 +2094,7 @@ class MethodTable | |||
|
|
|||
| MethodDesc *GetMethodDescForInterfaceMethod(TypeHandle ownerType, MethodDesc *pInterfaceMD, BOOL throwOnConflict); | |||
| MethodDesc *GetMethodDescForInterfaceMethod(MethodDesc *pInterfaceMD, BOOL throwOnConflict); // You can only use this one for non-generic interfaces | |||
| // ^-- I don't believe this is correct statement, we have PRECONDITION(!pInterfaceMD->HasClassOrMethodInstantiation()); which implies it can be used with generic interfaces | |||
There was a problem hiding this comment.
That is not the meaning of the precondition. Its actually the opposite. If the precondition isn't true, the method can't be used, and if the method is on a generic interface then HasClassOrMethodInstantiation will return true, so !HasClassOrMethodInstantiation will be false.
| @@ -581,7 +581,7 @@ class MethodDesc | |||
| // | |||
| // RequiresInstMethodDescArg() | |||
| // The method is itself generic and is shared between generic | |||
| // instantiations but is not itself generic. Furthermore | |||
| // instantiations but is not itself generic. WTF LOL. Furthermore | |||
There was a problem hiding this comment.
Words are hard, and copy/pasting is easy. The "but is not itself generic" is just wrong and should be deleted from this comment.
| @@ -581,7 +581,7 @@ class MethodDesc | |||
| // | |||
| // RequiresInstMethodDescArg() | |||
| // The method is itself generic and is shared between generic | |||
| // instantiations but is not itself generic. Furthermore | |||
| // instantiations but is not itself generic. WTF LOL. Furthermore | |||
| // no "this" pointer is given (e.g. a value type method), so we pass in the | |||
| // exact-instantiation method table as an extra argument. | |||
There was a problem hiding this comment.
method table [](start = 27, length = 12)
This should read "MethodDesc"
| MethodTable* pOwnerMT = OwnerClsHnd.GetMethodTable(); | ||
| pOwnerMT = OwnerClsHnd.GetMethodTable(); | ||
|
|
||
| if (!canCastStraightForward && !(pOwnerMT->IsInterface() && pObjMT->CanCastToInterface(pOwnerMT))) |
There was a problem hiding this comment.
What scenario are you addressing here?
There was a problem hiding this comment.
Sorry, I found your explanation above. If pOwnerMT isn't an interface, why don't we return false here?
In reply to: 570649611 [](ancestors = 570649611)
| @@ -7762,7 +7762,7 @@ getMethodInfoHelper( | |||
| ftn, | |||
| false); | |||
|
|
|||
| // Shared generic or static per-inst methods and shared methods on generic structs | |||
| // Shared generic or static per-inst methods, shared methods on generic structs and default interface methods | |||
There was a problem hiding this comment.
This is a good comment change.
|
@Rattenkrieg We are interested in this change, and based on the performance numbers I'm convinced its a nice small worthwhile change. One reason why this feature wasn't built is that it crosses feature areas and expertise of various teams, and is difficult for us to actually review for correctness. I have a few concerns
|
|
Assigning a shepherd for this PR, feel free to change. |
|
@Rattenkrieg Are you able to answer my questions? If so, please let us know, as otherwise we'll be closing this PR soon. |
|
Hey David! I cannot open this PR anymore, I'm getting
This page is taking too long to load.
Sorry about that. Please try refreshing > and contact us if the problem
persists.
Neither mobile version nor desktop one works. Seems that's because I've
accidentally merged tons of files from main while ago and github is
overwhelmed with big diffs now. So I've missed your questions, sorry.
I definitely looking to finish this PR. I guess the only option for me is
to resubmit changes with new PR and I would gladly ask you to state your
questions in the new one.
…On Tue, Mar 23, 2021, 03:02 David Wrighton ***@***.***> wrote:
@Rattenkrieg <https://github.com/Rattenkrieg> Are you able to answer my
questions? If so, please let us know, as otherwise we'll be closing this PR
soon.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#43668 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABVALU7AGEC2BAPGLN6QY2DTE6V6NANCNFSM4SZAV26A>
.
|
|
@Rattenkrieg you can append |
|
Thank you @lambdageek now I'm back online. @davidwrighton I will address your feedback (hopefully) this weekend and will open a new PR with smaller carbon footprint. |
|
@Rattenkrieg can this PR be closed since you are hoping to open a new one after addressing feedback? |
|
@mangod9 sorry for inconvenience. I was planning to resubmit changes like month ago but currently I have technical issues. I'm really really looking to open new PR soon. |
|
Thanks for the update! |
ghost
locked as resolved and limited conversation to collaborators
Fixes #9588
I don't believe this could be that easy, otherwise why this wasn't added in dotnet/coreclr#15979 🤔
I'm struggling to find jit asm changes at the moment.
produces no diff, I've also tried with
--frameworkand--corelib.And I've also compared jit output from the following command
where XXX were
diamondshape_r.ilproj,non_virtual_calls_to_instance_methods.ilprojandsharedgenerics_d.ilprojand new project I created with the code from #39419... still no difference.
So my thoughts are: either I've missed some optimization flag or messed with setup in any other way OR we never reaching that code 🤷
Going to try tomorrow on Windows.
@MichalStrehovsky PTAL as you were the author of dotnet/coreclr#15979 and friends.