|
| 1 | +I'm still familiarizing myself with how this works and have a few observations/questions. I am working with a slightly simplified version of your repro: |
| 2 | + |
| 3 | + |
| 4 | +using System; |
| 5 | +using System.Runtime.CompilerServices; |
| 6 | +using System.Threading; |
| 7 | +Container<object>.MyStaticField = 5; |
| 8 | +Get().TryInvoke<object>(); |
| 9 | +[MethodImpl(MethodImplOptions.NoInlining)] |
| 10 | +static Base<IFoo> Get() => new Derived(); |
| 11 | +public class Container<T> |
| 12 | +{ |
| 13 | + public static int MyStaticField; |
| 14 | +} |
| 15 | +public abstract class Base<T> |
| 16 | +{ |
| 17 | + [MethodImpl(MethodImplOptions.AggressiveInlining)] |
| 18 | + public virtual void TryInvoke<TArg>() |
| 19 | + { |
| 20 | + Container<T>.MyStaticField = 3; |
| 21 | + } |
| 22 | +} |
| 23 | +public sealed class Derived : Base<IFoo> |
| 24 | +{ |
| 25 | + public override void TryInvoke<TArg>() |
| 26 | + { |
| 27 | + base.TryInvoke<TArg>(); |
| 28 | + } |
| 29 | +} |
| 30 | +public interface IFoo; |
| 31 | + |
| 32 | + |
| 33 | + |
| 34 | +It seems we are scanning Base<Canon>.TryInvoke<Canon>, see the field access and create a TypeNonGCStaticBaseGenericLookupResult for the Container<Canon>. |
| 35 | +It looks like the intention is for this to create an instantiated GC static base when GetTarget is called with an IFoo instantiation. But that never happens. |
| 36 | +I think GetTarget is supposed to be called during the generic lookup result's InstantiateDependiencies. But that isn't called for the IFoo instantiation. |
| 37 | +Is this supposed to work through ShadowConcreteMethodNode? I see the scanner creating ShadowConcreteMethodNodes for some other methods but not Base<IFoo>.TryInvoke. When we get to TypeGenericDictionaryNode.GetConditionalStaticDependencies for Base<IFoo>, it skips Base<IFoo>.TryInvoke because of this check: |
| 38 | + |
| 39 | +if (!EETypeNode.MethodHasNonGenericILMethodBody(method)) |
| 40 | + |
| 41 | + continue; |
| 42 | + |
| 43 | +It seems like that's expected (I see "// Generic methods have their own generic dictionaries") but now I'm not sure where we'd normally expect the instantiated non GC static base to be created. |
| 44 | +Am I on the right track at all or should I be looking elsewhere? Hoping you can give some more pointers for where to look next. |
| 45 | + |
| 46 | +Another question I had while looking at the dependency graph - I see this edge in the codegen graph: |
| 47 | + |
| 48 | +__NONGCSTATICSmin_repro_Container_1<min_repro_IFoo> |
| 49 | + |
| 50 | +← reloc |
| 51 | + |
| 52 | +min_repro_Derived__TryInvoke<System___Canon> |
| 53 | + |
| 54 | +and it looks like that Derived.TryInvoke<Canon> is the only "source" node for the __NONGCSTATICS for Container<IFoo>. But the source doesn't mention IFoo at all. Is there some hidden secondary reason for these that causes it to be instantiated? |
| 55 | + |
| 56 | +I'll start by answering the second question because it will probably help answering the first one. If you recompile your repro with --parallelism:1 --codegenopt:JitDisasm=TryInvoke, RyuJIT will show the disassembly of both of the TryInvoke methods. I'll paste them here because I just had a look: |
| 57 | + |
| 58 | + |
| 59 | +; Assembly listing for method Base`1[System.__Canon]:TryInvoke[System.__Canon]():this (FullOpts) |
| 60 | + sub rsp, 40 |
| 61 | + mov qword ptr [rsp+0x20], rdx |
| 62 | + mov rcx, rdx |
| 63 | + call CORINFO_HELP_READYTORUN_GENERIC_STATIC_BASE |
| 64 | + mov dword ptr [rax], 3 |
| 65 | + add rsp, 40 |
| 66 | + ret |
| 67 | + |
| 68 | + |
| 69 | +; Assembly listing for method Derived:TryInvoke[System.__Canon]():this (FullOpts) |
| 70 | + mov dword ptr [(reloc 0x4000000000428a58)], 3 ; static handle |
| 71 | + ret |
| 72 | + |
| 73 | + |
| 74 | +What this tells us is that when RyuJIT was compiling Base.TryInvoke, it had no clue what static base are we going to read from. The method was generated as: take the generic context parameter (the "secret" parameter passed to the method in RDX (RCX is this since this is an instance generic method), and call a runtime helper to perform the actual lookup (the helper is callsite specific so it knows it needs to find the static base of Container<T>, given the generic context provided). once the helper returns, write the value 3 into the location it returned |
| 75 | + |
| 76 | +What happened in Derived.TryInvoke is very different. There is no call to Base.TryInvoke because we inlined it. And through the magic of inlining, we figured out that no runtime lookup is needed, because Container<T> is Container<IFoo> and it is always IFoo, no matter what TArg the TryInvoke<TArg> is called with |
| 77 | + |
| 78 | +So this is where the reloc is coming from (it is the reloc 0x40000...) in jitdisasm |
| 79 | + |
| 80 | +Here's one more workflow that can maybe help you get more familiar with how shared generics are compiled. |
| 81 | + |
| 82 | +Add following line to src\coreclr\nativeaot\Test.CoreLib\src\System\Runtime\RuntimeHelpers.cs: namespace System.Runtime { class TypeLoaderExports { static IntPtr GVMLookupForSlot(object obj, RuntimeMethodHandle slot) => 0; } } |
| 83 | +Paste your repro code into a test.cs file at the root of the runtime repo. Update it so that it doesn't use top level statements and instead has a static void Main() (must be void with no parameters; top level statements are not that) |
| 84 | +build the clr.aot subset so that you have an updated Test.CoreLib with the GVMLookupForSlot method |
| 85 | +build the test.cs: at the root of the repo: dotnet.cmd exec .dotnet\sdk\10.0.100-rc.2.25502.107\Roslyn\bincore\csc.dll -noconfig -nostdlib test.cs -r:artifacts\bin\Test.CoreLib\x64\Debug\Test.CoreLib.dll (on Linux, use dotnet.sh of course and fix file paths) |
| 86 | +Run ilc with the following command line (fixing paths as needed): d:\git\runtime1\test.exe -r:D:\git\runtime1\artifacts\bin\Test.CoreLib\x64\Debug\Test.CoreLib.dll -o:d:\temp\a.obj --systemmodule:Test.CoreLib -O |
| 87 | +You should hit the exact same assert. But this time the only generic code we're looking at are the handful of methods in test.cs, so if you set breakpoints on interesting code, you're guaranteed not to get any noise |
| 88 | + |
| 89 | +Then make a test2.cs file with following content (the difference is that we no longer have generic virtual method, just regular generic methods): |
| 90 | + |
| 91 | +using System; |
| 92 | +using System.Runtime.CompilerServices; |
| 93 | +using System.Threading; |
| 94 | +class Program |
| 95 | +{ |
| 96 | + static void Main() |
| 97 | + { |
| 98 | + Container<object>.MyStaticField = 5; |
| 99 | + new Derived().TryInvoke2<object>(); |
| 100 | + } |
| 101 | +} |
| 102 | +public class Container<T> |
| 103 | +{ |
| 104 | + public static int MyStaticField; |
| 105 | +} |
| 106 | +public abstract class Base<T> |
| 107 | +{ |
| 108 | + [MethodImpl(MethodImplOptions.AggressiveInlining)] |
| 109 | + public void TryInvoke1<TArg>() |
| 110 | + { |
| 111 | + Container<T>.MyStaticField = 3; |
| 112 | + } |
| 113 | +} |
| 114 | +public sealed class Derived : Base<IFoo> |
| 115 | +{ |
| 116 | + [MethodImpl(MethodImplOptions.NoInlining)] |
| 117 | + public void TryInvoke2<TArg>() |
| 118 | + { |
| 119 | + base.TryInvoke1<TArg>(); |
| 120 | + } |
| 121 | +} |
| 122 | +public interface IFoo; |
| 123 | + |
| 124 | +If you add --codegenopt:JitDisasm=TryInvoke* to the ILC command line, you'll see that it compiles into the exact same assembly as the generic virtual method case, but doesn't crash at compile time |
| 125 | + |
| 126 | +The reason (and the difference) between these is the existence of MethodGenericDictionaryNode in one graph and absence in the other graph. Put a breakpoint on the constructor. The Test.CoreLib workflow should make this very convenient to investigate what it brings into the compilation and why |
| 127 | + |
| 128 | +MethodGenericDictionaryNode is the node that is in shared generics responsible of providing a "context" to shared generic methods. In the above example, it is expected that the MethodGenericDictionary for Base<IFoo>.TryInvoke1<object> would have one entry and that entry is the static base of Container<IFoo>. The MethodGenericDictionary for Derived.TryInvoke2<object> should also have only one entry, but that entry is the generic dictionary of the former TryInvoke1 instantiation |
| 129 | + |
| 130 | +A node related to MethodGenericDictionaryNode is DictionaryLayoutNode. The layout node doesn't get emitted into the executable, but it dictates what the dictionary nodes will have in their slots. We have one layout node per each canonical generic method body. It describes the slots in abstract terms (so instead of Container<IFoo> it would only have Container<!0>). |
| 131 | + |
| 132 | +(These things are called dictionaries, but it really is just an array of entries that we index into at runtime) |
| 133 | + |
| 134 | +The problem that we're running into with the GVMs is that when generic virtual method lookup happens, we make sure to compile the method body at compile time, but we do not make sure generic dictionaries are created. We just create the layouts and encode their contents so that the runtime type loader can create the dictionary as part of the first GVM dispatch at runtime. The unfortunate inlining that RyuJIT does makes some of the dictionary contents necessary at compile time |
| 135 | + |
| 136 | +(I think I understand the problem now, but I'm not 100% sure on a fix. So at this point it would be great if you could get to the same level of understanding so that we can start figuring out a fix . One obvious fix would be to disable all inlining into generic virtual methods, but that's not great and I have a reason to believe it affects other situations too) |
| 137 | + |
| 138 | +Michal Strehovsky |
| 139 | +What happened in Derived.TryInvoke is very different. There is no call to Base.TryInvoke because we inlined it. And through the magic of inlining, we figured out that no runtime lookup is needed, because Container<T> is Container<IFoo> and it is always IFoo, no matter what TArg the TryInvoke<TArg> … |
| 140 | +Ah, thanks - that actually makes a lot of sense now. I thought I saw the same edge in the graph when I disabled inlining for the base method, but I must have been looking at a stale output. |
| 141 | + |
| 142 | +I'm noticing that the same repro with NoInlining on Base.TryInvoke doesn't have a nongcstatics node for Container<IFoo>. Are the nongcstatic nodes only created for the direct references that don't go through the R2R helper? |
| 143 | + |
| 144 | +Michal Strehovsky |
| 145 | +MethodGenericDictionaryNode is the node that is in shared generics responsible of providing a "context" to shared generic methods. In the above example, it is expected that the MethodGenericDictionary for Base<IFoo>.TryInvoke1<object> would have one entry and that entry is the static base of |
| 146 | +Great, I can track this piece - I see these nodes created when I set breakpoints using the setup you recommended, and can see these dependencies in the graph (this is from the scanner graph but I see similar things in the codegen graph, with extra reloc edges): |
| 147 | + |
| 148 | +__NONGCSTATICStest2_Container_1<test2_IFoo> (Id=315) |
| 149 | + ← Dictionary dependency |
| 150 | + [test2]Base`1<IFoo>.TryInvoke1<object>() backed by test2_Base_1<System___Canon>__TryInvoke1<System___Canon> (Id=308) |
| 151 | + ← Dictionary contents |
| 152 | + __GenericDict_test2_Base_1<test2_IFoo>__TryInvoke1<Object> (Id=231) |
| 153 | + ← Dictionary dependency |
| 154 | + [test2]Derived.TryInvoke2<object>() backed by test2_Derived__TryInvoke2<System___Canon> (Id=167) |
| 155 | + ← Dictionary contents |
| 156 | + __GenericDict_test2_Derived__TryInvoke2<Object> (Id=164) |
| 157 | + ← call |
| 158 | + test2_Program__Main (Id=110) |
| 159 | + ← call |
| 160 | + test2__Module___MainMethodWrapper (Id=91) |
| 161 | + ← call |
| 162 | + test2__Module___StartupCodeMain (Id=9) |
| 163 | + ← |
| 164 | + Startup Code Main Method (Id=392) (root) |
| 165 | + |
| 166 | + |
| 167 | + |
| 168 | +Sven Boemer |
| 169 | +I'm noticing that the same repro with NoInlining on Base.TryInvoke doesn't have a nongcstatics node for Container<IFoo>. Are the nongcstatic nodes only created for the direct references that don't go through the R2R helper? |
| 170 | +they are created if there's a static reference to it from the graph at compile time. marking Base.TryInvoke noinlining means we don't have a static reference anymore. it will still be referred to at runtime, but at that time we'd use a nongcstatics base (and generic dictionary and....) that was allocated at runtime by the runtime type loader in system.private.typeloader |
| 171 | + |
| 172 | +When I'm stepping through the ILImporter ctor, for the method Base`1<System.__Canon>.TryInvoke<__Canon>, it sees that the uninstantiated method IL is different, so it does GetSharedRuntimeFormMethodTarget. |
| 173 | +This returns a Base`1<T_System.__Canon>.TryInvoke<__Canon>. What's the difference between those two? |
| 174 | + |
| 175 | +Michal Strehovsky |
| 176 | +The problem that we're running into with the GVMs is that when generic virtual method lookup happens, we make sure to compile the method body at compile time, but we do not make sure generic dictionaries are created. We just create the layouts and encode their contents so that the runtime type load… |
| 177 | +Ok I think I understand this comment now. I guess it isn't obvious to me why we create the dictionaries during compilation for non-virtual calls, but defer it until runtime for GVMs. Have we essentially been assuming that the JIT won't inline calls to GVMs? Is that by design or by accident? |
| 178 | + |
| 179 | +My first naiive thought for a fix is this: for the purpose of generic dictionary tracking, we should treat GVM calls like non-virtual generic method calls and create the dictionaries up-front. But I'm probably missing some more context on what makes GVMs special. |
| 180 | + |
| 181 | + |
| 182 | + |
0 commit comments