Update jit for new likely class records #51664

AndyAyersMS · 2021-04-22T03:03:11Z

Fix the jit to consume the new "LikelyClass" records seen from crossgen-processed PGO data.

…sic block data

…cords Update jit for new likely class records

AndyAyersMS · 2021-04-22T03:10:27Z

cc @dotnet/jit-contrib

Unfortunately assessing diffs here is a bit tricky... PMI doesn't see this profile data (because we disable R2R for PMI) and SPMI does not have the method context info for the methods that would have diffs.

Using the latter we can ballpark how many methods would have diffs; it looks like maybe 2000 or so across all the collections.

I am going to hack PMI to not disable R2R and see if we see anything useful there. Also will try crossgen2 based PMI.

AndyAyersMS · 2021-04-22T03:14:37Z

See #51643 for context.

AndyAyersMS · 2021-04-22T03:24:11Z

Here are the R2R-enabled PMI diffs.

PMI CodeSize Diffs for System.Private.CoreLib.dll, framework assemblies for x64 default jit

Summary of Code Size diffs:
(Lower is better)

Total bytes of base: 50659241
Total bytes of diff: 50741806
Total bytes of delta: 82565 (0.16% of base)
    diff is a regression.


Top file regressions (bytes):
       12879 : System.Linq.Expressions.dasm (1.66% of base)
        7587 : System.Private.DataContractSerialization.dasm (1.00% of base)
        7378 : Microsoft.VisualBasic.Core.dasm (1.54% of base)
        6797 : System.Private.Xml.dasm (0.19% of base)
        4781 : Newtonsoft.Json.dasm (0.55% of base)
        4085 : xunit.execution.dotnet.dasm (1.71% of base)
        2664 : Microsoft.CSharp.dasm (0.71% of base)
        2520 : System.Text.Json.dasm (0.31% of base)
        2345 : Microsoft.CodeAnalysis.dasm (0.13% of base)
        2201 : FSharp.Core.dasm (0.06% of base)
        2010 : xunit.assert.dasm (1.46% of base)
        1941 : xunit.core.dasm (2.65% of base)
        1706 : System.ComponentModel.TypeConverter.dasm (0.62% of base)
        1513 : System.Reflection.MetadataLoadContext.dasm (0.85% of base)
        1480 : System.Collections.Concurrent.dasm (0.38% of base)
        1470 : System.Composition.Hosting.dasm (1.69% of base)
        1297 : System.ComponentModel.Composition.dasm (0.38% of base)
        1236 : Microsoft.Diagnostics.Tracing.TraceEvent.dasm (0.04% of base)
         986 : System.Runtime.Serialization.Formatters.dasm (1.01% of base)
         914 : System.IO.Compression.dasm (1.20% of base)

Top file improvements (bytes):
        -304 : System.Security.Permissions.dasm (-2.00% of base)
         -72 : System.Diagnostics.Process.dasm (-0.09% of base)

83 total files with Code Size differences (2 improved, 81 regressed), 188 unchanged.

Top method regressions (bytes):
        1480 (203.30% of base) : System.Collections.Concurrent.dasm - BlockingCollection`1:Dispose(bool):this (8 methods)
        1404 (51.32% of base) : System.Private.Xml.dasm - XmlILStorageMethods:.ctor(Type):this
        1110 ( 8.09% of base) : System.Linq.Expressions.dasm - CallInstruction:FastCreate(MethodInfo,ref):CallInstruction (17 methods)
         888 (71.61% of base) : System.Composition.Hosting.dasm - <>c__1`1:<GetMetadataViewProvider>b__1_3(PropertyInfo):bool:this (8 methods)
         860 (10.34% of base) : System.Text.Json.dasm - JsonPropertyInfo`1:Initialize(Type,Type,Type,ubyte,MemberInfo,JsonConverter,Nullable`1,Nullable`1,JsonSerializerOptions):this (8 methods)
         721 ( 5.07% of base) : Microsoft.VisualBasic.Core.dasm - VBBinder:BindToMethod(int,ref,byref,ref,CultureInfo,ref,byref):MethodBase:this
         573 (29.81% of base) : Microsoft.VisualBasic.Core.dasm - VBBinder:GetMethodsByName(Type,IReflect,String,int):ref:this
         526 (12.62% of base) : System.Private.DataContractSerialization.dasm - CollectionDataContract:IsCollectionOrTryCreate(Type,bool,byref,byref,bool):bool
         493 (88.51% of base) : System.Linq.Expressions.dasm - TypeUtils:StrictHasReferenceConversionTo(Type,Type,bool):bool
         474 (45.27% of base) : System.Reflection.MetadataLoadContext.dasm - Assignability:CanCastTo(Type,Type,CoreTypes):bool
         472 (280.95% of base) : System.Private.Xml.dasm - <>c__DisplayClass1_0`2:<GetSetMemberValueDelegateWithType>b__0(Object,Object):this (8 methods)
         472 (280.95% of base) : System.Private.Xml.dasm - <>c__DisplayClass1_2`2:<GetSetMemberValueDelegateWithType>b__3(Object,Object):this (8 methods)
         464 (24.91% of base) : Microsoft.CodeAnalysis.dasm - Win32ResourceConversions:AppendIconToResourceStream(Stream,Stream)
         459 ( 9.59% of base) : Newtonsoft.Json.dasm - ExpressionReflectionDelegateFactory:CreateSet(PropertyInfo):Action`2:this (8 methods)
         436 (14.73% of base) : Newtonsoft.Json.dasm - ReflectionDelegateFactory:CreateGet(MemberInfo):Func`2:this (8 methods)
         408 (12.24% of base) : xunit.assert.dasm - TypeErasedEqualityComparer:Equals(Object,Object):bool:this (8 methods)
         408 (12.24% of base) : xunit.core.dasm - TypeErasedEqualityComparer:Equals(Object,Object):bool:this (8 methods)
         408 (12.24% of base) : xunit.execution.dotnet.dasm - TypeErasedEqualityComparer:Equals(Object,Object):bool:this (8 methods)
         376 (28.66% of base) : System.Private.DataContractSerialization.dasm - <>c__DisplayClass41_2`1:<GetCollectionSetItemDelegate>b__3(Object,Object,int):Object:this (8 methods)
         376 (47.47% of base) : System.Private.DataContractSerialization.dasm - <>c__DisplayClass41_3`1:<GetCollectionSetItemDelegate>b__7(Object,Object,int):Object:this (8 methods)

Top method improvements (bytes):
        -194 (-33.16% of base) : Microsoft.Extensions.DependencyInjection.dasm - ExpressionResolverBuilder:VisitConstructor(ConstructorCallSite,Object):Expression:this
        -187 (-27.91% of base) : System.Linq.Expressions.dasm - TypeUtils:IsLegalExplicitVariantDelegateConversion(Type,Type):bool
        -155 (-31.38% of base) : System.Reflection.MetadataLoadContext.dasm - PropertyPolicies:IsSuppressedByMoreDerivedMember(PropertyInfo,ref,int,int):bool:this
        -144 (-11.73% of base) : System.Private.Xml.dasm - XsltCompileContext:FindBestMethod(ref,bool,bool,String,ref):MethodInfo:this
        -124 (-12.50% of base) : System.Diagnostics.Process.dasm - Process:KillTree(SafeProcessHandle):List`1:this
         -36 (-1.27% of base) : System.Net.Http.dasm - <SendAsync>d__4:MoveNext():this
         -36 (-6.72% of base) : System.Data.Common.dasm - AggregateNode:Bind(DataTable,List`1):this
         -24 (-5.39% of base) : System.Data.Common.dasm - LookupNode:Bind(DataTable,List`1):this
         -24 (-1.38% of base) : System.Data.Common.dasm - UnaryNode:EvalUnaryOp(int,Object):Object:this
         -24 (-3.97% of base) : System.Data.Common.dasm - AggregateNode:Eval(DataRow,int):Object:this
         -24 (-20.87% of base) : System.Data.Common.dasm - AggregateNode:Eval(ref):Object:this
         -19 (-1.79% of base) : System.Security.Cryptography.Algorithms.dasm - ECDiffieHellmanCng:DeriveSecretAgreementHandle(ECDiffieHellmanPublicKey):SafeNCryptSecretHandle:this
         -16 (-61.54% of base) : System.Security.Permissions.dasm - AllMembershipCondition:ToString():String:this
         -16 (-61.54% of base) : System.Security.Permissions.dasm - ApplicationDirectory:ToString():String:this
         -16 (-61.54% of base) : System.Security.Permissions.dasm - ApplicationDirectoryMembershipCondition:ToString():String:this
         -16 (-61.54% of base) : System.Security.Permissions.dasm - GacInstalled:ToString():String:this
         -16 (-61.54% of base) : System.Security.Permissions.dasm - GacMembershipCondition:ToString():String:this
         -16 (-61.54% of base) : System.Security.Permissions.dasm - Hash:ToString():String:this
         -16 (-61.54% of base) : System.Security.Permissions.dasm - HashMembershipCondition:ToString():String:this
         -16 (-61.54% of base) : System.Security.Permissions.dasm - PermissionRequestEvidence:ToString():String:this

Top method regressions (percentages):
          57 (407.14% of base) : Newtonsoft.Json.dasm - TypeExtensions:IsPrimitive(Type):bool
          70 (388.89% of base) : System.Reflection.Context.dasm - DelegatingType:HasElementTypeImpl():bool:this
          60 (333.33% of base) : System.Reflection.Context.dasm - DelegatingType:IsArrayImpl():bool:this
          57 (316.67% of base) : System.Reflection.Context.dasm - DelegatingType:IsPrimitiveImpl():bool:this
          55 (305.56% of base) : Microsoft.CodeAnalysis.dasm - TempFileStream:DisposeUnderlyingStream():this
          55 (305.56% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - CtfMetadataLegacyParser:System.IDisposable.Dispose():this
         472 (280.95% of base) : System.Private.Xml.dasm - <>c__DisplayClass1_0`2:<GetSetMemberValueDelegateWithType>b__0(Object,Object):this (8 methods)
         472 (280.95% of base) : System.Private.Xml.dasm - <>c__DisplayClass1_2`2:<GetSetMemberValueDelegateWithType>b__3(Object,Object):this (8 methods)
          60 (260.87% of base) : System.IO.Compression.dasm - PositionPreservingWriteOnlyStreamWrapper:Dispose(bool):this
          60 (260.87% of base) : System.IO.Packaging.dasm - ZipWrappingStream:Dispose(bool):this
          60 (260.87% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - CtfMetadataLegacyParser:Dispose(bool):this
          60 (250.00% of base) : System.IO.Compression.dasm - ZipArchiveEntry:CloseStreams():this
          45 (250.00% of base) : System.Reflection.Context.dasm - DelegatingType:IsByRefImpl():bool:this
          45 (250.00% of base) : System.Reflection.Context.dasm - DelegatingType:IsPointerImpl():bool:this
          57 (247.83% of base) : Newtonsoft.Json.Bson.dasm - AsyncBinaryWriterOwningWriter:Dispose(bool):this
         194 (204.21% of base) : System.Reflection.TypeExtensions.dasm - TypeExtensions:GetMethods(Type):ref
        1480 (203.30% of base) : System.Collections.Concurrent.dasm - BlockingCollection`1:Dispose(bool):this (8 methods)
         133 (201.52% of base) : FSharp.Core.dasm - LeafExpressionConverter:isNamedType(Type):bool
         133 (201.52% of base) : FSharp.Core.dasm - ReflectUtils:isNamedType(Type):bool
         133 (201.52% of base) : FSharp.Core.dasm - Impl:isNamedType(Type):bool

Top method improvements (percentages):
         -16 (-61.54% of base) : System.Security.Permissions.dasm - AllMembershipCondition:ToString():String:this
         -16 (-61.54% of base) : System.Security.Permissions.dasm - ApplicationDirectory:ToString():String:this
         -16 (-61.54% of base) : System.Security.Permissions.dasm - ApplicationDirectoryMembershipCondition:ToString():String:this
         -16 (-61.54% of base) : System.Security.Permissions.dasm - GacInstalled:ToString():String:this
         -16 (-61.54% of base) : System.Security.Permissions.dasm - GacMembershipCondition:ToString():String:this
         -16 (-61.54% of base) : System.Security.Permissions.dasm - Hash:ToString():String:this
         -16 (-61.54% of base) : System.Security.Permissions.dasm - HashMembershipCondition:ToString():String:this
         -16 (-61.54% of base) : System.Security.Permissions.dasm - PermissionRequestEvidence:ToString():String:this
         -16 (-61.54% of base) : System.Security.Permissions.dasm - Publisher:ToString():String:this
         -16 (-61.54% of base) : System.Security.Permissions.dasm - PublisherMembershipCondition:ToString():String:this
         -16 (-61.54% of base) : System.Security.Permissions.dasm - Site:ToString():String:this
         -16 (-61.54% of base) : System.Security.Permissions.dasm - SiteMembershipCondition:ToString():String:this
         -16 (-61.54% of base) : System.Security.Permissions.dasm - StrongName:ToString():String:this
         -16 (-61.54% of base) : System.Security.Permissions.dasm - StrongNameMembershipCondition:ToString():String:this
         -16 (-61.54% of base) : System.Security.Permissions.dasm - Url:ToString():String:this
         -16 (-61.54% of base) : System.Security.Permissions.dasm - UrlMembershipCondition:ToString():String:this
         -16 (-61.54% of base) : System.Security.Permissions.dasm - Zone:ToString():String:this
         -16 (-61.54% of base) : System.Security.Permissions.dasm - ZoneMembershipCondition:ToString():String:this
         -16 (-61.54% of base) : System.Security.Permissions.dasm - StrongNamePublicKeyBlob:ToString():String:this
        -194 (-33.16% of base) : Microsoft.Extensions.DependencyInjection.dasm - ExpressionResolverBuilder:VisitConstructor(ConstructorCallSite,Object):Expression:this

1093 total methods with Code Size differences (41 improved, 1052 regressed), 250390 unchanged.

AndyAyersMS · 2021-04-22T03:25:32Z

Sample regression diff (showing GDV kicking in and enabling some inlining):

before

; Assembly listing for method TypeExtensions:IsPrimitive(Type):bool
; Emitting BLENDED_CODE for X64 CPU with AVX - Windows
; optimized code
; rsp based frame
; fully interruptible
; No PGO data
; 1 inlinees with PGO data; 0 single block inlinees; 0 inlinees without PGO data
; Final local variable assignments
;
;  V00 arg0         [V00,T00] (  4,  4   )     ref  ->  rcx         class-hnd
;# V01 OutArgs      [V01    ] (  1,  1   )  lclBlk ( 0) [rsp+00H]   "OutgoingArgSpace"
;
; Lcl frame size = 0

G_M56356_IG01:
						;; bbWeight=1    PerfScore 0.00
G_M56356_IG02:
       mov      rax, qword ptr [rcx]
       mov      rax, qword ptr [rax+112]
       mov      rax, qword ptr [rax+40]
						;; bbWeight=1    PerfScore 6.00
G_M56356_IG03:
       rex.jmp  rax
						;; bbWeight=1    PerfScore 2.00

after

; Assembly listing for method TypeExtensions:IsPrimitive(Type):bool
; Emitting BLENDED_CODE for X64 CPU with AVX - Windows
; optimized code
; rsp based frame
; fully interruptible
; No PGO data
; 1 inlinees with PGO data; 3 single block inlinees; 0 inlinees without PGO data
; Final local variable assignments
;
;  V00 arg0         [V00,T00] (  6,  3.52)     ref  ->  rcx         class-hnd
;  V01 OutArgs      [V01    ] (  1,  1   )  lclBlk (32) [rsp+00H]   "OutgoingArgSpace"
;  V02 tmp1         [V02,T02] (  2,  0.97)     int  ->  rax         "guarded devirt return temp"
;* V03 tmp2         [V03    ] (  0,  0   )     ref  ->  zero-ref    class-hnd exact "guarded devirt this exact temp"
;  V04 tmp3         [V04,T01] (  2,  1.94)   ubyte  ->  rcx         "Inlining Arg"
;
; Lcl frame size = 40

G_M56356_IG01:
       sub      rsp, 40
						;; bbWeight=1    PerfScore 0.25
G_M56356_IG02:
       mov      rax, 0xD1FFAB1E
       cmp      qword ptr [rcx], rax
       jne      SHORT G_M56356_IG05
						;; bbWeight=1    PerfScore 3.25
G_M56356_IG03:
       call     RuntimeTypeHandle:GetCorElementType(RuntimeType):ubyte
       movzx    rcx, al
       mov      eax, 1
       shl      eax, cl
       test     eax, 0xD1FFAB1E
       setne    al
       movzx    rax, al
       movzx    rax, al
						;; bbWeight=0.49 PerfScore 2.55
G_M56356_IG04:
       add      rsp, 40
       ret      
						;; bbWeight=0.49 PerfScore 0.61
G_M56356_IG05:
       mov      rax, qword ptr [rcx]
       mov      rax, qword ptr [rax+112]
       mov      rax, qword ptr [rax+40]
						;; bbWeight=0.01 PerfScore 0.09
G_M56356_IG06:
       add      rsp, 40
       rex.jmp  rax

AndyAyersMS · 2021-04-22T03:40:12Z

crossgen2 jit-diffs is hitting the assert from #10821 in SPC:

Assertion failed '((regMask & emitThisGCrefRegs) && (ins == INS_add)) || ((regMask & emitThisByrefRegs) && (ins == INS_add || ins == INS_sub))' 
in 'System.Text.Encoding:GetCharsWithFallback(long,int,long,int,int,int,System.Text.DecoderNLS):int:this' during 'Emit code' (IL size 183)

This happens without these changes too.

AndyAyersMS · 2021-04-22T15:32:19Z

The superpmi CI test is hitting some replay failures... investigating

      MISSING: Method context 2975 failed to replay: SuperPMI assertion 'ResolveVirtualMethod->GetIndex(key) != -1' failed ("Didn't find 00007FFAC10B9C58-00000048c10ba380-00007FFAC10B9DA9")
      MISSING: Method context 2974 failed to replay: SuperPMI assertion 'ResolveVirtualMethod->GetIndex(key) != -1' failed ("Didn't find 00007FFAC108FC70-00000060c10bcb30-00007FFAC10B0039")

AndyAyersMS · 2021-04-22T18:31:05Z

Failure is on replay of System.IO.StreamWriter:Flush(bool,bool):this (which is hit in both collections).

During collection we see:

  Likely class for 00007FFE1AB99AA0 (System.IO.Strategies.FileStreamStrategy) is 00007FFE1AB9AB88 (System.IO.Strategies.BufferedFileStreamStrategy) [likelihood:100 classes seen:1]

and during replay

  Likely class for 00007FFE1ABA9AA0 (System.IO.Strategies.FileStreamStrategy) is 000000601ABAAB88 (hackishClassName) [likelihood:100 classes seen:1]

and so during replay we issue a resolveVirtualMethod call that does not have the right arguments.

The replay data looks similar, just the high part of the address is different, so perhaps we're still not recording the right amount of data for a PGO payload, or something else is corrupting the data.

Dumping the raw MC for that method, I see

2-GetPgoInstrumentationResults key ftn-00007FFE1ABA8B20, value res-00000000 schemaCnt-3 profileBufSize-8 schema{
 0-{Offset 0000000000000000 ILOffset 0 Kind 320(0x140) Count 1 Other 9 Data ?}
 1-{Offset 0000000000000000 ILOffset 0 Kind 385(0x181) Count 1 Other 0 Data E 5871}
 2-{Offset 0000000000000004 ILOffset 7 Kind 451(0x1c3) Count 1 Other 356 Data N 1 L 1 C 000000601ABAAB88}
} data_index-1692

so the method get likely class data at offset 4. So the profileBufSize should be 12, not 8. Hmm.

The bug is here:

        if (pInSchema[i].Offset >= maxOffset)
        {
            maxOffset = pInSchema[i].Offset + pInSchema[i].Count * sizeof(uintptr_t);
        }

We process the first record and set maxOffset to 8. We then don't consider the impact of the record with offset 4, even though it has greater overall extent.

AndyAyersMS · 2021-04-22T19:42:07Z

@dotnet/jit-contrib ping

BruceForstall

I think the current implementation of getLikelyClass, as a raw function export from the JIT, is unacceptable. It needs to either be a member of the JIT-EE interface (maybe a member of ICorJitCompiler? or maybe ICorJitCompiler should have a kind of QueryInterface that would give back some kind of ICorJitPgoUtility interface), or be in a different dll. The JIT code should have full access to the jithost (so config variables work, memory allocation works), etc.

davidwrighton · 2021-04-22T21:16:36Z

@BruceForstall frankly I agree with you. If someone has the bandwidth to suggest what the api should actually look like and tweak the jit side of the fence to provide/use the api, I can take the cost for integration into crossgen2. The ergonomics of what's needed in the jit though are rather alien to me, so I'd really rather not drive defining that interface. For instance, I tried to do better when I put this together in the first place, and I was not successful in coming up with a way to integrate with the JIT's memory allocation apis. I eventually gave up and made a stack allocated fixed size buffer which was "good enough" to solve the functional problems I was working on.

AndyAyersMS and others added 5 commits April 21, 2021 13:38

partial fixes for likely class changes

b554a73

Fix likely class bug

69e0ccd

Comment out assert which is faulty in the presence of edge and not ba…

27c9cfd

…sic block data

Merge pull request #1 from davidwrighton/UpdateJitForNewLikelyClassRe…

2228641

…cords Update jit for new likely class records

use fgCalledCount for JitOrder output

f64765d

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 22, 2021

fix pgo data extent computation

7a05700

BruceForstall approved these changes Apr 22, 2021

View reviewed changes

AndyAyersMS merged commit 57104c7 into dotnet:main Apr 23, 2021

AndyAyersMS deleted the UpdateJitForNewLikelyClassRecords branch April 23, 2021 00:46

AndyAyersMS mentioned this pull request Apr 23, 2021

PGO: GetLikelyClass changes missing logic to enable guarded devirtualization #51643

Closed

JulieLeeMSFT mentioned this pull request Apr 27, 2021

What's new in .NET 6 Preview 4 dotnet/core#6098

Closed

karelz added this to the 6.0.0 milestone May 20, 2021

ghost locked as resolved and limited conversation to collaborators Jun 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update jit for new likely class records #51664

Update jit for new likely class records #51664

AndyAyersMS commented Apr 22, 2021

AndyAyersMS commented Apr 22, 2021

AndyAyersMS commented Apr 22, 2021

AndyAyersMS commented Apr 22, 2021

AndyAyersMS commented Apr 22, 2021

AndyAyersMS commented Apr 22, 2021

AndyAyersMS commented Apr 22, 2021

AndyAyersMS commented Apr 22, 2021 •

edited

Loading

AndyAyersMS commented Apr 22, 2021

BruceForstall left a comment

davidwrighton commented Apr 22, 2021

Update jit for new likely class records #51664

Update jit for new likely class records #51664

Conversation

AndyAyersMS commented Apr 22, 2021

AndyAyersMS commented Apr 22, 2021

AndyAyersMS commented Apr 22, 2021

AndyAyersMS commented Apr 22, 2021

AndyAyersMS commented Apr 22, 2021

before

after

AndyAyersMS commented Apr 22, 2021

AndyAyersMS commented Apr 22, 2021

AndyAyersMS commented Apr 22, 2021 • edited Loading

AndyAyersMS commented Apr 22, 2021

BruceForstall left a comment

Choose a reason for hiding this comment

davidwrighton commented Apr 22, 2021

AndyAyersMS commented Apr 22, 2021 •

edited

Loading