JIT: revise how the jit tracks use of generics context #34827

AndyAyersMS · 2020-04-10T18:45:28Z

The generics context is reported by the jit specially whenever it feeds into
runtime lookups, as expansion of those lookups can expose pointers into runtime
data structures, and we don't want those data structures to be collected if
jitted code is still using them.

Sometimes uses of the context are optimized away, and reporting costs
code size and GC space, so we don't want to report the context unless there
is an actual use.

This change revises how the jit keeps track of context use -- instead of trying
to incrementally ref count uses of the generics context, we now just leverage
existing passes which do local accounting.

Initial motivation for this came from #34641 where the context use was
over-reported, but investigation showed we also some times under-report as
the context var could be cloned without changing the ref count.

So this change fixes both under and over reporting.

Closes #34641.

The generics context is reported by the jit specially whenever it feeds into runtime lookups, as expansion of those lookups can expose pointers into runtime data structures, and we don't want those data structures to be collected if jitted code is still using them. Sometimes uses of the context are optimized away, and reporting costs code size and GC space, so we don't want to report the context unless there is an actual use. This change revises how the jit keeps track of context use -- instead of trying to incrementally ref count uses of the generics context, we now just leverage existing passes which do local accounting. Initial motivation for this came from dotnet#34641 where the context use was over-reported, but investigation showed we also some times under-report as the context var could be cloned without changing the ref count. So this change fixes both under and over reporting. Closes dotnet#34641.

AndyAyersMS · 2020-04-10T18:47:14Z

cc @dotnet/jit-contrib

There were more cases of under-reporting than over-reporting, so a slight code size increase overall.

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 diff: 1445 (0.00% of base)
    diff is a regression.
Top file regressions (bytes):
         675 : System.Collections.Immutable.dasm (0.06% of base)
         183 : System.Private.CoreLib.dasm (0.00% of base)
         176 : System.Collections.dasm (0.03% of base)
         101 : Microsoft.CodeAnalysis.dasm (0.01% of base)
         100 : System.Text.Json.dasm (0.01% of base)
          59 : System.Collections.Concurrent.dasm (0.02% of base)
          38 : Microsoft.Diagnostics.Tracing.TraceEvent.dasm (0.00% of base)
          38 : System.Linq.Parallel.dasm (0.00% of base)
          30 : System.Threading.Tasks.Dataflow.dasm (0.00% of base)
          14 : System.Private.DataContractSerialization.dasm (0.00% of base)
          14 : System.Private.Xml.dasm (0.00% of base)
           8 : System.Linq.Expressions.dasm (0.00% of base)
           7 : Microsoft.Diagnostics.FastSerialization.dasm (0.01% of base)
           5 : CommandLine.dasm (0.00% of base)
Top file improvements (bytes):
          -2 : System.Net.Http.dasm (-0.00% of base)
          -1 : System.Threading.Channels.dasm (-0.00% of base)
16 total files with Code Size differences (2 improved, 14 regressed), 248 unchanged.
Top method regressions (bytes):
          95 ( 0.20% of base) : System.Text.Json.dasm - ObjectDefaultConverter`1:OnTryRead(byref,Type,JsonSerializerOptions,byref,byref):bool:this (7 methods)
          54 ( 1.01% of base) : System.Private.CoreLib.dasm - Enumerator:MoveNext():bool:this (59 methods)
          42 ( 5.64% of base) : System.Collections.Immutable.dasm - Builder:Add(KeyValuePair`2):this (14 methods)
          22 ( 0.73% of base) : System.Private.CoreLib.dasm - LockedStack:Trim(int,int,int,int):this (7 methods)
          21 ( 9.81% of base) : System.Collections.dasm - SortedList`2:System.Collections.Generic.ICollection<System.Collections.Generic.KeyValuePair<TKey,TValue>>.Add(KeyValuePair`2):this (7 methods)
          19 (11.59% of base) : Microsoft.CodeAnalysis.dasm - TextKeyedCache`1:AddSharedEntry(int,SharedEntryValue):this
          18 ( 6.52% of base) : System.Collections.Immutable.dasm - ImmutableDictionary`2:get_Origin():MutationInput:this (7 methods)
          18 (56.25% of base) : System.Linq.Parallel.dasm - PairOutputKeyBuilder`2:Combine(__Canon,long):Pair`2:this
          17 ( 3.71% of base) : System.Collections.Immutable.dasm - Builder:Apply(MutationResult):this (7 methods)
          15 ( 9.09% of base) : System.Collections.Immutable.dasm - Builder:GetValueOrDefault(__Canon,long):long:this (2 methods)
          14 ( 1.83% of base) : System.Collections.Concurrent.dasm - ConcurrentDictionary`2:System.Collections.Generic.ICollection<System.Collections.Generic.KeyValuePair<TKey,TValue>>.Contains(KeyValuePair`2):bool:this (7 methods)
          14 ( 2.65% of base) : System.Collections.Immutable.dasm - Node:FindLastIndex(Predicate`1):int:this (7 methods)
          14 (14.00% of base) : System.Collections.Immutable.dasm - Node:TryGetValue(__Canon,IComparer`1,byref):bool:this
          14 (11.48% of base) : System.Collections.Immutable.dasm - Node:TryGetKey(__Canon,IComparer`1,byref):bool:this
          14 ( 1.45% of base) : System.Collections.Immutable.dasm - Node:Contains(KeyValuePair`2,IComparer`1,IEqualityComparer`1):bool:this (7 methods)
          14 ( 1.92% of base) : System.Private.CoreLib.dasm - Dictionary`2:System.Collections.Generic.ICollection<System.Collections.Generic.KeyValuePair<TKey,TValue>>.Contains(KeyValuePair`2):bool:this (7 methods)
          13 ( 7.51% of base) : Microsoft.CodeAnalysis.dasm - TextKeyedCache`1:FindSharedEntry(ref,int,int,int):SharedEntryValue:this
          13 ( 3.10% of base) : System.Collections.Concurrent.dasm - ConcurrentDictionary`2:TryRemove(KeyValuePair`2):bool:this (7 methods)
          13 ( 4.71% of base) : System.Collections.Concurrent.dasm - ConcurrentDictionary`2:.ctor():this (7 methods)
          13 ( 4.96% of base) : System.Collections.Concurrent.dasm - ConcurrentDictionary`2:.ctor(IEqualityComparer`1):this (7 methods)
Top method improvements (bytes):
         -11 (-0.51% of base) : System.Private.CoreLib.dasm - ArraySortHelper`2:HeapSort(Span`1,Span`1,IComparer`1) (7 methods)
          -9 (-17.65% of base) : System.Collections.Concurrent.dasm - Partitioner:GetDefaultChunkSize():int (7 methods)
          -9 (-18.75% of base) : System.Collections.Immutable.dasm - ImmutableExtensions:IsValueType():bool (7 methods)
          -9 (-1.11% of base) : System.Linq.Expressions.dasm - ReadOnlyCollectionBuilder`1:ValidateNullValue(Object,String) (7 methods)
          -9 (-17.65% of base) : System.Linq.Parallel.dasm - Scheduling:GetDefaultChunkSize():int (7 methods)
          -5 (-0.97% of base) : System.Private.CoreLib.dasm - TlsOverPerCoreLockedStacksArrayPool`1:Gen2GcCallbackFunc(Object):bool (9 methods)
          -5 (-0.36% of base) : System.Private.CoreLib.dasm - ArraySortHelper`1:HeapSort(Span`1,Comparison`1) (7 methods)
          -2 (-0.14% of base) : System.Threading.Tasks.Dataflow.dasm - SourceObservable`1:get_DebuggerDisplayContent():Object:this (7 methods)
          -1 (-0.21% of base) : System.Net.Http.dasm - ResettableCompletionSource`1:GetValueTask():ValueTask`1:this (7 methods)
          -1 (-0.27% of base) : System.Net.Http.dasm - ResettableCompletionSource`1:GetTypelessValueTask():ValueTask:this (7 methods)
          -1 (-0.23% of base) : System.Threading.Channels.dasm - AsyncOperation`1:get_ValueTaskOfT():ValueTask`1:this (7 methods)
Top method regressions (percentages):
           9 (128.57% of base) : Microsoft.CodeAnalysis.dasm - <>c:<get_InInsertionOrder>b__10_1(KeyValuePair`2):__Canon:this
           9 (128.57% of base) : System.Collections.Immutable.dasm - <>c:<get_Keys>b__30_0(KeyValuePair`2):__Canon:this
           9 (128.57% of base) : System.Linq.Parallel.dasm - <>c:<MakeResultSelectorFunction>b__5_0(Pair`2):__Canon:this
           9 (128.57% of base) : System.Threading.Tasks.Dataflow.dasm - <>c:<get_InputQueue>b__5_0(KeyValuePair`2):__Canon:this
          18 (56.25% of base) : System.Linq.Parallel.dasm - PairOutputKeyBuilder`2:Combine(__Canon,long):Pair`2:this
           9 (29.03% of base) : Microsoft.CodeAnalysis.dasm - SmallDictionary`2:get_Keys():KeyCollection:this (7 methods)
           9 (29.03% of base) : Microsoft.CodeAnalysis.dasm - SmallDictionary`2:get_Values():ValueCollection:this (7 methods)
           9 (29.03% of base) : System.Private.CoreLib.dasm - Task`1:GetAwaiter():TaskAwaiter`1:this (7 methods)
          13 (25.00% of base) : System.Collections.Immutable.dasm - Node:Contains(__Canon,IComparer`1):bool:this
           9 (24.32% of base) : System.Collections.Immutable.dasm - <>c:<get_Values>b__32_0(KeyValuePair`2):long:this (7 methods)
           3 (23.08% of base) : System.Private.CoreLib.dasm - Unsafe:Write(long,__Canon)
           3 (23.08% of base) : System.Private.CoreLib.dasm - Unsafe:Write(byref,__Canon)
          10 (21.74% of base) : System.Collections.Immutable.dasm - SecurePooledObject`1:.ctor(__Canon):this
           9 (18.75% of base) : Microsoft.CodeAnalysis.dasm - <>c:<get_InInsertionOrder>b__10_0(KeyValuePair`2):int:this (7 methods)
          13 (18.57% of base) : System.Collections.Immutable.dasm - SortedInt32KeyNode`1:SetItem(int,__Canon,IEqualityComparer`1,byref,byref):SortedInt32KeyNode`1:this
          13 (17.11% of base) : System.Collections.Immutable.dasm - Node:ContainsKey(__Canon,IComparer`1):bool:this
           5 (15.62% of base) : CommandLine.dasm - <>c__DisplayClass9_0`1:<Index>b__0(__Canon,int):KeyValuePair`2:this
          14 (14.00% of base) : System.Collections.Immutable.dasm - Node:TryGetValue(__Canon,IComparer`1,byref):bool:this
          13 (13.27% of base) : System.Collections.Immutable.dasm - ImmutableDictionary`2:get_Item(__Canon):long:this
          19 (11.59% of base) : Microsoft.CodeAnalysis.dasm - TextKeyedCache`1:AddSharedEntry(int,SharedEntryValue):this
Top method improvements (percentages):
          -9 (-18.75% of base) : System.Collections.Immutable.dasm - ImmutableExtensions:IsValueType():bool (7 methods)
          -9 (-17.65% of base) : System.Collections.Concurrent.dasm - Partitioner:GetDefaultChunkSize():int (7 methods)
          -9 (-17.65% of base) : System.Linq.Parallel.dasm - Scheduling:GetDefaultChunkSize():int (7 methods)
          -9 (-1.11% of base) : System.Linq.Expressions.dasm - ReadOnlyCollectionBuilder`1:ValidateNullValue(Object,String) (7 methods)
          -5 (-0.97% of base) : System.Private.CoreLib.dasm - TlsOverPerCoreLockedStacksArrayPool`1:Gen2GcCallbackFunc(Object):bool (9 methods)
         -11 (-0.51% of base) : System.Private.CoreLib.dasm - ArraySortHelper`2:HeapSort(Span`1,Span`1,IComparer`1) (7 methods)
          -5 (-0.36% of base) : System.Private.CoreLib.dasm - ArraySortHelper`1:HeapSort(Span`1,Comparison`1) (7 methods)
          -1 (-0.27% of base) : System.Net.Http.dasm - ResettableCompletionSource`1:GetTypelessValueTask():ValueTask:this (7 methods)
          -1 (-0.23% of base) : System.Threading.Channels.dasm - AsyncOperation`1:get_ValueTaskOfT():ValueTask`1:this (7 methods)
          -1 (-0.21% of base) : System.Net.Http.dasm - ResettableCompletionSource`1:GetValueTask():ValueTask`1:this (7 methods)
          -2 (-0.14% of base) : System.Threading.Tasks.Dataflow.dasm - SourceObservable`1:get_DebuggerDisplayContent():Object:this (7 methods)
174 total methods with Code Size differences (11 improved, 163 regressed), 243616 unchanged.

AndyAyersMS · 2020-04-13T17:26:17Z

@dotnet/jit-contrib PTAL

AndyAyersMS · 2020-04-15T01:07:25Z

@BruceForstall maybe you can look this over.

There might be some "cheaper" way to identify this uses that are part of runtime lookups, seems like flag space is at a premium... any ideas?

BruceForstall · 2020-04-15T01:26:09Z

Will try to look soon

erozenfeld · 2020-04-15T01:28:53Z

src/coreclr/src/jit/compiler.h

@@ -3127,7 +3125,7 @@ class Compiler

 #endif // defined(DEBUG) && defined(TARGET_X86)

-    unsigned lvaGenericsContextUseCount;
+    unsigned lvaGenericsContextInUse;


Suggested change

unsigned lvaGenericsContextInUse;

bool lvaGenericsContextInUse;

erozenfeld · 2020-04-15T01:30:47Z

src/coreclr/src/jit/compiler.hpp

@@ -1944,14 +1943,12 @@ inline bool Compiler::lvaKeepAliveAndReportThis()
    // because collectible types need the generics context when gc-ing.
    if (genericsContextIsThis)
    {
-        const bool isUsed   = lvaGenericsContextUseCount > 0;
+        const bool isUsed   = lvaGenericsContextInUse;


You can just use lvaGenericsContextInUse without introducing isUsed.

CarolEidt · 2020-04-16T23:20:04Z

There might be some "cheaper" way to identify this uses that are part of runtime lookups, seems like flag space is at a premium... any ideas?

The only thing I could think of would be to add a "pass-through" node above it. That would mean having to add handling for that wherever these might be encountered, so it might be worse.

erozenfeld

Looks good overall. Left a few minor suggestions.

erozenfeld · 2020-04-22T01:48:30Z

src/coreclr/src/jit/compiler.hpp

-            JITDUMP("Reporting this as generic context: %u refs%s\n", lvaGenericsContextUseCount,
-                    mustKeep ? ", must keep" : "");
-
+            JITDUMP("Reporting this as generic context: %s\n", mustKeep ? "must keep" : "referenced");


You kept the JITDUMP here but removed it a few lines above and in lvaReportParamTypeArg(). It would be better to be consistent.

erozenfeld · 2020-04-22T01:52:09Z

src/coreclr/src/jit/compiler.h

@@ -3127,7 +3125,7 @@ class Compiler

 #endif // defined(DEBUG) && defined(TARGET_X86)

-    unsigned lvaGenericsContextUseCount;
+    bool lvaGenericsContextInUse;


Super nit: You are using "generics context" and "generic context" inconsistently in var names and comments in this change. I think it should be "generic context".

There are many many uses of "generics context" so it seems simplest to standardize on that.

Hmm many votes in both camps. There are 137 case-insensitive uses of

generic.*(context|ctxt)

and 77 uses of

generics.*(context|ctxt)

The "S" variant appears in enums in corinfo.h so can't easily be updated. But we can fix other appearances if you think it's worthwhile.

Probably not worth cleaning this up in this PR.

erozenfeld · 2020-04-22T02:19:03Z

src/coreclr/src/jit/lclvars.cpp

}
+

Suggested change

bool reportParamTypeArg = lvaReportParamTypeArg();

if (lvaKeepAliveAndReportThis())

{

lvaGetDesc(0u)->lvImplicitlyReferenced = reportParamTypeArg;

}

else if (reportParamTypeArg)

{

// We should have a context arg.

assert(info.compTypeCtxtArg != BAD_VAR_NUM);

lvaGetDesc(info.compTypeCtxtArg)->lvImplicitlyReferenced = true;

}

Will update.

AndyAyersMS · 2020-04-22T21:08:28Z

Reran diffs; similar to the above.

Dotnet-GitSync-Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 10, 2020

AndyAyersMS mentioned this pull request Apr 10, 2020

JIT emitting unnecessary code for non-emitted method call #34641

Closed

jaredpar mentioned this pull request Apr 10, 2020

Losing connection to agents during CI / PR #34832

Closed

AndyAyersMS mentioned this pull request Apr 14, 2020

Add IR support for multireg lclVars & intrinsics #34822

Merged

erozenfeld reviewed Apr 15, 2020

View reviewed changes

review feedback

5ca4a17

erozenfeld approved these changes Apr 22, 2020

View reviewed changes

AndyAyersMS added 3 commits April 22, 2020 00:50

Merge branch 'master' into InvestiateGenericContextReporting

c709e93

Merge branch 'master' into InvestiateGenericContextReporting

655f972

review feedback

8ee5fc1

AndyAyersMS merged commit 510d664 into dotnet:master Apr 23, 2020

AndyAyersMS deleted the InvestiateGenericContextReporting branch April 23, 2020 08:06

ghost locked as resolved and limited conversation to collaborators Dec 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JIT: revise how the jit tracks use of generics context #34827

JIT: revise how the jit tracks use of generics context #34827

AndyAyersMS commented Apr 10, 2020

AndyAyersMS commented Apr 10, 2020

AndyAyersMS commented Apr 13, 2020

AndyAyersMS commented Apr 15, 2020

BruceForstall commented Apr 15, 2020

erozenfeld Apr 15, 2020

erozenfeld Apr 15, 2020

CarolEidt commented Apr 16, 2020

erozenfeld left a comment

erozenfeld Apr 22, 2020

AndyAyersMS Apr 22, 2020

erozenfeld Apr 22, 2020

AndyAyersMS Apr 22, 2020

AndyAyersMS Apr 22, 2020

erozenfeld Apr 22, 2020

erozenfeld Apr 22, 2020

AndyAyersMS Apr 22, 2020

AndyAyersMS commented Apr 22, 2020

	unsigned lvaGenericsContextInUse;
	bool lvaGenericsContextInUse;

+    bool reportParamTypeArg = lvaReportParamTypeArg();
+    if (lvaKeepAliveAndReportThis())
+    {
+        lvaGetDesc(0u)->lvImplicitlyReferenced = reportParamTypeArg;
+    }
+    else if (reportParamTypeArg)
+    {
+        // We should have a context arg.
+        assert(info.compTypeCtxtArg != BAD_VAR_NUM);
+        lvaGetDesc(info.compTypeCtxtArg)->lvImplicitlyReferenced = true;
+    }

JIT: revise how the jit tracks use of generics context #34827

JIT: revise how the jit tracks use of generics context #34827

Conversation

AndyAyersMS commented Apr 10, 2020

AndyAyersMS commented Apr 10, 2020

AndyAyersMS commented Apr 13, 2020

AndyAyersMS commented Apr 15, 2020

BruceForstall commented Apr 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarolEidt commented Apr 16, 2020

erozenfeld left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndyAyersMS commented Apr 22, 2020