Support for devirtualizing array interface methods

[AndyAyersMS]

Update jit and runtime to devirtualize interface calls on arrays.
This resolves the first two issues noted in #62457.

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Update jit and runtime to devirtualize interface calls on arrays.
This resolves the first two issues noted in #62457.

Author:	AndyAyersMS
Assignees:	-
Labels:	area-CodeGen-coreclr
Milestone:	-

[AndyAyersMS]

cc @dotnet/jit-contrib @jkotas

@jkotas I imagine there might be some gotcha here. The type punning done by SZArrayHelper seems to work out ok as the this is cast back to the right typed array.

Diffs (via pmi) show a few hits. PGO should see more. The diffs are a bit misleading because until some of these methods are called the jit will first see a no-inline wrapper stub.

Summary of Code Size diffs:
(Lower is better)

Total bytes of base: 61548636
Total bytes of diff: 61547858
Total bytes of delta: -778 (-0.00 % of base)
Total relative delta: -2.95
    diff is an improvement.
    relative diff is an improvement.


Top file regressions (bytes):
          21 : Microsoft.CodeAnalysis.dasm (0.00% of base)
           4 : Microsoft.CodeAnalysis.VisualBasic.dasm (0.00% of base)
           4 : Microsoft.Diagnostics.Tracing.TraceEvent.dasm (0.00% of base)
           4 : Microsoft.CodeAnalysis.CSharp.dasm (0.00% of base)

Top file improvements (bytes):
        -158 : Microsoft.CSharp.dasm (-0.04% of base)
        -131 : System.Linq.Expressions.dasm (-0.02% of base)
         -95 : System.Threading.Tasks.Dataflow.dasm (-0.01% of base)
         -95 : System.Linq.Parallel.dasm (-0.00% of base)
         -93 : System.Linq.dasm (-0.01% of base)
         -44 : Newtonsoft.Json.dasm (-0.00% of base)
         -44 : System.Private.CoreLib.dasm (-0.00% of base)
         -31 : System.Net.Http.dasm (-0.00% of base)
         -24 : FSharp.Core.dasm (-0.00% of base)
         -20 : System.Private.DataContractSerialization.dasm (-0.00% of base)
         -16 : CommandLine.dasm (-0.00% of base)
         -16 : System.Composition.Convention.dasm (-0.01% of base)
         -16 : xunit.console.dasm (-0.02% of base)
         -12 : System.IO.FileSystem.Watcher.dasm (-0.08% of base)
          -8 : System.Composition.Hosting.dasm (-0.01% of base)
          -8 : System.Linq.Queryable.dasm (-0.00% of base)

20 total files with Code Size differences (16 improved, 4 regressed), 255 unchanged.

Top method regressions (bytes):
          21 (32.31% of base) : Microsoft.CodeAnalysis.dasm - <>c__DisplayClass38_0:<ParseSeparatedStrings>b__0(ushort):bool:this
           4 ( 0.61% of base) : Microsoft.CodeAnalysis.CSharp.dasm - CSharpDiagnosticFilter:Filter(Diagnostic,int,int,IDictionary`2):Diagnostic
           4 ( 1.26% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - LinuxPerfScriptEventParser:SkipPreamble(FastStream):this
           4 ( 0.58% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - VisualBasicDiagnosticFilter:Filter(Diagnostic,int,IDictionary`2):Diagnostic

Top method improvements (bytes):
         -95 (-50.53% of base) : System.Linq.Parallel.dasm - ArrayMergeHelper`1:GetEnumerator():IEnumerator`1:this (8 methods)
         -95 (-54.29% of base) : System.Threading.Tasks.Dataflow.dasm - ImmutableArray`1:GetEnumerator():IEnumerator`1:this (8 methods)
         -93 (-3.98% of base) : System.Linq.dasm - AppendPrependN`1:MoveNext():bool:this (8 methods)
         -47 (-6.84% of base) : Microsoft.CSharp.dasm - DispCallableMetaObject:BindSetIndex(SetIndexBinder,ref,DynamicMetaObject):DynamicMetaObject:this
         -47 (-9.40% of base) : Microsoft.CSharp.dasm - IDispatchMetaObject:BindSetIndex(SetIndexBinder,ref,DynamicMetaObject):DynamicMetaObject:this
         -44 (-12.36% of base) : System.Private.CoreLib.dasm - ConditionalWeakTable`2:System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<TKey,TValue>>.GetEnumerator():IEnumerator`1:this
         -31 (-11.15% of base) : System.Net.Http.dasm - HttpHeaderValueCollection`1:GetEnumerator():IEnumerator`1:this
         -23 (-0.80% of base) : System.Linq.Expressions.dasm - StackSpiller:RewriteListInitExpression(Expression,int):Result:this
         -22 (-14.01% of base) : Microsoft.CSharp.dasm - ComMetaObject:BindSetIndex(SetIndexBinder,ref,DynamicMetaObject):DynamicMetaObject:this
         -18 (-6.04% of base) : Newtonsoft.Json.dasm - CollectionUtils:ResolveEnumerableCollectionConstructor(Type,Type,Type):ConstructorInfo
         -16 (-5.21% of base) : System.Composition.Convention.dasm - ImportConventionBuilder:IsSupportedImportManyType(TypeInfo):bool:this
         -16 (-3.26% of base) : xunit.console.dasm - Program:Main(ref):int
         -14 (-0.68% of base) : Microsoft.CSharp.dasm - ComInvokeBinder:GenerateTryBlock():Expression:this
         -14 (-9.86% of base) : Microsoft.CSharp.dasm - ComMetaObject:BindInvoke(InvokeBinder,ref):DynamicMetaObject:this
         -14 (-9.86% of base) : Microsoft.CSharp.dasm - ComMetaObject:BindInvokeMember(InvokeMemberBinder,ref):DynamicMetaObject:this
         -14 (-5.71% of base) : Newtonsoft.Json.dasm - KeyValuePairConverter:InitializeReflectionObject(Type):ReflectionObject
         -12 (-1.06% of base) : System.Linq.Expressions.dasm - StackSpiller:RewriteInvocationExpression(Expression,int):Result:this
         -12 (-1.30% of base) : System.Linq.Expressions.dasm - StackSpiller:RewriteMethodCallExpression(Expression,int):Result:this
         -10 (-2.00% of base) : System.Linq.Expressions.dasm - StackSpiller:RewriteDynamicExpression(Expression):Result:this
          -8 (-4.06% of base) : CommandLine.dasm - <>c:<ChangeTypeScalar>b__2_1(IEnumerable`1):this

Top method regressions (percentages):
          21 (32.31% of base) : Microsoft.CodeAnalysis.dasm - <>c__DisplayClass38_0:<ParseSeparatedStrings>b__0(ushort):bool:this
           4 ( 1.26% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - LinuxPerfScriptEventParser:SkipPreamble(FastStream):this
           4 ( 0.61% of base) : Microsoft.CodeAnalysis.CSharp.dasm - CSharpDiagnosticFilter:Filter(Diagnostic,int,int,IDictionary`2):Diagnostic
           4 ( 0.58% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - VisualBasicDiagnosticFilter:Filter(Diagnostic,int,IDictionary`2):Diagnostic

Top method improvements (percentages):
         -95 (-54.29% of base) : System.Threading.Tasks.Dataflow.dasm - ImmutableArray`1:GetEnumerator():IEnumerator`1:this (8 methods)
         -95 (-50.53% of base) : System.Linq.Parallel.dasm - ArrayMergeHelper`1:GetEnumerator():IEnumerator`1:this (8 methods)
          -6 (-35.29% of base) : System.IO.FileSystem.Watcher.dasm - ImmutableStringList:GetEnumerator():IEnumerator`1:this
          -6 (-35.29% of base) : System.IO.FileSystem.Watcher.dasm - ImmutableStringList:System.Collections.IEnumerable.GetEnumerator():IEnumerator:this
         -22 (-14.01% of base) : Microsoft.CSharp.dasm - ComMetaObject:BindSetIndex(SetIndexBinder,ref,DynamicMetaObject):DynamicMetaObject:this
         -44 (-12.36% of base) : System.Private.CoreLib.dasm - ConditionalWeakTable`2:System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<TKey,TValue>>.GetEnumerator():IEnumerator`1:this
         -31 (-11.15% of base) : System.Net.Http.dasm - HttpHeaderValueCollection`1:GetEnumerator():IEnumerator`1:this
         -14 (-9.86% of base) : Microsoft.CSharp.dasm - ComMetaObject:BindInvoke(InvokeBinder,ref):DynamicMetaObject:this
         -14 (-9.86% of base) : Microsoft.CSharp.dasm - ComMetaObject:BindInvokeMember(InvokeMemberBinder,ref):DynamicMetaObject:this
         -47 (-9.40% of base) : Microsoft.CSharp.dasm - IDispatchMetaObject:BindSetIndex(SetIndexBinder,ref,DynamicMetaObject):DynamicMetaObject:this
          -6 (-8.57% of base) : System.Private.DataContractSerialization.dasm - CollectionDataContract:IsCollectionInterface(Type):bool
          -8 (-7.55% of base) : System.Linq.Queryable.dasm - EnumerableRewriter:<GetPublicType>g__ImplementsIGrouping|7_0(Type):bool
         -47 (-6.84% of base) : Microsoft.CSharp.dasm - DispCallableMetaObject:BindSetIndex(SetIndexBinder,ref,DynamicMetaObject):DynamicMetaObject:this
         -18 (-6.04% of base) : Newtonsoft.Json.dasm - CollectionUtils:ResolveEnumerableCollectionConstructor(Type,Type,Type):ConstructorInfo
         -14 (-5.71% of base) : Newtonsoft.Json.dasm - KeyValuePairConverter:InitializeReflectionObject(Type):ReflectionObject
          -8 (-5.33% of base) : System.Private.DataContractSerialization.dasm - ClassDataContract:IsNonSerializedMember(Type,String):bool
         -16 (-5.21% of base) : System.Composition.Convention.dasm - ImportConventionBuilder:IsSupportedImportManyType(TypeInfo):bool:this
          -8 (-4.06% of base) : CommandLine.dasm - <>c:<ChangeTypeScalar>b__2_1(IEnumerable`1):this
         -93 (-3.98% of base) : System.Linq.dasm - AppendPrependN`1:MoveNext():bool:this (8 methods)
          -8 (-3.51% of base) : System.Linq.Expressions.dasm - Expression:GetDelegateType(ref):Type

49 total methods with Code Size differences (45 improved, 4 regressed), 278976 unchanged.

[AndyAyersMS]

Sample diff from

runtime/src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/LambdaExpression.cs

Lines 1115 to 1121 in 09cd9c2

	public static Type GetDelegateType(params Type[] typeArgs)
	{
	ContractUtils.RequiresNotEmpty(typeArgs, nameof(typeArgs));
	ContractUtils.RequiresNotNullItems(typeArgs, nameof(typeArgs));
	return Compiler.DelegateHelpers.MakeDelegateType(typeArgs);
	}
	}

The call to Contract.RequiresNotEmpty gets inlined. That method takes an ICollection<T> argument.

;; Assembly listing for method Expression:GetDelegateType(ref):Type

;; before
       mov      rcx, rsi
       mov      r11, 0xD1FFAB1E
       call     [r11]ICollection`1:get_Count():int:this
       test     eax, eax

;; after
       mov      rcx, rsi
       call     SZArrayHelper:get_Count():int:this   // nominally this should inline (see note above)

[jkotas]

@jkotas I imagine there might be some gotcha here.

Agree. Looks like that there are failing tests. The implementation of generic array interfaces was always a bug farm.

[AndyAyersMS]

Issue seems to be here:

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/ConditionalWeakTable.cs

Lines 235 to 244 in 02f70d0

	IEnumerator<KeyValuePair<TKey, TValue>> IEnumerable<KeyValuePair<TKey, TValue>>.GetEnumerator()
	{
	lock (_lock)
	{
	Container c = _container;
	return c is null || c.FirstFreeEntry == 0 ?
	((IEnumerable<KeyValuePair<TKey, TValue>>)Array.Empty<KeyValuePair<TKey, TValue>>()).GetEnumerator() :
	new Enumerator(this);
	}
	}

The jit devirtualizes the empty array case and (best guess) somehow we don't properly initialize the static SZGenericArrayEnumerator<T>.Empty.

impDevirtualizeCall: Trying to devirtualize interface call:
    class for 'this' is System.Collections.Generic.KeyValuePair`2[System.__Canon,System.__Canon][] [final] (attrib 010e0010)
    base method is System.Collections.Generic.IEnumerable`1[KeyValuePair`2]::GetEnumerator
--- base class is interface
    devirt to System.SZArrayHelper::GetEnumerator -- final class
               [000088] --CXG--------             *  CALL ind stub ref   
               [000058] ------------- this in r0  +--*  LCL_VAR   ref    V09 tmp4         
               [000087] ------------- calli tgt   \--*  LCL_VAR   int    V11 tmp6         
    final class; can devirtualize (Frodo)
... after devirt...
               [000088] --CXG--------             *  CALL nullcheck ref    System.SZArrayHelper.GetEnumerator  // generic method
               [000058] ------------- this in r0  \--*  LCL_VAR   ref    V09 tmp4  

Possibly two issues?

1. the method we devirtualize to might require an inst param, but the method desc says otherwise.
2. if it does indeed require an inst param, the jit is not providing one

If we can fix the first, I can either have the jit bail out on these cases, or see if I can find the right type to pass somehow.

[AndyAyersMS]

Call stack is

    frame #0: 0xf7a667a2 libcoreclr.so`DBG_DebugBreak + 2
    frame #1: 0xf7a09216 libcoreclr.so`::DebugBreak() at debug.cpp:406
    frame #2: 0xf78ca648 libcoreclr.so`CHECK::Setup(this=0xfffebad8, message=<unavailable>, condition=<unavailable>, file=<unavailable>, line=1583) at check.cpp:198
  * frame #3: 0xf7781222 libcoreclr.so`JIT_GetGenericsGCStaticBase_Framed(pMT=0xe956741c) at jithelpers.cpp:1583
    frame #4: 0xf7781582 libcoreclr.so`JIT_GetGenericsGCStaticBase(pMT=<unavailable>) at jithelpers.cpp:1620

FFFEBB08 f7a667a2 [HelperMethodFrame: fffebb08] 
FFFEBBE8 F0D7B186 System.SZArrayHelper.GetEnumerator[[System.Collections.Generic.KeyValuePair`2[[System.__Canon, System.Private.CoreLib],[System.__Canon, System.Private.CoreLib]], System.Private.CoreLib]]()
FFFEBC00 E7BAE128 System.Runtime.CompilerServices.ConditionalWeakTable`2[[System.__Canon, System.Private.CoreLib],[System.__Canon, System.Private.CoreLib]].System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<TKey,TValue>>.GetEnumerator()
FFFEBC28 E959A6A8 System.Linq.Enumerable.Count[[System.Collections.Generic.KeyValuePair`2[[System.__Canon, System.Private.CoreLib],[System.__Canon, System.Private.CoreLib]], System.Private.CoreLib]](System.Collections.Generic.IEnumerable`1<System.Collections.Generic.KeyValuePair`2<System.__Canon,System.__Canon>>)
FFFEBC60 E959A52A System.Runtime.CompilerServices.Tests.ConditionalWeakTableTests.GetEnumerator_Empty_ReturnsEmptyEnumerator()

and the method table in native frame 3 is

(lldb) expr -f s -- pMT->debug_m_szClassName
(LPCUTF8) $4 = "System.SZGenericArrayEnumerator`1[KeyValuePair`2]"

[jkotas]

the method we devirtualize to might require an inst param, but the method desc says otherwise.

I think it means that the devirtualization is stripping the instantiating stub, and you are using virtual method desc signature to call the devirtualized method desc.

I think the devirtualization has to avoid stripping the instantiating stub. The instantiating stub is needed to compute the value of the inst param.

Or you can bail out in situations where shared generics are involved. I think you will need to bail out for shared generic callsites anyway for the same reason we are bailing out on inlining of shared generic code (we do not have a way to represent these cases on the VM side).

[AndyAyersMS]

Going to close this for now -- hope to get back to it once OSR is done.