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

Interface devirt #10192

Merged
merged 3 commits into from Mar 17, 2017

Conversation

Projects
None yet
7 participants
@AndyAyersMS
Copy link
Member

AndyAyersMS commented Mar 15, 2017

Three commits, first has VM changes, second has jit changes, third has tests.

@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

AndyAyersMS commented Mar 15, 2017

@jkotas @JosephTremoulet PTAL
cc @dotnet/jit-contrib

More details in the per-commit messages.

For interface calls, when jitting, this invokes the same paths in the runtime as a stub dispatch would, and so may trigger cache updates. It is not clear yet if this update is desirable or should be suppressed. The impact on the caches is similar to what would be without interface call devirtualization, and opportunities for devirtualization are still somewhat scarce, so it may not be easy to find cases where performance differs.

Will post some stats shortly, and workup the desktop equivalent and run tests there too.

@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

AndyAyersMS commented Mar 15, 2017

Data from jit-diff on frameworks. A total of 397 interface calls were devirtualized.

Total bytes of diff: 53 (0.00 % of base)
    diff is a regression.

Total byte diff includes 0 bytes from reconciling methods
        Base had    0 unique methods,        0 unique bytes
        Diff had    0 unique methods,        0 unique bytes

Top file regressions by size (bytes):
         520 : System.Linq.Parallel.dasm (0.08 % of base)
         252 : Microsoft.CodeAnalysis.VisualBasic.dasm (0.01 % of base)
         212 : System.Net.Http.dasm (0.10 % of base)
         148 : System.Security.Cryptography.Algorithms.dasm (0.19 % of base)
          98 : System.Security.Cryptography.X509Certificates.dasm (0.13 % of base)

Top file improvements by size (bytes):
       -1297 : System.Private.CoreLib.dasm (-0.04 % of base)
         -76 : System.IO.FileSystem.dasm (-0.12 % of base)
         -60 : Microsoft.Win32.Registry.dasm (-0.27 % of base)
         -31 : System.Collections.dasm (-0.03 % of base)
         -14 : System.Net.Requests.dasm (-0.01 % of base)

19 total files with size differences (7 improved, 12 regressed).

Top method regessions by size (bytes):
          97 : System.Private.CoreLib.dasm - System.Threading.Tasks.Task:WaitAllBlockingCore(ref,int,struct):bool
          61 : Microsoft.CodeAnalysis.CSharp.dasm - Microsoft.CodeAnalysis.CSharp.Emit.PEModuleBuilder:GetExportedTypes(struct):ref:this
          59 : System.Security.Cryptography.Algorithms.dasm - Internal.Cryptography.CngCommon:HashData(ref,int,int,struct):ref
          59 : System.Security.Cryptography.Algorithms.dasm - Internal.Cryptography.CngCommon:HashData(ref,struct):ref
          59 : System.Security.Cryptography.Cng.dasm - Internal.Cryptography.CngCommon:HashData(ref,int,int,struct):ref

Top method improvements by size (bytes):
         -42 : System.Private.CoreLib.dasm - System.Reflection.RuntimeMethodInfo:GetObjectData(ref,struct):this
         -31 : System.Private.CoreLib.dasm - System.Runtime.InteropServices.WindowsRuntime.DictionaryKeyEnumerator`2[__Canon,__Canon][System.__Canon,System.__Canon]:System.Collections.IEnumerator.get_Current():ref:this
         -31 : System.Private.CoreLib.dasm - System.Runtime.InteropServices.WindowsRuntime.DictionaryValueEnumerator`2[__Canon,__Canon][System.__Canon,System.__Canon]:System.Collections.IEnumerator.get_Current():ref:this
         -31 : System.Private.CoreLib.dasm - System.Runtime.InteropServices.WindowsRuntime.EnumeratorToIteratorAdapter`1[__Canon][System.__Canon]:System.Runtime.InteropServices.WindowsRuntime.IBindableIterator.get_Current():ref:this
         -31 : System.Private.CoreLib.dasm - System.Runtime.InteropServices.WindowsRuntime.ReadOnlyDictionaryKeyEnumerator`2[__Canon,__Canon][System.__Canon,System.__Canon]:System.Collections.IEnumerator.get_Current():ref:this

371 total methods with size differences (292 improved, 79 regressed).

Cherry-picked good looking diff where we can now dead code a runtime lookup:

; Assembly listing for method System.Collections.Concurrent.ConcurrentStack`1
;  [__Canon][System.__Canon]:System.Collections.IEnumerable.GetEnumerator():ref:this

; ** Before **
; Total bytes of code 63
G_M45156_IG01:
       push     rsi
       sub      rsp, 48
       mov      qword ptr [rsp+28H], rcx
       mov      rsi, rcx
G_M45156_IG02:
       mov      rcx, qword ptr [rsi]
       mov      rdx, qword ptr [rcx+56]
       mov      rdx, qword ptr [rdx]
       mov      r11, qword ptr [rdx+40]
       test     r11, r11
       jne      SHORT G_M45156_IG03
       lea      rdx, [(reloc)]
       call     CORINFO_HELP_RUNTIMEHANDLE_CLASS
       mov      r11, rax
G_M45156_IG03:
       mov      rcx, rsi
       mov      rax, qword ptr [r11]
       cmp      dword ptr [rcx], ecx
G_M45156_IG04:
       add      rsp, 48
       pop      rsi
       rex.jmp  rax

; ** After **
; Devirtualized interface call to System.Collections.Generic.IEnumerable`1[__Canon]:GetEnumerator; 
;  now direct call to System.Collections.Concurrent.ConcurrentStack`1[__Canon][System.__Canon]:GetEnumerator [final method]
;
; Total bytes of code 38
G_M45158_IG01:
       push     rsi
       sub      rsp, 16
       mov      qword ptr [rsp+08H], rcx
       mov      rsi, rcx
G_M45158_IG02:
       mov      rdx, qword ptr [rsi]
       mov      rdx, gword ptr [rsi+8]
       mov      rcx, rsi
       lea      rax, [(reloc)]
G_M45158_IG03:
       add      rsp, 16
       pop      rsi
       rex.jmp  rax
@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

AndyAyersMS commented Mar 15, 2017

Not sure yet what is up with the failing windows pri1 tests. I don't normally run the full pri-1 suite locally. Investigating.

//
// For generic interface methods we must have an ownerType to
// safely devirtualize.
if (ownerType != nullptr)

This comment has been minimized.

Copy link
@jkotas

jkotas Mar 15, 2017

Member

May need o check here that the type implements the interface, similar to what is done for the non-interface case?

This comment has been minimized.

Copy link
@AndyAyersMS

AndyAyersMS Mar 15, 2017

Author Member

Yes, that check is needed. The failing tests have instances where casts are missing and the jit ends up seeing an invalid type.

; Loader\classloader\TypeGeneratorTests\TypeGeneratorTest908\Generated908\Generated908.exe
; Framework:MethodCallingTest()
; local0 is type object

IL_0113  06                ldloc.0     
IL_0114  25                dup         
IL_0115  6f 28 00 00 06    callvirt     0x6000028

What is the best way to do this?

This comment has been minimized.

Copy link
@AndyAyersMS

AndyAyersMS Mar 15, 2017

Author Member

Using CanCastToInterface seems to work.

This comment has been minimized.

Copy link
@jkotas

jkotas Mar 16, 2017

Member

Right.

@AndyAyersMS AndyAyersMS force-pushed the AndyAyersMS:InterfaceDevirt branch from d5d5d01 to 913795e Mar 16, 2017

@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

AndyAyersMS commented Mar 16, 2017

Updated with additional check on VM side. Full Pri1 suite passes locally.

@JosephTremoulet
Copy link
Contributor

JosephTremoulet left a comment

LGTM, nothing but nits.

TypeHandle DerivedClsHnd(derivedClass);
MethodTable* pDerivedMT = DerivedClsHnd.GetMethodTable();
_ASSERTE(pDerivedMT->IsRestored() && pDerivedMT->IsFullyLoaded());

This comment has been minimized.

Copy link
@JosephTremoulet

JosephTremoulet Mar 16, 2017

Contributor

Nit: inserted trailing whitespace

// exactly derived class. It is up to the jit to determine whether
// directly calling this method is correct.
pDevirtMD = pDerivedMT->GetMethodDescForSlot(slot);
_ASSERTE(pDevirtMD->IsRestored());

This comment has been minimized.

Copy link
@JosephTremoulet

JosephTremoulet Mar 16, 2017

Contributor

Redundant with the assert 3 lines down; did you mean to have both?

This comment has been minimized.

Copy link
@AndyAyersMS

AndyAyersMS Mar 16, 2017

Author Member

Since it is common to both virtual and interface, we only want the second one... will fix.

{
new public int F() { return 33; }
override public int G() { return 22; }

This comment has been minimized.

Copy link
@JosephTremoulet

JosephTremoulet Mar 16, 2017

Contributor

You could consider adding an interface method that B explicitly implements (int Iz.H() { return 11; })

You could also consider adding interfaces that B implements and Z re-implements (for various combinations of implementing explicitly/name-matching-virtual/name-matching-nonvirtual/inheriting-from-B).

public static int Main()
{
// B:F Z:G B:F B:G
return Fx(new Z()) + Gy(new Z()) + Fx(new B()) + Gy(new B()) - 65;

This comment has been minimized.

Copy link
@JosephTremoulet

JosephTremoulet Mar 16, 2017

Contributor

Can we get ((Ix)new Z()).F() as well, or is there something special about having the cast to interface happen implicitly for argument passing?

This comment has been minimized.

Copy link
@AndyAyersMS

AndyAyersMS Mar 16, 2017

Author Member

I'll look into this. If C# can elide emitting an actual cast that version should also lead to devirtualization.

We currently lose types in checkcast. I have a partial fix worked up but need to understand (if downcasts are explicit) which casts add type info and which casts lose type info and avoid losing type info.

This comment has been minimized.

Copy link
@benaadams

benaadams Mar 16, 2017

Collaborator

There are some interface casts that change behaviour, like casting a dictionary to IDictionary changes the enumerator behaviour (from KeyValuePair to DictionaryEntry); may or maynot before something to watch for?

This comment has been minimized.

Copy link
@AndyAyersMS

AndyAyersMS Mar 16, 2017

Author Member

@benaadams I believe that is a C# language feature. At runtime an object only has one type and casting does not alter the object in any way.

This comment has been minimized.

Copy link
@AndyAyersMS

AndyAyersMS Mar 16, 2017

Author Member

Looks like downcasts (to supertype) aren't explicit. Will update the test with some more cases, though I am not sure I will get every possible variation.

@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

AndyAyersMS commented Mar 16, 2017

Windows debug failure was a timeout in tailcall_v4_hijacking.

Windows release failures don't repro locally. Only one of the cases does any interface call devirtualization, and it looks to be correct.

Hate to do this but I am going to retry....

@dotnet-bot retest Windows_NT x64 Release Priority 1 Build and Test
@dotnet-bot retest Windows_NT x64 Debug Build and Test

AndyAyersMS added some commits Mar 16, 2017

Interface call devirtualization: VM side of changes
Extend resolveVirtualMethod to take in the owner type needed to resolve
shared generic interface calls, and implement VM support for interface
call devirtualization. Add a check that the class presented by the jit
implements the interface, to catch cases where the IL is missing type
casts.

Update jit GUID, SPMI and zap to reflect the changed signature.
Interface call devirtualization: jit side of changes
Unblock VM queries where the base class is an interface (queries
where the implementing class is also an interface are still blocked
as no resolution is possible). Pass along the context handle that
the jit got from getCallInfo.

@AndyAyersMS AndyAyersMS force-pushed the AndyAyersMS:InterfaceDevirt branch from 913795e to 4e02195 Mar 16, 2017

@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

AndyAyersMS commented Mar 16, 2017

Retest was green.

Updates per @JosephTremoulet comments.

@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

AndyAyersMS commented Mar 17, 2017

Ported changes over to destktop, initial testing looks good there too.

{
new public int F() { return 13; }
override public int G() { return 17; }
int Iz.H() { return 19; }

This comment has been minimized.

Copy link
@JosephTremoulet

JosephTremoulet Mar 17, 2017

Contributor

If Iz had a couple more methods, B and Z could implement and re-implement them similar to how they do F and G (shadowing and overriding). I don't know/recall what the various C#isms map to in the MSIL for slots and overrides well enough to know if that would be testing anything novel over what you have.

@JosephTremoulet

This comment has been minimized.

Copy link
Contributor

JosephTremoulet commented Mar 17, 2017

Updates look good with one comment, still green from me.

@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

AndyAyersMS commented Mar 17, 2017

@jkotas any other feedback?

@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

AndyAyersMS commented Mar 17, 2017

@dotnet-bot test Windows_NT x64 corefx_baseline

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Mar 17, 2017

LGTM

@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

AndyAyersMS commented Mar 17, 2017

@RussKeldorph what is the trigger phrase for corefx testing?

@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

AndyAyersMS commented Mar 17, 2017

Ah, looks like what I used above worked, but the test leg seems broken....

@RussKeldorph

This comment has been minimized.

Copy link
Member

RussKeldorph commented Mar 17, 2017

@AndyAyersMS Yes, corefx jobs are broken due to 14207f4 breaking change. Apparently we have to wait until corefx ingests a coreclr with those bits before the break can be resolved. @rahku is tracking it.

@AndyAyersMS AndyAyersMS merged commit 8208cb5 into dotnet:master Mar 17, 2017

14 of 15 checks passed

Windows_NT x64 Checked Build and Test (Jit - CoreFx) Build finished.
Details
CentOS7.1 x64 Debug Build and Test Build finished.
Details
FreeBSD x64 Checked Build Build finished.
Details
Linux ARM Emulator Cross Debug Build Build finished.
Details
Linux ARM Emulator Cross Release Build Build finished.
Details
OSX10.12 x64 Checked Build and Test Build finished.
Details
Ubuntu x64 Checked Build and Test Build finished.
Details
Ubuntu x64 Formatting Build finished.
Details
Windows_NT arm Cross Debug Build Build finished.
Details
Windows_NT arm Cross Release Build Build finished.
Details
Windows_NT arm64 Cross Debug Build Build finished.
Details
Windows_NT x64 Debug Build and Test Build finished.
Details
Windows_NT x64 Formatting Build finished.
Details
Windows_NT x64 Release Priority 1 Build and Test Build finished.
Details
Windows_NT x86 Checked Build and Test Build finished.
Details

@AndyAyersMS AndyAyersMS deleted the AndyAyersMS:InterfaceDevirt branch Mar 17, 2017

@karelz karelz added this to the 2.0.0 milestone Aug 28, 2017

@karelz karelz added this to the 2.0.0 milestone Aug 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.