Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Instantiating stub for devirtualized default interface method on a generic type#43668

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@Rattenkrieg
Copy link
Contributor

Fixes#9588
I don't believe this could be that easy, otherwise why this wasn't added indotnet/coreclr#15979 🤔
I'm struggling to find jit asm changes at the moment.

./jit-diff diff --output /tmp/jit-diffs \--core_root ~/projs/runtime/artifacts/tests/coreclr/Linux.x64.Release/Tests/Core_Root \--base ~/projs/runtime/artifacts/bin/coreclr/Linux.x64.Release \--diff ~/projs/runtime/artifacts/bin/coreclr/Linux.x64.Release.patched \--arch x64 --crossgen ~/projs/runtime/artifacts/tests/coreclr/Linux.x64.Release/Tests/Core_Root/crossgen \--tests --test_root ~/projs/runtime/src/tests/

produces no diff, I've also tried with--framework and--corelib.
And I've also compared jit output from the following command

COMPlus_JitDump=Main COMPlus_JitDisasm=Main CORE_ROOT=~/projs/runtime/artifacts/tests/coreclr/Linux.x64.Debug/Tests/Core_Root \~/projs/runtime/artifacts/tests/coreclr/Linux.x64.Release/XXX

where XXX werediamondshape_r.ilproj,non_virtual_calls_to_instance_methods.ilproj andsharedgenerics_d.ilproj and 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 ofdotnet/coreclr#15979 and friends.

am11 reacted with thumbs up emoji
@MichalStrehovsky
Copy link
Member

@MichalStrehovsky PTAL as you were the author ofdotnet/coreclr#15979 and friends.

Let me know if you have trouble extracting the failing tests from the CI. E.g. here is one:

    Loader\classloader\DefaultInterfaceMethods\constrainedcall\constrainedcall\constrainedcall.cmd [FAIL]            Assert failure(PID 2016 [0x000007e0], Thread: 2844 [0x0b1c]): false            CORECLR! MethodDesc::FindOrCreateAssociatedMethodDesc + 0x1F9E (0x00007ff8`1658dc06)      CORECLR! CEEInfo::resolveVirtualMethodHelper + 0x350 (0x00007ff8`163b102c)      CORECLR! CEEInfo::resolveVirtualMethod + 0x17C (0x00007ff8`163b0c5c)      CLRJIT! Compiler::impDevirtualizeCall + 0x5A2 (0x00007ff8`15585606)      CLRJIT! Compiler::impImportCall + 0x1E68 (0x00007ff8`155931d0)      CLRJIT! Compiler::impImportBlockCode + 0x45AE (0x00007ff8`1558c9be)      CLRJIT! Compiler::impImportBlock + 0xD3 (0x00007ff8`15587b53)      CLRJIT! Compiler::impImport + 0x39F (0x00007ff8`1558727f)      CLRJIT! Compiler::fgImport + 0x17 (0x00007ff8`15543237)      CLRJIT! Phase::Run + 0x3B (0x00007ff8`15622f3b)          File: F:\workspace\_work\1\s\src\coreclr\src\vm\genmeth.cpp Line: 811          Image: C:\h\w\A23308C7\p\CoreRun.exe

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.

@Rattenkrieg
Copy link
ContributorAuthor

Let me know if you have trouble extracting the failing tests from the CI. E.g. here is one:

Thank you! I've managed to dig through pipelines but I'm struggling to reproduce that failure.
What I did:

# to have clrjit and coreclr libs with assertions enabled> .\build.cmd clr+libs -rc checked -c checked -lc debug# otherwise tests won't be built> .\build.cmd clr+libs -rc release -c release -lc release> .\src\tests\build.cmd checked# run all tests just in case there are other failures> .\src\tests\run.cmd checked# at this point everything fine except couple of bencharks like from benchmarkgame# trying to reproduce Loader\classloader\DefaultInterfaceMethods\constrainedcall\constrainedcall\constrainedcall.cmd failure> $env:CORE_ROOT=".\runtime\artifacts\tests\coreclr\Windows_NT.x64.Checked\Tests\Core_Root"> .\artifacts\tests\coreclr\Windows_NT.x64.Checked\Loader\classloader\DefaultInterfaceMethods\constrainedcall\constrainedcall\constrainedcall.cmd

... and it passes

BEGIN EXECUTION "C:\Users\sergey\source\repos\runtime\artifacts\tests\coreclr\Windows_NT.x64.Checked\Tests\Core_Root\corerun.exe" constrainedcall.dllConstrained calls that require runtime lookup are OKRuntime does not support lookups with runtime determined boxingCompile time constraint resolution to default interface method OKExpected: 100Actual: 100END EXECUTION - PASSEDPASSED

🤯

I see there is setup being done duringpipeline builds:

C:\h\w\A1A808CD\w\AB4A0904\e>set __TestEnv=C:\h\w\A1A808CD\w\AB4A0904\u\SetStressModes_no_tiered_compilation.cmd

I can't find that file in my repo/artifacts. Could there be some crucial setup steps like settingCOMPlus_MakeThisFailForSure etc?

@MichalStrehovsky
Copy link
Member

like setting COMPlus_MakeThisFailForSure etc?

:) Try COMPlus_TieredCompilation=0

am11 and Rattenkrieg reacted with thumbs up emoji

@Rattenkrieg
Copy link
ContributorAuthor

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

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.

Copy link
ContributorAuthor

@RattenkriegRattenkrieg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I'm done with general logic changes. The gist is:
resolveVirtualMethodHelper now returns wrappedMethodDesc for devirtualized default interface method.

interfaceI<T>{stringDefaultTypeOf()=>typeof(T).Name;}classC:I<string>{}publicstaticclassProgram{[MethodImpl(MethodImplOptions.AggressiveOptimization)]staticintMain(){varc=newC();vardcs=((I<string>)c).DefaultTypeOf();if(dcs!="String")return200;return100;}}

For the program above we will getI'1[[System.String]][System.String].DefaultTypeOf frompDerivedMT->GetMethodDescForInterfaceMethod(TypeHandle(pOwnerMT), pBaseMD, FALSE); Which is already an instantiating stub!
Then we going to unwrap it intoI'1[[System.__Canon]][System.String].DefaultTypeOf, tell to caller that*requiresInstMethodTableArg = true and do our business inimpDevirtualizeCall:
Get the instantiation param from context:GenTree* instParam = gtNewIconEmbClsHndNode(exactClassHandle);. Context here isI<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: 00000Fcall     CORINFO_HELP_NEWSFASTIN0003:000014movrsi,raxIN0004:000017movrcx,rsiIN0005: 00001Acall     System.Object:.ctor():thisIN0006: 00001Fmovrcx,rsiIN0007:000022movrdx,0x7FF95AE40020                                                                            # I`1[System.String] type handleIN0008: 00002Ccall     I`1[__Canon][System.__Canon]:DefaultTypeOf():System.String:this

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

MichalStrehovsky reacted with laugh emoji
Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I've delayed this check because it forbids several devirt oportunities:

interfaceI<T>{stringDefaultTypeOf()=>typeof(T).Name;}classC:I<string>{}publicstaticclassProgram{[MethodImpl(MethodImplOptions.AggressiveOptimization)]staticintMain(){varc=newC();vardcs=((I<string>)c).DefaultTypeOf();if(dcs!="String")return200;return100;}}

The check above makes us return nullptr for((I<string>)c).DefaultTypeOf(); Since it takes!pTargetMT->HasVariance() branch and we fail to comapreI<String> withI<__Canon> .

https://github.com/dotnet/runtime/blob/dfc8da5dd778c6b165e6be9e7c16c82ebf571bfb/src/coreclr/src/vm/methodtable.inl#L1543

So this change should allow us to devirtualize cases likeclass C<T> : I<T> andclass C : I<string>.
By the way cases likeclass C : I<int> are fine since we comparing I<Int32> withI<Int32>

@Rattenkrieg
Copy link
ContributorAuthor

I wonder what could be the reason behind inliner failure

impDevirtualizeCall: Trying to devirtualize interface call:    class for 'this' is C [exact] (attrib 20000000)    base method is I`1[__Canon]::DefaultTypeOf--- base class is interface    devirt to I`1[[System.Object, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]][System.Object]::DefaultTypeOf -- exact               [000010] --C-G-------              *  CALLV stub ref    I`1[__Canon][System.__Canon].DefaultTypeOf               [000009] ------------ this in rcx  \--*  LCL_VAR   ref    V00 loc0    exact; can devirtualize... after devirt...               [000010] --C-G-------              *  CALL nullcheck ref    I`1[[System.Object, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]][System.Object].DefaultTypeOf               [000009] ------------ this in rcx  \--*  LCL_VAR   ref    V00 loc0INLINER: during 'impMarkInlineCandidate' result 'failed this callee' reason 'cannot get method info' for 'Program:Main():int' calling 'I`1[[System.Object, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]][System.Object]:DefaultTypeOf():System.String:this'INLINER: Marking I`1[[System.Object, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]][System.Object]:DefaultTypeOf():System.String:this as NOINLINE because of cannot get method infoINLINER: during 'impMarkInlineCandidate' result 'failed this callee' reason 'cannot get method info'

@Rattenkrieg
Copy link
ContributorAuthor

Rattenkrieg commentedOct 27, 2020
edited
Loading

BTW all failures in checked builds are happening here

PRECONDITION(IsArray() ||ImplementsEquivalentInterface(ownerType.GetMethodTable()) || ownerType.GetMethodTable()->HasVariance());

This is because of this reordering
https://github.com/dotnet/runtime/pull/43668/files#r512813024
I'm investigating.

EDIT:

It appears I've messed with conditionals. Fixed in61a7aab

@davidwrighton
Copy link
Member

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.

Rattenkrieg reacted with eyes emoji

@Rattenkrieg
Copy link
ContributorAuthor

Rattenkrieg commentedOct 30, 2020
edited
Loading

@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 thatCORINFO_METHOD_HANDLE they dealing with is actually an instantiating/unboxing stub. You could see such contract being implemented in this PR betwenhttps://github.com/dotnet/runtime/blob/25f5d6b35a323c4124927aada8d87e416c501e16/src/coreclr/src/vm/jitinterface.cpp#L8963-L8971 andhttps://github.com/dotnet/runtime/blob/25f5d6b35a323c4124927aada8d87e416c501e16/src/coreclr/src/jit/importer.cpp#L20822

I would really love to hear your criticism on stated above.

For performance sakewe are trading additional instruction for storing type handle in register * for eliminated virtual stub dispatch and potential inlining. Consider following c# code:

  • EDIT: it appears there is no regression wrt additional instructions and register allocation. For nondevirtualized case there would be a store of virtual stub in r11 introduced at morph stage, see
    if (call->IsVirtualStub())
interfaceI<T>{stringDefaultTypeOf()=>typeof(T).Name;}classC:I<string>{}publicstaticclassProgram{[MethodImpl(MethodImplOptions.AggressiveOptimization)]staticintMain(){I<string>c=newC();vardcs=c.DefaultTypeOf();if(dcs!="String")return200;return100;}}

when jitted relevant parts of Main would be:

00007FFEFA6AC33Acall        00007FFEFA690088                   # C.ctor()00007FFEFA6AC33Fmovrcx,rsi00007FFEFA6AC342movrdx,7FFEFAA10860h                  # I`1[[System.String]] type handle00007FFEFA6AC34Ccall        00007FFEFA6ABDB8                   # I`1[__Canon][System.__Canon]:DefaultTypeOf()00007FFEFA6AC351movrcx,rax00007FFEFA6AC354movrdx,1DD3FD031A8h00007FFEFA6AC35Emovrdx,qword ptr[rdx]00007FFEFA6AC361call        00007FFEFA695978                   # String equals00007FFEFA6AC366testeax,eax00007FFEFA6AC368je          00007FFEFA6AC37500007FFEFA6AC36Amoveax,0C8h00007FFEFA6AC36Faddrsp,20h00007FFEFA6AC373poprsi00007FFEFA6AC374ret

00007FFEFA6ABDB8 initially would contain

00007FFEFA6ABDB8call        PrecodeFixupThunk (07FFF594EEAD0h)

and after jitting ofDefaultTypeOf() just a jmp to actual code

00007FFEFA6ABDB8jmp         00007FFEFA6AC3D0

If we replace interface with class with virtual method like that:

classI<T>{publicvirtualstringDefaultTypeOf()=>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 ofC is sufficient) and direct call replacement with indirect(tbh I don't know what mechanism drives that and why this is the virtual call convention):

00007FFEFA68C36Fcall        qword ptr[7FFEFA9F0730h]

For me this is clearly better than VSD in non-devirtualized interface case and just onemov worse than devirtualized superclass case.

In given example we were not able to inline due to limitations mentioned in#38477 But if we try something like that:

interfaceI<T>{TGetAt(inti,T[]tx)=>tx[i];}classC:I<string>{}publicstaticclassProgram{privatestaticstring[]tx=newstring[]{"test"};[MethodImpl(MethodImplOptions.AggressiveOptimization)]staticintMain(){I<string>c=newC();vardcs=c.GetAt(0,newstring[]{"test"});if(dcs!="test")return200;return100;}}

we will get inlined code

IN0001:000005movrcx,0x7FFEFA9F0858IN0002: 00000Fcall     CORINFO_HELP_NEWSFASTIN0003:000014movrcx,raxIN0004:000017call     System.Object:.ctor():thisIN0005: 00001Cmovrcx,0x7FFEFA9618E0IN0006:000026movedx,1IN0007: 00002Bcall     CORINFO_HELP_NEWARR_1_OBJIN0008:000030learcx, bword ptr[rax+16]IN0009:000034movrdx,0x284C25431A8IN000a: 00003Emovrsi, gword ptr[rdx]IN000b:000041movrdx,rsiIN000c:000044call     CORINFO_HELP_ASSIGN_REFIN000d:000049movrcx,rsiIN000e: 00004Cmovrdx,rsiIN000f: 00004Fcall     System.String:op_Inequality(System.String,System.String):boolIN0010:000054testeax,eaxIN0011:000056je       SHORT G_M24375_IG05

I'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 describedhere then?

@Rattenkrieg
Copy link
ContributorAuthor

Rattenkrieg commentedOct 31, 2020
edited
Loading

Some asm diff stats:
> .\jitutils\bin\jit-diff diff --output c:\diffs --core_root <...>\Windows_NT.x64.Debug\Tests\Core_Root --diff <...>\patched\Windows_NT.x64.Debug\ --base <...>\artifacts\bin\coreclr\Windows_NT.x64.Debug\ --pmi --tests --test_root <...>\artifacts\tests\coreclr\Windows_NT.x64.Release\Loader\classloader\DefaultInterfaceMethods\

Top file improvements (bytes):        -111 : devirttest39419.dasm (-75.00% of base)         -55 : constrainedcall\constrained3\constrained3.dasm (-49.11% of base)         -14 : devirttestinline\devirttestinline\devirttestinline.dasm (-6.17% of base)        ... rest is the noise from my debug c# code

devirttestinline is the example given in the end of my previous message,constrained3 is herehttps://github.com/dotnet/runtime/blob/master/src/tests/Loader/classloader/DefaultInterfaceMethods/constrainedcall/constrained3.il DIMs were inlined and affected by other optimizations like constant folding:

IN0008:000000subrsp,40IN0009:000004xorrax,raxIN000a:000006mov      qword ptr[V00rsp+20H],raxG_M29659_IG02:        ; offs=00000BH, size=0029H, bbWeight=1    PerfScore 6.75, gcrefRegs=00000000 {}, byrefRegs=00000000 {}, byrefIN0001:00000Bmovrcx,0x13F900031A8                                        #"IFrobber<T>:Frob"IN0002:000015movrcx, gword ptr[rcx]IN0003:000018call     System.Console:WriteLine(System.String)IN0004: 00001Dmovrcx,0x13F900031B0                                        #"IRobber<T>:Frob"IN0005:000027movrcx, gword ptr[rcx]IN0006: 00002Acall     System.Console:WriteLine(System.String)IN0007: 00002Fmoveax,100                                                  #34+66G_M29659_IG03:        ; offs=000034H, size=0005H, bbWeight=1    PerfScore 1.25, epilog, nogc, extendIN000b:000034addrsp,40IN000c:000038ret

devirttest39419 is example from#39419:

IN0005:000000subrsp,56IN0006:000004xorrax,raxIN0007:000006mov      qword ptr[V05rsp+28H],raxIN0008:00000Bmov      qword ptr[V05+0x8rsp+30H],raxG_M27646_IG02:        ; offs=000010H, size=0010H, bbWeight=1    PerfScore 2.00, gcrefRegs=00000000 {}, byrefRegs=00000000 {}, byrefIN0001:000010learcx, bword ptr[V05rsp+28H]IN0002:000015movedx,42IN0003: 00001Acall     IM`1[Int32][System.Int32]:DefaultM(int):System.Threading.Tasks.ValueTaskIN0004: 00001FnopG_M27646_IG03:        ; offs=000020H, size=0005H, bbWeight=1    PerfScore 1.25, epilog, nogc, extendIN0009:000020addrsp,56IN000a:000024ret

EDIT: to have stats reported by jit-diff properly, it has to be patched to move coreclr binaries around, seedotnet/jitutils#296

@davidwrighton
Copy link
Member

@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.

@AndyAyersMS
Copy link
Member

This is going to take a bit of time to sort through. But some quick initial thoughts:

  • superpmi handling for trueIN, OUT parameters is going to be more complex than what you have implemented. From what I can tell we actually don't have any trueIN, OUT parameters on the jit interface currently (resolveToken is marked that way but the set of fields read and written are distinct). If we keep with the current approach you will need to use theIN value for the key and record theOUT value as part of the result.
  • Because of that it might seem tempting to instead have separateIN andOUT params; at that point we might as well consider turning the entire set of arguments and results into a struct (much like we do forCORINFO_RESOLVED_TOKEN) with distinct in and out parts; then SPMI memoization can continue to work in the simple manner it does now.
  • Do you see any codegen impact in the framework code (pass-f tojit-diff)?

@Rattenkrieg
Copy link
ContributorAuthor

@AndyAyersMS I'll check params passing solution forCORINFO_RESOLVED_TOKEN and change my code accordingly, thanks!

jit-diff -f
> .\jitutils\bin\jit-diff diff --output c:\diffs --core_root <...>\Windows_NT.x64.Debug\Tests\Core_Root --diff <...>\patched\Windows_NT.x64.Debug\ --base <...>\artifacts\bin\coreclr\Windows_NT.x64.Debug\   --pmi -fBeginning PMI CodeSize Diffs for System.Private.CoreLib.dll, framework assemblies- Finished 268/268 Base 268/268 Diff [5699.9 sec]Completed PMI CodeSize Diffs for System.Private.CoreLib.dll, framework assemblies in 5701.03sDiffs (if any) can be viewed by comparing: c:\diffs\dasmset_50\base c:\diffs\dasmset_50\diffgit diff --no-index --diff-filter=M --exit-code --numstat c:\diffs\dasmset_50\diff c:\diffs\dasmset_50\baseAnalyzing CodeSize diffs...Found 9 files with textual diffs.PMI CodeSize Diffs for System.Private.CoreLib.dll, framework assemblies for  default jitSummary of Code Size diffs:(Lower is better)Total bytes of base: 54772077Total bytes of diff: 54769555Total bytes of delta: -2522 (-0.00% of base)    diff is an improvement.Top file improvements (bytes):        -946 : Newtonsoft.Json.dasm (-0.12% of base)        -588 : Microsoft.CodeAnalysis.VisualBasic.dasm (-0.01% of base)        -578 : Microsoft.CodeAnalysis.CSharp.dasm (-0.01% of base)        -197 : Microsoft.Diagnostics.Tracing.TraceEvent.dasm (-0.01% of base)        -154 : System.ComponentModel.Composition.dasm (-0.05% of base)         -28 : System.ComponentModel.Annotations.dasm (-0.07% of base)         -20 : Microsoft.CodeAnalysis.dasm (-0.00% of base)          -8 : System.Diagnostics.StackTrace.dasm (-0.19% of base)          -3 : System.Linq.Expressions.dasm (-0.00% of base)9 total files with Code Size differences (9 improved, 0 regressed), 259 unchanged.Top method improvements (bytes):        -500 (-1.68% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Microsoft.CodeAnalysis.CSharp.CSharpCommandLineParser:Parse(System.Collections.Generic.IEnumerable`1[[System.String, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]],System.String,System.String,System.String):Microsoft.CodeAnalysis.CSharp.CSharpCommandLineArguments:this        -490 (-1.42% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Microsoft.CodeAnalysis.VisualBasic.VisualBasicCommandLineParser:Parse(System.Collections.Generic.IEnumerable`1[[System.String, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]],System.String,System.String,System.String):Microsoft.CodeAnalysis.VisualBasic.VisualBasicCommandLineArguments:this         -84 (-8.70% of base) : System.ComponentModel.Composition.dasm - System.ComponentModel.Composition.Hosting.CompositionServices:GetPartMetadataForType(System.Type,int):System.Collections.Generic.IDictionary`2[[System.String, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.Object, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]]         -73 (-4.68% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.JsonValidatingReader:get_CurrentMemberSchemas():System.Collections.Generic.IList`1[[Newtonsoft.Json.Schema.JsonSchemaModel, Newtonsoft.Json, Version=12.0.0.0, Culture=neutral, PublicKeyToken=30ad4fe6b2a6aeed]]:this         -60 (-9.19% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.Utilities.DynamicProxyMetaObject`1[__Canon][System.__Canon]:CallMethodReturnLast(System.String,System.Dynamic.DynamicMetaObjectBinder,System.Collections.Generic.IEnumerable`1[[System.Linq.Expressions.Expression, System.Linq.Expressions, Version=5.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]],Fallback[__Canon]):System.Dynamic.DynamicMetaObject:this         -60 (-10.08% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.Utilities.DynamicProxyMetaObject`1[Byte][System.Byte]:CallMethodReturnLast(System.String,System.Dynamic.DynamicMetaObjectBinder,System.Collections.Generic.IEnumerable`1[[System.Linq.Expressions.Expression, System.Linq.Expressions, Version=5.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]],Fallback[Byte]):System.Dynamic.DynamicMetaObject:this         -60 (-10.08% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.Utilities.DynamicProxyMetaObject`1[Int16][System.Int16]:CallMethodReturnLast(System.String,System.Dynamic.DynamicMetaObjectBinder,System.Collections.Generic.IEnumerable`1[[System.Linq.Expressions.Expression, System.Linq.Expressions, Version=5.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]],Fallback[Int16]):System.Dynamic.DynamicMetaObject:this         -60 (-10.08% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.Utilities.DynamicProxyMetaObject`1[Int32][System.Int32]:CallMethodReturnLast(System.String,System.Dynamic.DynamicMetaObjectBinder,System.Collections.Generic.IEnumerable`1[[System.Linq.Expressions.Expression, System.Linq.Expressions, Version=5.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]],Fallback[Int32]):System.Dynamic.DynamicMetaObject:this         -60 (-10.08% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.Utilities.DynamicProxyMetaObject`1[Double][System.Double]:CallMethodReturnLast(System.String,System.Dynamic.DynamicMetaObjectBinder,System.Collections.Generic.IEnumerable`1[[System.Linq.Expressions.Expression, System.Linq.Expressions, Version=5.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]],Fallback[Double]):System.Dynamic.DynamicMetaObject:this         -60 (-10.08% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.Utilities.DynamicProxyMetaObject`1[Vector`1][System.Numerics.Vector`1[System.Single]]:CallMethodReturnLast(System.String,System.Dynamic.DynamicMetaObjectBinder,System.Collections.Generic.IEnumerable`1[[System.Linq.Expressions.Expression, System.Linq.Expressions, Version=5.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]],Fallback[Vector`1]):System.Dynamic.DynamicMetaObject:this         -60 (-10.08% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.Utilities.DynamicProxyMetaObject`1[Int64][System.Int64]:CallMethodReturnLast(System.String,System.Dynamic.DynamicMetaObjectBinder,System.Collections.Generic.IEnumerable`1[[System.Linq.Expressions.Expression, System.Linq.Expressions, Version=5.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]],Fallback[Int64]):System.Dynamic.DynamicMetaObject:this         -36 (-0.73% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - Microsoft.Diagnostics.Tracing.Etlx.TraceLog:CopyRawEvents(Microsoft.Diagnostics.Tracing.TraceEventDispatcher,FastSerialization.IStreamWriter):this         -30 (-3.71% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.Utilities.DynamicProxyMetaObject`1[__Canon][System.__Canon]:BuildCallMethodWithResult(System.String,System.Dynamic.DynamicMetaObjectBinder,System.Collections.Generic.IEnumerable`1[[System.Linq.Expressions.Expression, System.Linq.Expressions, Version=5.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]],System.Dynamic.DynamicMetaObject,Fallback[__Canon]):System.Dynamic.DynamicMetaObject:this         -30 (-4.02% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.Utilities.DynamicProxyMetaObject`1[Byte][System.Byte]:BuildCallMethodWithResult(System.String,System.Dynamic.DynamicMetaObjectBinder,System.Collections.Generic.IEnumerable`1[[System.Linq.Expressions.Expression, System.Linq.Expressions, Version=5.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]],System.Dynamic.DynamicMetaObject,Fallback[Byte]):System.Dynamic.DynamicMetaObject:this         -30 (-4.02% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.Utilities.DynamicProxyMetaObject`1[Int16][System.Int16]:BuildCallMethodWithResult(System.String,System.Dynamic.DynamicMetaObjectBinder,System.Collections.Generic.IEnumerable`1[[System.Linq.Expressions.Expression, System.Linq.Expressions, Version=5.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]],System.Dynamic.DynamicMetaObject,Fallback[Int16]):System.Dynamic.DynamicMetaObject:this         -30 (-4.02% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.Utilities.DynamicProxyMetaObject`1[Int32][System.Int32]:BuildCallMethodWithResult(System.String,System.Dynamic.DynamicMetaObjectBinder,System.Collections.Generic.IEnumerable`1[[System.Linq.Expressions.Expression, System.Linq.Expressions, Version=5.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]],System.Dynamic.DynamicMetaObject,Fallback[Int32]):System.Dynamic.DynamicMetaObject:this         -30 (-4.02% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.Utilities.DynamicProxyMetaObject`1[Double][System.Double]:BuildCallMethodWithResult(System.String,System.Dynamic.DynamicMetaObjectBinder,System.Collections.Generic.IEnumerable`1[[System.Linq.Expressions.Expression, System.Linq.Expressions, Version=5.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]],System.Dynamic.DynamicMetaObject,Fallback[Double]):System.Dynamic.DynamicMetaObject:this         -30 (-4.02% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.Utilities.DynamicProxyMetaObject`1[Vector`1][System.Numerics.Vector`1[System.Single]]:BuildCallMethodWithResult(System.String,System.Dynamic.DynamicMetaObjectBinder,System.Collections.Generic.IEnumerable`1[[System.Linq.Expressions.Expression, System.Linq.Expressions, Version=5.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]],System.Dynamic.DynamicMetaObject,Fallback[Vector`1]):System.Dynamic.DynamicMetaObject:this         -30 (-4.02% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.Utilities.DynamicProxyMetaObject`1[Int64][System.Int64]:BuildCallMethodWithResult(System.String,System.Dynamic.DynamicMetaObjectBinder,System.Collections.Generic.IEnumerable`1[[System.Linq.Expressions.Expression, System.Linq.Expressions, Version=5.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]],System.Dynamic.DynamicMetaObject,Fallback[Int64]):System.Dynamic.DynamicMetaObject:this         -30 (-2.08% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.Utilities.ExpressionReflectionDelegateFactory:BuildMethodCall(System.Reflection.MethodBase,System.Type,System.Linq.Expressions.ParameterExpression,System.Linq.Expressions.ParameterExpression):System.Linq.Expressions.Expression:thisTop method improvements (percentages):         -60 (-10.08% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.Utilities.DynamicProxyMetaObject`1[Byte][System.Byte]:CallMethodReturnLast(System.String,System.Dynamic.DynamicMetaObjectBinder,System.Collections.Generic.IEnumerable`1[[System.Linq.Expressions.Expression, System.Linq.Expressions, Version=5.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]],Fallback[Byte]):System.Dynamic.DynamicMetaObject:this         -60 (-10.08% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.Utilities.DynamicProxyMetaObject`1[Int16][System.Int16]:CallMethodReturnLast(System.String,System.Dynamic.DynamicMetaObjectBinder,System.Collections.Generic.IEnumerable`1[[System.Linq.Expressions.Expression, System.Linq.Expressions, Version=5.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]],Fallback[Int16]):System.Dynamic.DynamicMetaObject:this         -60 (-10.08% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.Utilities.DynamicProxyMetaObject`1[Int32][System.Int32]:CallMethodReturnLast(System.String,System.Dynamic.DynamicMetaObjectBinder,System.Collections.Generic.IEnumerable`1[[System.Linq.Expressions.Expression, System.Linq.Expressions, Version=5.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]],Fallback[Int32]):System.Dynamic.DynamicMetaObject:this         -60 (-10.08% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.Utilities.DynamicProxyMetaObject`1[Double][System.Double]:CallMethodReturnLast(System.String,System.Dynamic.DynamicMetaObjectBinder,System.Collections.Generic.IEnumerable`1[[System.Linq.Expressions.Expression, System.Linq.Expressions, Version=5.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]],Fallback[Double]):System.Dynamic.DynamicMetaObject:this         -60 (-10.08% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.Utilities.DynamicProxyMetaObject`1[Vector`1][System.Numerics.Vector`1[System.Single]]:CallMethodReturnLast(System.String,System.Dynamic.DynamicMetaObjectBinder,System.Collections.Generic.IEnumerable`1[[System.Linq.Expressions.Expression, System.Linq.Expressions, Version=5.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]],Fallback[Vector`1]):System.Dynamic.DynamicMetaObject:this         -60 (-10.08% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.Utilities.DynamicProxyMetaObject`1[Int64][System.Int64]:CallMethodReturnLast(System.String,System.Dynamic.DynamicMetaObjectBinder,System.Collections.Generic.IEnumerable`1[[System.Linq.Expressions.Expression, System.Linq.Expressions, Version=5.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]],Fallback[Int64]):System.Dynamic.DynamicMetaObject:this         -60 (-9.19% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.Utilities.DynamicProxyMetaObject`1[__Canon][System.__Canon]:CallMethodReturnLast(System.String,System.Dynamic.DynamicMetaObjectBinder,System.Collections.Generic.IEnumerable`1[[System.Linq.Expressions.Expression, System.Linq.Expressions, Version=5.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]],Fallback[__Canon]):System.Dynamic.DynamicMetaObject:this         -84 (-8.70% of base) : System.ComponentModel.Composition.dasm - System.ComponentModel.Composition.Hosting.CompositionServices:GetPartMetadataForType(System.Type,int):System.Collections.Generic.IDictionary`2[[System.String, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.Object, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]]         -30 (-8.29% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.Schema.JsonSchemaBuilder:ProcessExtends(Newtonsoft.Json.Linq.JToken):this         -26 (-5.74% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - Microsoft.Diagnostics.Tracing.Etlx.TraceProcesses:ToString():System.String:this         -26 (-5.74% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - Microsoft.Diagnostics.Tracing.Etlx.TraceThreads:ToString():System.String:this         -26 (-5.74% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - Microsoft.Diagnostics.Tracing.Etlx.TraceModuleFiles:ToString():System.String:this         -20 (-5.49% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax.SyntaxNodeExtensions:CreateSkippedTrivia(Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax.VisualBasicSyntaxNode,bool,bool,Microsoft.CodeAnalysis.DiagnosticInfo):Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax.SyntaxList`1[[Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax.VisualBasicSyntaxNode, Microsoft.CodeAnalysis.VisualBasic, Version=1.1.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35]]         -10 (-5.41% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.Converters.ExpandoObjectConverter:ReadList(Newtonsoft.Json.JsonReader):System.Object:this         -22 (-4.94% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - Microsoft.Diagnostics.Tracing.Analysis.TraceProcesses:ToString():System.String:this         -20 (-4.78% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.Utilities.DynamicProxyMetaObject`1[Byte][System.Byte]:CallMethodNoResult(System.String,System.Dynamic.DynamicMetaObjectBinder,System.Linq.Expressions.Expression[],Fallback[Byte]):System.Dynamic.DynamicMetaObject:this         -20 (-4.78% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.Utilities.DynamicProxyMetaObject`1[Int16][System.Int16]:CallMethodNoResult(System.String,System.Dynamic.DynamicMetaObjectBinder,System.Linq.Expressions.Expression[],Fallback[Int16]):System.Dynamic.DynamicMetaObject:this         -20 (-4.78% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.Utilities.DynamicProxyMetaObject`1[Int32][System.Int32]:CallMethodNoResult(System.String,System.Dynamic.DynamicMetaObjectBinder,System.Linq.Expressions.Expression[],Fallback[Int32]):System.Dynamic.DynamicMetaObject:this         -20 (-4.78% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.Utilities.DynamicProxyMetaObject`1[Double][System.Double]:CallMethodNoResult(System.String,System.Dynamic.DynamicMetaObjectBinder,System.Linq.Expressions.Expression[],Fallback[Double]):System.Dynamic.DynamicMetaObject:this         -20 (-4.78% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.Utilities.DynamicProxyMetaObject`1[Vector`1][System.Numerics.Vector`1[System.Single]]:CallMethodNoResult(System.String,System.Dynamic.DynamicMetaObjectBinder,System.Linq.Expressions.Expression[],Fallback[Vector`1]):System.Dynamic.DynamicMetaObject:this64 total methods with Code Size differences (64 improved, 0 regressed), 340865 unchanged.Completed analysis in 34.54s

All these improvements are due to#43668 (comment)
Newtonsoft.Json.JsonValidatingReader:get_CurrentMemberSchemas()
Newtonsoft.Json.Utilities.DynamicProxyMetaObject`1[__Canon][System.__Canon]:CallMethodReturnLast
Newtonsoft.Json.Utilities.DynamicProxyMetaObject`1[Byte][System.Byte]:CallMethodReturnLast same diff we got for Int32, Vector etc
Newtonsoft.Json.Utilities.ExpressionReflectionDelegateFactory:BuildMethodCall
Microsoft.CodeAnalysis.CSharp.CSharpCommandLineParser:Parse
System.ComponentModel.Composition.Hosting.CompositionServices:GetPartMetadataForType

@Rattenkrieg
Copy link
ContributorAuthor

@davidwrighton

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.).

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.

@RattenkriegRattenkriegforce-pushed thedefault-interface-method-virtualization branch fromc751e71 tof145018CompareNovember 3, 2020 13:18
@Rattenkrieg
Copy link
ContributorAuthor

Here are benchmarks made from pushed tests. Ran with

COMPlus_TieredCompilation=0 python ./scripts/benchmarks_ci.py --frameworks net6.0 \--filter 'Devirtualization*' \                              --corerun \~/projs/runtime/artifacts/bin/testhost/net6.0-Linux-Release-x64/shared/Microsoft.NETCore.App/6.0.0/corerun \~/projs/patched/artifacts/bin/testhost/net6.0-Linux-Release-x64/shared/Microsoft.NETCore.App/6.0.0/corerun
results
BenchmarkDotNet=v0.12.1.1448-nightly,OS=fedora 33Intel Core i7-8700 CPU 3.20GHz (Coffee Lake), 1 CPU, 12 logical and 6 physical cores.NETSDK=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 RyuJITPowerPlanMode=00000000-0000-0000-0000-000000000000Arguments=/p:DebugType=portableIterationTime=250.0000 msMaxIterationCount=20MinIterationCount=15WarmupCount=1
MethodJobToolchainMeanErrorStdDevMedianMinMaxRatioGen 0Gen 1Gen 2Allocated
DevirttestStructsJob-UOOGDK/patched/artifacts/bin/testhost/net6.0-Linux-Release-x64/shared/Microsoft.NETCore.App/6.0.0/corerun33.96 ns0.199 ns0.186 ns33.89 ns33.74 ns34.27 ns0.800.0076--48 B
DevirttestStructsJob-ENJNBQ/runtime/artifacts/bin/testhost/net6.0-Linux-Release-x64/shared/Microsoft.NETCore.App/6.0.0/corerun42.51 ns0.294 ns0.260 ns42.53 ns41.85 ns42.81 ns1.000.0114--72 B
MethodJobToolchainMeanErrorStdDevMedianMinMaxRatioGen 0Gen 1Gen 2Allocated
DevirttestSingleVarianceJob-DTZMLL/patched/artifacts/bin/testhost/net6.0-Linux-Release-x64/shared/Microsoft.NETCore.App/6.0.0/corerun16.00 ns0.111 ns0.099 ns15.99 ns15.80 ns16.17 ns0.900.0038--24 B
DevirttestSingleVarianceJob-ZGIIZQ/runtime/artifacts/bin/testhost/net6.0-Linux-Release-x64/shared/Microsoft.NETCore.App/6.0.0/corerun17.75 ns0.146 ns0.137 ns17.70 ns17.59 ns18.00 ns1.000.0038--24 B
MethodJobToolchainMeanErrorStdDevMedianMinMaxRatioRatioSDGen 0Gen 1Gen 2Allocated
DevirttestSingleValueTypeNoDimJob-WIRPMH/patched/artifacts/bin/testhost/net6.0-Linux-Release-x64/shared/Microsoft.NETCore.App/6.0.0/corerun33.72 ns0.316 ns0.264 ns33.75 ns33.22 ns34.15 ns0.980.020.0063--40 B
DevirttestSingleValueTypeNoDimJob-THTLQV/runtime/artifacts/bin/testhost/net6.0-Linux-Release-x64/shared/Microsoft.NETCore.App/6.0.0/corerun34.38 ns0.711 ns0.594 ns34.46 ns33.32 ns35.48 ns1.000.000.0101--64 B
MethodJobToolchainMeanErrorStdDevMedianMinMaxRatioGen 0Gen 1Gen 2Allocated
DevirttestSingleOverStructJob-MNZJNQ/patched/artifacts/bin/testhost/net6.0-Linux-Release-x64/shared/Microsoft.NETCore.App/6.0.0/corerun11.73 ns0.127 ns0.119 ns11.67 ns11.58 ns12.00 ns0.69----
DevirttestSingleOverStructJob-HBBMYE/runtime/artifacts/bin/testhost/net6.0-Linux-Release-x64/shared/Microsoft.NETCore.App/6.0.0/corerun16.95 ns0.173 ns0.144 ns16.91 ns16.78 ns17.31 ns1.000.0038--24 B
MethodJobToolchainMeanErrorStdDevMedianMinMaxRatioGen 0Gen 1Gen 2Allocated
DevirttestSingleOverridingGenericJob-GJCFYK/patched/artifacts/bin/testhost/net6.0-Linux-Release-x64/shared/Microsoft.NETCore.App/6.0.0/corerun32.94 ns0.147 ns0.123 ns32.96 ns32.65 ns33.14 ns1.020.0101--64 B
DevirttestSingleOverridingGenericJob-AFELWE/runtime/artifacts/bin/testhost/net6.0-Linux-Release-x64/shared/Microsoft.NETCore.App/6.0.0/corerun32.42 ns0.242 ns0.227 ns32.41 ns32.00 ns32.78 ns1.000.0101--64 B
MethodJobToolchainMeanErrorStdDevMedianMinMaxRatioGen 0Gen 1Gen 2Allocated
DevirttestSingleOverridingJob-ORLOJQ/patched/artifacts/bin/testhost/net6.0-Linux-Release-x64/shared/Microsoft.NETCore.App/6.0.0/corerun1.264 ns0.0214 ns0.0200 ns1.252 ns1.245 ns1.308 ns0.18----
DevirttestSingleOverridingJob-UUMXEC/runtime/artifacts/bin/testhost/net6.0-Linux-Release-x64/shared/Microsoft.NETCore.App/6.0.0/corerun7.021 ns0.0767 ns0.0718 ns7.009 ns6.928 ns7.148 ns1.000.0038--24 B
MethodJobToolchainMeanErrorStdDevMedianMinMaxRatioGen 0Gen 1Gen 2Allocated
DevirttestInlineJob-MDETMM/patched/artifacts/bin/testhost/net6.0-Linux-Release-x64/shared/Microsoft.NETCore.App/6.0.0/corerun7.795 ns0.0910 ns0.0851 ns7.795 ns7.606 ns7.934 ns0.530.0051--32 B
DevirttestInlineJob-FIUGOW/runtime/artifacts/bin/testhost/net6.0-Linux-Release-x64/shared/Microsoft.NETCore.App/6.0.0/corerun14.603 ns0.0807 ns0.0715 ns14.608 ns14.483 ns14.714 ns1.000.0089--56 B
MethodJobToolchainMeanErrorStdDevMedianMinMaxRatioGen 0Gen 1Gen 2Allocated
DevirttestJob-UVTLEC/patched/artifacts/bin/testhost/net6.0-Linux-Release-x64/shared/Microsoft.NETCore.App/6.0.0/corerun30.32 ns0.176 ns0.147 ns30.31 ns30.08 ns30.65 ns0.880.0037--24 B
DevirttestJob-VZGJXJ/runtime/artifacts/bin/testhost/net6.0-Linux-Release-x64/shared/Microsoft.NETCore.App/6.0.0/corerun34.31 ns0.239 ns0.212 ns34.34 ns34.03 ns34.63 ns1.000.0038--24 B
MethodJobToolchainMeanErrorStdDevMedianMinMaxRatioGen 0Gen 1Gen 2Allocated
Devirttest39419Job-DPIOZQ/patched/artifacts/bin/testhost/net6.0-Linux-Release-x64/shared/Microsoft.NETCore.App/6.0.0/corerun0.0057 ns0.0068 ns0.0064 ns0.0028 ns0.0000 ns0.0213 ns0.001----
Devirttest39419Job-SBGEWN/runtime/artifacts/bin/testhost/net6.0-Linux-Release-x64/shared/Microsoft.NETCore.App/6.0.0/corerun5.4767 ns0.0385 ns0.0360 ns5.4802 ns5.4021 ns5.5348 ns1.0000.0038--24 B

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Be advised, although I'm not 100% sure, same below.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Agree, thanks for catching this.

@Rattenkrieg
Copy link
ContributorAuthor

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.

@Rattenkrieg
Copy link
ContributorAuthor

Rattenkrieg commentedNov 28, 2020
edited
Loading

Format is failing on CI
clang-tidy not found here: https://clrjit.blob.core.windows.net/clang-tools/centos.7-x64/clang-tidyEither clang-tidy or clang-format was not installed. Please install and put them on the PATH to use jit-format.Tools can be found at http://llvm.org/releases/download.html#3.8.0ERROR: Failed to download clang-format and clang-tidy.('Downloading', 'https://raw.githubusercontent.com/dotnet/jitutils/master/bootstrap.sh', 'to', '/tmp/tmpWMFNjA/bootstrap.sh')Making bootstrap executable('Running:', 'bash', '/tmp/tmpWMFNjA/bootstrap.sh')Deleting /tmp/tmpWMFNjA/bootstrap.shBootstrap failed
Same on my machine
Installing version 3.8 of clang toolsclang-format not found here: https://clrjit.blob.core.windows.net/clang-tools/fedora.33-x64/clang-formatclang-tidy not found here: https://clrjit.blob.core.windows.net/clang-tools/fedora.33-x64/clang-tidyEither clang-tidy or clang-format was not installed. Please install and put them on the PATH to use jit-format.Tools can be found at http://llvm.org/releases/download.html#3.8.0ERROR: Failed to download clang-format and clang-tidy.Deleting /tmp/tmpsontbc74/bootstrap.shBootstrap failed
Windows
PS C:\Users\sergey\source\repos\runtime> python .\src\tests\Common\scripts\format.py -c .\src\coreclr -o Windows -a x64Downloading https://raw.githubusercontent.com/dotnet/jitutils/master/ to C:\Users\sergey\AppData\Local\Temp\tmpbhz2gbc2\Traceback (most recent call last):  File ".\src\tests\Common\scripts\format.py", line 248, in <module>    return_code = main(sys.argv[1:])  File ".\src\tests\Common\scripts\format.py", line 129, in main    urlretrieve(bootstrapUrl, bootstrapPath)  File "C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.8_3.8.1776.0_x64__qbz5n2kfra8p0\lib\urllib\request.py", line 247, in urlretrieve    with contextlib.closing(urlopen(url, data)) as fp:  File "C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.8_3.8.1776.0_x64__qbz5n2kfra8p0\lib\urllib\request.py", line 222, in urlopen    return opener.open(url, data, timeout)  File "C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.8_3.8.1776.0_x64__qbz5n2kfra8p0\lib\urllib\request.py", line 531, in open    response = meth(req, response)  File "C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.8_3.8.1776.0_x64__qbz5n2kfra8p0\lib\urllib\request.py", line 640, in http_response    response = self.parent.error(  File "C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.8_3.8.1776.0_x64__qbz5n2kfra8p0\lib\urllib\request.py", line 569, in error    return self._call_chain(*args)  File "C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.8_3.8.1776.0_x64__qbz5n2kfra8p0\lib\urllib\request.py", line 502, in _call_chain    result = func(*args)  File "C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.8_3.8.1776.0_x64__qbz5n2kfra8p0\lib\urllib\request.py", line 649, in http_error_default    raise HTTPError(req.full_url, code, msg, hdrs, fp)urllib.error.HTTPError: HTTP Error 400: Bad Request

@RattenkriegRattenkriegforce-pushed thedefault-interface-method-virtualization branch from5fe23e4 toe91c55bCompareDecember 12, 2020 20:12
@Rattenkrieg
Copy link
ContributorAuthor

@davidwrighton@AndyAyersMS PTAL
There is bunch of tests which probably have to be merged/cleaned up. Also I've marked some existing comments in code which left me in doubt, looking for your feedback there as well.

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:

interfaceI{stringDefaultTypeOf<T>()=>typeof(T).Name;}classC:I{}publicstaticclassProgram{[MethodImpl(MethodImplOptions.AggressiveOptimization)]staticintMain(){Ic=newC();vardcs=c.DefaultTypeOf();if(dcs!="Object")return200;return100;}}

produces

IN0001:000008movrdi,0x12293E518IN0002:000012call     CORINFO_HELP_NEWSFASTIN0003:000017movrbx,raxIN0004: 00001Amovrdi,rbxIN0005: 00001Dcall     System.Object:.ctor():thisIN0006:000022movrdi,rbxIN0007:000025movrsi,0x12293E3C8IN0008: 00002Fmovrdx,0x12293E718IN0009:000039call     CORINFO_HELP_VIRTUAL_FUNC_PTRIN000a: 00003Emovrdi,rbxIN000b:000041callraxIN000c:000043movrdi,raxIN000d:000046movrsi,0x19B2371F0IN000e:000050movrsi, gword ptr[rsi]IN000f:000053call     System.String:op_Inequality(System.String,System.String):boolIN0010:000058testeax,eaxIN0011: 00005Aje       SHORT G_M24375_IG05

If we remove DIM and let class implement interface asm stays the same. However if we remove interface, method inlines perfectly:

classC{publicstringDefaultTypeOf<T>()=>typeof(T).Name;}
IN0001:000004movrdi,0x11CFFE3F8IN0002: 00000Ecall     CORINFO_HELP_NEWSFASTIN0003:000013movrdi,raxIN0004:000016call     System.Object:.ctor():thisIN0005:00001Bmovrdi,0x11CDA4388IN0006:000025call     CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPEIN0007: 00002Amovrdi,raxIN0008: 00002Dmovrax,0x11CC5A5F0IN0009:000037call     gword ptr[rax]System.RuntimeType:get_Name():System.String:thisIN000a:000039movrdi,raxIN000b: 00003Cmovrsi,0x19D8FD1F0IN000c:000046movrsi, gword ptr[rsi]IN000d:000049call     System.String:op_Inequality(System.String,System.String):boolIN000e: 00004Etesteax,eaxIN000f:000050je       SHORT G_M24375_IG05

But only for one generic param:

classC{publicstringDefaultTypeOf<T1,T2>()=>typeof(T1).Name+typeof(T2).Name;}
IN0001:000008movrdi,0x10C68E3F8IN0002:000012call     CORINFO_HELP_NEWSFASTIN0003:000017movrbx,raxIN0004: 00001Amovrdi,rbxIN0005: 00001Dcall     System.Object:.ctor():thisIN0006:000022movrdi,rbxIN0007:000025movrsi,0x10C68E608IN0008: 00002Fcall     C:DefaultTypeOf():System.String:thisIN0009:000034movrdi,raxIN000a:000037movrsi,0x194F911F0IN000b:000041movrsi, gword ptr[rsi]IN000c:000044call     System.String:op_Inequality(System.String,System.String):boolIN000d:000049testeax,eaxIN000e: 00004Bje       SHORT G_M24375_IG05

Have we ever considered optimizing such case?

@AndyAyersMS
Copy link
Member

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.

I believe all generic virtual methods are handled via theldvirtftn / calli path and that is not yet hooked into devirtualization.

I had been thinking it wouldn't be worth addressing until after the jit is able propagate methods intocallis and turn those into direct (and inlineable) calls, but the cost of the lookup is high enough that perhaps it's worth optimizing just that part.

@Rattenkrieg
Copy link
ContributorAuthor

Hey@davidwrighton is there still an interest from dotnet team/you in this PR?

@karelzkarelz removed the request for review froma teamFebruary 2, 2021 14:49
publicstaticclassProgram
{
[MethodImpl(MethodImplOptions.AggressiveOptimization)]
staticintMain()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 @@
usingSystem;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

u [](start = 0, length = 1)

All of the test files need the copyright attribution comment.


MethodDesc*GetMethodDescForInterfaceMethod(TypeHandleownerType,MethodDesc*pInterfaceMD,BOOLthrowOnConflict);
MethodDesc*GetMethodDescForInterfaceMethod(MethodDesc*pInterfaceMD,BOOLthrowOnConflict);// 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

// 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Words are hard, and copy/pasting is easy. The "but is not itself generic" is just wrong and should be deleted from this comment.

// 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

method table [](start = 27, length = 12)

This should read "MethodDesc"

MethodTable* pOwnerMT = OwnerClsHnd.GetMethodTable();
pOwnerMT = OwnerClsHnd.GetMethodTable();

if (!canCastStraightForward && !(pOwnerMT->IsInterface() && pObjMT->CanCastToInterface(pOwnerMT)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

What scenario are you addressing here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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)

false);

// Shared generic or static per-inst methods andshared methods on generic structs
// Shared generic or static per-inst methods,shared methods on generic structs and default interface methods
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This is a good comment change.

@davidwrighton
Copy link
Member

@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

  1. Testing. This sort of thing is always difficult to get right. I think your tests are pretty good now, we you should have a couple tests showing calls from shared generic code itself.
  2. I am not qualified to review the JIT changes. You'll need to get@AndyAyersMS or someone he delegates to review them.
  3. Crossgen2 changes. I find those to be of particular interest. As Crossgen2 doesn't currently actually handle default interface methods, I want to know how you found the problems here, and if they are problems without your JIT changes.

@danmoseley
Copy link
Member

Assigning a shepherd for this PR, feel free to change.

Base automatically changed frommaster tomainMarch 1, 2021 09:07
@davidwrighton
Copy link
Member

@Rattenkrieg Are you able to answer my questions? If so, please let us know, as otherwise we'll be closing this PR soon.

@Rattenkrieg
Copy link
ContributorAuthor

Rattenkrieg commentedMar 23, 2021 via email

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'veaccidentally merged tons of files from main while ago and github isoverwhelmed 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 isto resubmit changes with new PR and I would gladly ask you to state yourquestions 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> .

@lambdageek
Copy link
Member

lambdageek commentedMar 23, 2021
edited
Loading

@Rattenkrieg you can append?timeline_per_page=10 to the page URL to get it to open in a browser.https://github.com/dotnet/runtime/pull/43668?timeline_per_page=10#43668

Rattenkrieg reacted with heart emoji

@Rattenkrieg
Copy link
ContributorAuthor

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.

@mangod9
Copy link
Member

@Rattenkrieg can this PR be closed since you are hoping to open a new one after addressing feedback?

@Rattenkrieg
Copy link
ContributorAuthor

@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.

@mangod9
Copy link
Member

Thanks for the update!

@karelzkarelz added this to the6.0.0 milestoneMay 20, 2021
@ghostghost locked asresolvedand limited conversation to collaboratorsJun 19, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@AndyAyersMSAndyAyersMSAndyAyersMS left review comments

@davidwrightondavidwrightondavidwrighton left review comments

@marek-safarmarek-safarAwaiting requested review from marek-safar

@ahsonkhanahsonkhanAwaiting requested review from ahsonkhan

@akoeplingerakoeplingerAwaiting requested review from akoeplinger

@BrzVladBrzVladAwaiting requested review from BrzVlad

@CoffeeFluxCoffeeFluxAwaiting requested review from CoffeeFlux

@EgorBoEgorBoAwaiting requested review from EgorBo

@eiriktsarpaliseiriktsarpalisAwaiting requested review from eiriktsarpalis

@imhameedimhameedAwaiting requested review from imhameed

@jozkeejozkeeAwaiting requested review from jozkee

@lambdageeklambdageekAwaiting requested review from lambdageek

@lateralusXlateralusXAwaiting requested review from lateralusX

@layomialayomiaAwaiting requested review from layomia

@nariccnariccAwaiting requested review from naricc

@SamMonoRTSamMonoRTAwaiting requested review from SamMonoRT

@stevehartersteveharterAwaiting requested review from steveharter

@steveisoksteveisokAwaiting requested review from steveisok

@thaystgthaystgAwaiting requested review from thaystg

@vargazvargazAwaiting requested review from vargaz

+1 more reviewer

@sandreenkosandreenkosandreenko left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@davidwrightondavidwrighton

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

[Default interfaces] Support for default interface method devirtualization

11 participants

@Rattenkrieg@MichalStrehovsky@davidwrighton@AndyAyersMS@ViktorHofer@danmoseley@lambdageek@mangod9@sandreenko@karelz@Dotnet-GitSync-Bot

[8]ページ先頭

©2009-2025 Movatter.jp