From a5b2c101ebfb990ffd9d7b21e862378a517a3c1a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Sun, 4 Feb 2024 22:57:34 +0100 Subject: [PATCH 1/2] Place vtable slots deemed final by analysis into sealed vtable Virtual method table slots typically go into vtable - a variable-length part of the `MethodTable` data structure. Virtual method table slots are always inherited by `MethodTable` of the derived class. This means that vtable slots that are introduced in a class that has many derived classes can be quite expensive. One example are arrays - array types inherit all virtual methods from `System.Array` and there's many arrays. We therefore have an optimization - if a virtual method is newly introduced, but it's sealed at the same time, we place it into a separate data structure - a "sealed vtable". Sealed vtable slots do not get inherited by derived classes. Because they don't get inherited, we also need to make sure all non-interface calls into this virtual method get devirtualized (virtual dispatch needs to be aware of the special lookup that happens for sealed slots and only interface resolution is aware of how to do that). This PR makes this optimization kick in more often - not just when method is `virtual sealed` in metadata (which effectively only happens for non-virtual method in C# implementing an interface), but also when it's virtual and nothing else overrides it in the program. --- .../tools/Common/Compiler/MethodExtensions.cs | 12 ++++++++++-- .../aot/ILCompiler.Compiler/Compiler/Compilation.cs | 2 +- .../Compiler/DependencyAnalysis/EETypeNode.cs | 4 ++-- .../DependencyAnalysis/NativeLayoutVertexNode.cs | 2 +- .../DependencyAnalysis/ReflectedMethodNode.cs | 2 +- .../DependencyAnalysis/ReflectionInvokeMapNode.cs | 2 +- .../ReflectionVirtualInvokeMapNode.cs | 11 ++++------- .../Compiler/DependencyAnalysis/SealedVTableNode.cs | 8 ++++---- .../Target_ARM/ARMReadyToRunHelperNode.cs | 6 +++--- .../Target_ARM64/ARM64ReadyToRunHelperNode.cs | 6 +++--- .../LoongArch64ReadyToRunHelperNode.cs | 6 +++--- .../Target_RiscV64/RiscV64ReadyToRunHelperNode.cs | 6 +++--- .../Target_X64/X64ReadyToRunHelperNode.cs | 6 +++--- .../Compiler/VirtualMethodCallHelper.cs | 8 ++++---- .../JitInterface/CorInfoImpl.RyuJit.cs | 2 +- 15 files changed, 44 insertions(+), 39 deletions(-) diff --git a/src/coreclr/tools/Common/Compiler/MethodExtensions.cs b/src/coreclr/tools/Common/Compiler/MethodExtensions.cs index dd43f2d4c2ae8e..6df9586fb256eb 100644 --- a/src/coreclr/tools/Common/Compiler/MethodExtensions.cs +++ b/src/coreclr/tools/Common/Compiler/MethodExtensions.cs @@ -1,9 +1,13 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using ILCompiler.DependencyAnalysis; + using Internal.TypeSystem; using Internal.TypeSystem.Ecma; +using Debug = System.Diagnostics.Debug; + namespace ILCompiler { public static class MethodExtensions @@ -73,6 +77,7 @@ public static string GetUnmanagedCallersOnlyExportName(this EcmaMethod This) return null; } +#if !READYTORUN /// /// Determine whether a method can go into the sealed vtable of a type. Such method must be a sealed virtual /// method that is not overriding any method on a base type. @@ -82,15 +87,17 @@ public static string GetUnmanagedCallersOnlyExportName(this EcmaMethod This) /// since all of their collection interface methods are sealed and implemented on the System.Array and /// System.Array<T> base types, and therefore we can minimize the vtable sizes of all derived array types. /// - public static bool CanMethodBeInSealedVTable(this MethodDesc method) + public static bool CanMethodBeInSealedVTable(this MethodDesc method, NodeFactory factory) { + Debug.Assert(!method.OwningType.ContainsSignatureVariables(treatGenericParameterLikeSignatureVariable: true)); + bool isInterfaceMethod = method.OwningType.IsInterface; // Methods on interfaces never go into sealed vtable // We would hit this code path for default implementations of interface methods (they are newslot+final). // Interface types don't get physical slots, but they have logical slot numbers and that logic shouldn't // attempt to place final+newslot methods differently. - if (method.IsFinal && method.IsNewSlot && !isInterfaceMethod) + if (method.IsNewSlot && !isInterfaceMethod && factory.DevirtualizationManager.IsEffectivelySealed(method)) return true; // Implementations of static virtual method also go into the sealed vtable. @@ -101,6 +108,7 @@ public static bool CanMethodBeInSealedVTable(this MethodDesc method) return false; } +#endif public static bool NotCallableWithoutOwningEEType(this MethodDesc method) { diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Compilation.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Compilation.cs index 98d2e96faadfa1..19bb6506286202 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Compilation.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Compilation.cs @@ -113,7 +113,7 @@ public DelegateCreationInfo GetDelegateCtor(TypeDesc delegateType, MethodDesc ta // If we're creating a delegate to a virtual method that cannot be overridden, devirtualize. // This is not just an optimization - it's required for correctness in the presence of sealed // vtable slots. - if (followVirtualDispatch && (target.IsFinal || target.OwningType.IsSealed())) + if (followVirtualDispatch && NodeFactory.DevirtualizationManager.IsEffectivelySealed(target)) followVirtualDispatch = false; if (followVirtualDispatch) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/EETypeNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/EETypeNode.cs index 4cf85b1177912a..8149c2c7886ddf 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/EETypeNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/EETypeNode.cs @@ -407,7 +407,7 @@ public sealed override IEnumerable GetConditionalSt // We don't do this if the method can be placed in the sealed vtable since // those can never be overriden by children anyway. bool canUseTentativeMethod = isNonInterfaceAbstractType - && !decl.CanMethodBeInSealedVTable() + && !decl.CanMethodBeInSealedVTable(factory) && factory.CompilationModuleGroup.AllowVirtualMethodOnAbstractTypeOptimization(canonImpl); IMethodNode implNode = canUseTentativeMethod ? factory.TentativeMethodEntrypoint(canonImpl, impl.OwningType.IsValueType) : @@ -1039,7 +1039,7 @@ private void OutputVirtualSlots(NodeFactory factory, ref ObjectDataBuilder objDa // Final NewSlot methods cannot be overridden, and therefore can be placed in the sealed-vtable to reduce the size of the vtable // of this type and any type that inherits from it. - if (declMethod.CanMethodBeInSealedVTable() && !declType.IsArrayTypeWithoutGenericInterfaces()) + if (declMethod.CanMethodBeInSealedVTable(factory) && !declType.IsArrayTypeWithoutGenericInterfaces()) continue; bool shouldEmitImpl = !implMethod.IsAbstract; diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NativeLayoutVertexNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NativeLayoutVertexNode.cs index 574176b4c6ee11..1248906a4fba37 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NativeLayoutVertexNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NativeLayoutVertexNode.cs @@ -1433,7 +1433,7 @@ private static void ProcessVTableEntriesForCallingConventionSignatureGeneration( MethodDesc implMethod = closestDefType.FindVirtualFunctionTargetMethodOnObjectType(declMethod); - if (implMethod.CanMethodBeInSealedVTable() && !implType.IsArrayTypeWithoutGenericInterfaces()) + if (implMethod.CanMethodBeInSealedVTable(factory) && !implType.IsArrayTypeWithoutGenericInterfaces()) { // Sealed vtable entries on other types in the hierarchy should not be reported (types read entries // from their own sealed vtables, and not from the sealed vtables of base types). diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectedMethodNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectedMethodNode.cs index 8c5a55d60335cb..79c291cdcf332e 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectedMethodNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectedMethodNode.cs @@ -68,7 +68,7 @@ public override IEnumerable GetStaticDependencies(NodeFacto } else { - if (ReflectionVirtualInvokeMapNode.NeedsVirtualInvokeInfo(slotDefiningMethod) && !factory.VTable(slotDefiningMethod.OwningType).HasFixedSlots) + if (ReflectionVirtualInvokeMapNode.NeedsVirtualInvokeInfo(factory, slotDefiningMethod) && !factory.VTable(slotDefiningMethod.OwningType).HasFixedSlots) dependencies.Add(factory.VirtualMethodUse(slotDefiningMethod), "Virtually callable reflectable method"); } } diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionInvokeMapNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionInvokeMapNode.cs index ceb0e7ecf76ee7..e9d5b45d3ee802 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionInvokeMapNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionInvokeMapNode.cs @@ -156,7 +156,7 @@ public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false) if (method.IsDefaultConstructor) flags |= InvokeTableFlags.IsDefaultConstructor; - if (ReflectionVirtualInvokeMapNode.NeedsVirtualInvokeInfo(method)) + if (ReflectionVirtualInvokeMapNode.NeedsVirtualInvokeInfo(factory, method)) flags |= InvokeTableFlags.HasVirtualInvoke; if (!method.IsAbstract) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionVirtualInvokeMapNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionVirtualInvokeMapNode.cs index 9e62841554db2f..b2f8e973c16724 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionVirtualInvokeMapNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionVirtualInvokeMapNode.cs @@ -39,15 +39,12 @@ public void AppendMangledName(NameMangler nameMangler, Utf8StringBuilder sb) public override bool StaticDependenciesAreComputed => true; protected override string GetName(NodeFactory factory) => this.GetMangledName(factory.NameMangler); - public static bool NeedsVirtualInvokeInfo(MethodDesc method) + public static bool NeedsVirtualInvokeInfo(NodeFactory factory, MethodDesc method) { if (!method.IsVirtual) return false; - if (method.IsFinal) - return false; - - if (method.OwningType.IsSealed()) + if (factory.DevirtualizationManager.IsEffectivelySealed(method)) return false; return true; @@ -83,7 +80,7 @@ public static MethodDesc GetDeclaringVirtualMethodAndHierarchyDistance(MethodDes public static void GetVirtualInvokeMapDependencies(ref DependencyList dependencies, NodeFactory factory, MethodDesc method) { - if (NeedsVirtualInvokeInfo(method)) + if (NeedsVirtualInvokeInfo(factory, method)) { dependencies ??= new DependencyList(); @@ -137,7 +134,7 @@ public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false) continue; // Only virtual methods are interesting - if (!NeedsVirtualInvokeInfo(method)) + if (!NeedsVirtualInvokeInfo(factory, method)) continue; // diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/SealedVTableNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/SealedVTableNode.cs index d27944d434e81d..85c9e40eec058f 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/SealedVTableNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/SealedVTableNode.cs @@ -125,7 +125,7 @@ public bool BuildSealedVTableSlots(NodeFactory factory, bool relocsOnly) MethodDesc implMethod = declType.FindVirtualFunctionTargetMethodOnObjectType(virtualSlots[i]); - if (implMethod.CanMethodBeInSealedVTable()) + if (implMethod.CanMethodBeInSealedVTable(factory)) _sealedVTableEntries.Add(SealedVTableEntry.FromVirtualMethod(implMethod)); } @@ -162,8 +162,7 @@ public bool BuildSealedVTableSlots(NodeFactory factory, bool relocsOnly) // dispatch will walk the inheritance chain). if (implMethod != null) { - if (implMethod.Signature.IsStatic || - (implMethod.CanMethodBeInSealedVTable() && !implMethod.OwningType.HasSameTypeDefinition(declType))) + if (implMethod.Signature.IsStatic || !implMethod.OwningType.HasSameTypeDefinition(declType)) { TypeDesc implType = declType; while (!implType.HasSameTypeDefinition(implMethod.OwningType)) @@ -173,7 +172,8 @@ public bool BuildSealedVTableSlots(NodeFactory factory, bool relocsOnly) if (!implType.IsTypeDefinition) targetMethod = factory.TypeSystemContext.GetMethodForInstantiatedType(implMethod.GetTypicalMethodDefinition(), (InstantiatedType)implType); - _sealedVTableEntries.Add(SealedVTableEntry.FromVirtualMethod(targetMethod)); + if (targetMethod.CanMethodBeInSealedVTable(factory) || implMethod.Signature.IsStatic) + _sealedVTableEntries.Add(SealedVTableEntry.FromVirtualMethod(targetMethod)); } } else diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_ARM/ARMReadyToRunHelperNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_ARM/ARMReadyToRunHelperNode.cs index 8323c6057803d2..d6ecb47a38d612 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_ARM/ARMReadyToRunHelperNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_ARM/ARMReadyToRunHelperNode.cs @@ -22,7 +22,7 @@ protected override void EmitCode(NodeFactory factory, ref ARMEmitter encoder, bo MethodDesc targetMethod = (MethodDesc)Target; Debug.Assert(!targetMethod.OwningType.IsInterface); - Debug.Assert(!targetMethod.CanMethodBeInSealedVTable()); + Debug.Assert(!targetMethod.CanMethodBeInSealedVTable(factory)); int pointerSize = factory.Target.PointerSize; @@ -126,7 +126,7 @@ protected override void EmitCode(NodeFactory factory, ref ARMEmitter encoder, bo if (target.TargetNeedsVTableLookup) { - Debug.Assert(!target.TargetMethod.CanMethodBeInSealedVTable()); + Debug.Assert(!target.TargetMethod.CanMethodBeInSealedVTable(factory)); encoder.EmitLDR(encoder.TargetRegister.Arg2, encoder.TargetRegister.Arg1); @@ -175,7 +175,7 @@ protected override void EmitCode(NodeFactory factory, ref ARMEmitter encoder, bo encoder.EmitLDR(encoder.TargetRegister.Result, encoder.TargetRegister.Arg0); - Debug.Assert(!targetMethod.CanMethodBeInSealedVTable()); + Debug.Assert(!targetMethod.CanMethodBeInSealedVTable(factory)); int slot = VirtualMethodSlotHelper.GetVirtualMethodSlot(factory, targetMethod, targetMethod.OwningType); Debug.Assert(slot != -1); diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_ARM64/ARM64ReadyToRunHelperNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_ARM64/ARM64ReadyToRunHelperNode.cs index 0fca02c971a4f1..6d94afdc3ee4be 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_ARM64/ARM64ReadyToRunHelperNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_ARM64/ARM64ReadyToRunHelperNode.cs @@ -23,7 +23,7 @@ protected override void EmitCode(NodeFactory factory, ref ARM64Emitter encoder, MethodDesc targetMethod = (MethodDesc)Target; Debug.Assert(!targetMethod.OwningType.IsInterface); - Debug.Assert(!targetMethod.CanMethodBeInSealedVTable()); + Debug.Assert(!targetMethod.CanMethodBeInSealedVTable(factory)); int pointerSize = factory.Target.PointerSize; @@ -161,7 +161,7 @@ protected override void EmitCode(NodeFactory factory, ref ARM64Emitter encoder, if (target.TargetNeedsVTableLookup) { - Debug.Assert(!target.TargetMethod.CanMethodBeInSealedVTable()); + Debug.Assert(!target.TargetMethod.CanMethodBeInSealedVTable(factory)); encoder.EmitLDR(encoder.TargetRegister.Arg2, encoder.TargetRegister.Arg1); @@ -210,7 +210,7 @@ protected override void EmitCode(NodeFactory factory, ref ARM64Emitter encoder, encoder.EmitLDR(encoder.TargetRegister.Result, encoder.TargetRegister.Arg0); - Debug.Assert(!targetMethod.CanMethodBeInSealedVTable()); + Debug.Assert(!targetMethod.CanMethodBeInSealedVTable(factory)); int slot = VirtualMethodSlotHelper.GetVirtualMethodSlot(factory, targetMethod, targetMethod.OwningType); Debug.Assert(slot != -1); diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_LoongArch64/LoongArch64ReadyToRunHelperNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_LoongArch64/LoongArch64ReadyToRunHelperNode.cs index 49505b9dff7760..2b54f9ccdcc634 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_LoongArch64/LoongArch64ReadyToRunHelperNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_LoongArch64/LoongArch64ReadyToRunHelperNode.cs @@ -23,7 +23,7 @@ protected override void EmitCode(NodeFactory factory, ref LoongArch64Emitter enc MethodDesc targetMethod = (MethodDesc)Target; Debug.Assert(!targetMethod.OwningType.IsInterface); - Debug.Assert(!targetMethod.CanMethodBeInSealedVTable()); + Debug.Assert(!targetMethod.CanMethodBeInSealedVTable(factory)); int pointerSize = factory.Target.PointerSize; @@ -133,7 +133,7 @@ protected override void EmitCode(NodeFactory factory, ref LoongArch64Emitter enc if (target.TargetNeedsVTableLookup) { - Debug.Assert(!target.TargetMethod.CanMethodBeInSealedVTable()); + Debug.Assert(!target.TargetMethod.CanMethodBeInSealedVTable(factory)); encoder.EmitLD(encoder.TargetRegister.Arg2, encoder.TargetRegister.Arg1, 0); @@ -182,7 +182,7 @@ protected override void EmitCode(NodeFactory factory, ref LoongArch64Emitter enc encoder.EmitLD(encoder.TargetRegister.Result, encoder.TargetRegister.Arg0, 0); - Debug.Assert(!targetMethod.CanMethodBeInSealedVTable()); + Debug.Assert(!targetMethod.CanMethodBeInSealedVTable(factory)); int slot = VirtualMethodSlotHelper.GetVirtualMethodSlot(factory, targetMethod, targetMethod.OwningType); Debug.Assert(slot != -1); diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_RiscV64/RiscV64ReadyToRunHelperNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_RiscV64/RiscV64ReadyToRunHelperNode.cs index e4288acba29a36..1d430fb1f96fa4 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_RiscV64/RiscV64ReadyToRunHelperNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_RiscV64/RiscV64ReadyToRunHelperNode.cs @@ -23,7 +23,7 @@ protected override void EmitCode(NodeFactory factory, ref RiscV64Emitter encoder MethodDesc targetMethod = (MethodDesc)Target; Debug.Assert(!targetMethod.OwningType.IsInterface); - Debug.Assert(!targetMethod.CanMethodBeInSealedVTable()); + Debug.Assert(!targetMethod.CanMethodBeInSealedVTable(factory)); int pointerSize = factory.Target.PointerSize; @@ -130,7 +130,7 @@ protected override void EmitCode(NodeFactory factory, ref RiscV64Emitter encoder if (target.TargetNeedsVTableLookup) { - Debug.Assert(!target.TargetMethod.CanMethodBeInSealedVTable()); + Debug.Assert(!target.TargetMethod.CanMethodBeInSealedVTable(factory)); encoder.EmitLD(encoder.TargetRegister.Arg2, encoder.TargetRegister.Arg1, 0); @@ -179,7 +179,7 @@ protected override void EmitCode(NodeFactory factory, ref RiscV64Emitter encoder encoder.EmitLD(encoder.TargetRegister.Result, encoder.TargetRegister.Arg0, 0); - Debug.Assert(!targetMethod.CanMethodBeInSealedVTable()); + Debug.Assert(!targetMethod.CanMethodBeInSealedVTable(factory)); int slot = VirtualMethodSlotHelper.GetVirtualMethodSlot(factory, targetMethod, targetMethod.OwningType); Debug.Assert(slot != -1); diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_X64/X64ReadyToRunHelperNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_X64/X64ReadyToRunHelperNode.cs index 6bab293d079e3b..66265e9d71bb49 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_X64/X64ReadyToRunHelperNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_X64/X64ReadyToRunHelperNode.cs @@ -23,7 +23,7 @@ protected override void EmitCode(NodeFactory factory, ref X64Emitter encoder, bo MethodDesc targetMethod = (MethodDesc)Target; Debug.Assert(!targetMethod.OwningType.IsInterface); - Debug.Assert(!targetMethod.CanMethodBeInSealedVTable()); + Debug.Assert(!targetMethod.CanMethodBeInSealedVTable(factory)); AddrMode loadFromThisPtr = new AddrMode(encoder.TargetRegister.Arg0, null, 0, 0, AddrModeSize.Int64); encoder.EmitMOV(encoder.TargetRegister.Result, ref loadFromThisPtr); @@ -162,7 +162,7 @@ protected override void EmitCode(NodeFactory factory, ref X64Emitter encoder, bo if (target.TargetNeedsVTableLookup) { - Debug.Assert(!target.TargetMethod.CanMethodBeInSealedVTable()); + Debug.Assert(!target.TargetMethod.CanMethodBeInSealedVTable(factory)); AddrMode loadFromThisPtr = new AddrMode(encoder.TargetRegister.Arg1, null, 0, 0, AddrModeSize.Int64); encoder.EmitMOV(encoder.TargetRegister.Arg2, ref loadFromThisPtr); @@ -210,7 +210,7 @@ protected override void EmitCode(NodeFactory factory, ref X64Emitter encoder, bo AddrMode loadFromThisPtr = new AddrMode(encoder.TargetRegister.Arg0, null, 0, 0, AddrModeSize.Int64); encoder.EmitMOV(encoder.TargetRegister.Result, ref loadFromThisPtr); - Debug.Assert(!targetMethod.CanMethodBeInSealedVTable()); + Debug.Assert(!targetMethod.CanMethodBeInSealedVTable(factory)); int slot = VirtualMethodSlotHelper.GetVirtualMethodSlot(factory, targetMethod, targetMethod.OwningType); Debug.Assert(slot != -1); diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/VirtualMethodCallHelper.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/VirtualMethodCallHelper.cs index ec398d37433966..6a9f36af5c40ac 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/VirtualMethodCallHelper.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/VirtualMethodCallHelper.cs @@ -34,7 +34,7 @@ public static int GetDefaultInterfaceMethodSlot(NodeFactory factory, MethodDesc /// public static int GetVirtualMethodSlot(NodeFactory factory, MethodDesc method, TypeDesc implType, bool countDictionarySlots = true) { - if (method.CanMethodBeInSealedVTable()) + if (method.CanMethodBeInSealedVTable(factory)) { // If the method is a sealed newslot method, it will be put in the sealed vtable instead of the type's vtable. In this // case, the slot index return should be the index in the sealed vtable, plus the total number of vtable slots. @@ -72,7 +72,7 @@ public static int GetVirtualMethodSlot(NodeFactory factory, MethodDesc method, T int numSealedVTableEntries = 0; for (int slot = 0; slot < virtualSlots.Count; slot++) { - if (virtualSlots[slot].CanMethodBeInSealedVTable()) + if (virtualSlots[slot].CanMethodBeInSealedVTable(factory)) { numSealedVTableEntries++; continue; @@ -110,7 +110,7 @@ private static int GetNumberOfSlotsInCurrentType(NodeFactory factory, TypeDesc i IReadOnlyList virtualSlots = factory.VTable(implType).Slots; for (int slot = 0; slot < virtualSlots.Count; slot++) { - if (virtualSlots[slot].CanMethodBeInSealedVTable()) + if (virtualSlots[slot].CanMethodBeInSealedVTable(factory)) continue; numVTableSlots++; } @@ -163,7 +163,7 @@ private static int GetNumberOfBaseSlots(NodeFactory factory, TypeDesc owningType foreach (var vtableMethod in baseVirtualSlots) { // Methods in the sealed vtable should be excluded from the count - if (vtableMethod.CanMethodBeInSealedVTable()) + if (vtableMethod.CanMethodBeInSealedVTable(factory)) continue; baseSlots++; } diff --git a/src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs b/src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs index 7da670a1d6aa3c..14676fd8fe917f 100644 --- a/src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs +++ b/src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs @@ -1836,7 +1836,7 @@ private void getMethodVTableOffset(CORINFO_METHOD_STRUCT_* method, ref uint offs // Normalize to the slot defining method. We don't have slot information for the overrides. methodDesc = MetadataVirtualMethodAlgorithm.FindSlotDefiningMethodForVirtualMethod(methodDesc); - Debug.Assert(!methodDesc.CanMethodBeInSealedVTable()); + Debug.Assert(!methodDesc.CanMethodBeInSealedVTable(_compilation.NodeFactory)); // Avoid asking about slots on types like Foo. We might not have that information. // Canonically-equivalent types have the same slots, so ask for Foo<__Canon, __Canon>. From 2c3cf8d63c459475ee1de50d1b7d26ac2fd81fea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Mon, 5 Feb 2024 07:29:28 +0100 Subject: [PATCH 2/2] Tweak to CanMethodBeInSealedVTable --- .../tools/Common/Compiler/MethodExtensions.cs | 35 +++++++++++++------ 1 file changed, 25 insertions(+), 10 deletions(-) diff --git a/src/coreclr/tools/Common/Compiler/MethodExtensions.cs b/src/coreclr/tools/Common/Compiler/MethodExtensions.cs index 6df9586fb256eb..160b6859871256 100644 --- a/src/coreclr/tools/Common/Compiler/MethodExtensions.cs +++ b/src/coreclr/tools/Common/Compiler/MethodExtensions.cs @@ -91,19 +91,34 @@ public static bool CanMethodBeInSealedVTable(this MethodDesc method, NodeFactory { Debug.Assert(!method.OwningType.ContainsSignatureVariables(treatGenericParameterLikeSignatureVariable: true)); - bool isInterfaceMethod = method.OwningType.IsInterface; + TypeDesc owningType = method.OwningType; + + // Interface types don't have physical slots so we never optimize to sealed slots + if (owningType.IsInterface) + return false; - // Methods on interfaces never go into sealed vtable - // We would hit this code path for default implementations of interface methods (they are newslot+final). - // Interface types don't get physical slots, but they have logical slot numbers and that logic shouldn't - // attempt to place final+newslot methods differently. - if (method.IsNewSlot && !isInterfaceMethod && factory.DevirtualizationManager.IsEffectivelySealed(method)) + // Implementations of static virtual methods go into the sealed vtable. + if (method.Signature.IsStatic) return true; - // Implementations of static virtual method also go into the sealed vtable. - // Again, we don't let that happen for interface methods because the slot numbers are only logical, - // not physical. - if (method.Signature.IsStatic && !isInterfaceMethod) + // If the owning type is already considered sealed, there's little benefit in placing the slots + // in the sealed vtable: the sealed vtable has these properties: + // + // 1. We don't need to repeat them in derived classes. + // 2. The slots use 4-byte relative pointers, so they can be smaller. + // 3. The sealed vtable is shared among canonically-equivalent types. + // + // Benefit 1 doesn't apply to sealed types by definition. Benefit 2 doesn't manifest itself + // when data dehydration is enabled (which is the default) since pointers are compressed either way. + // Benefit 3 is still real, so we condition this opt out on type not having a canonical form. + if (factory.DevirtualizationManager.IsEffectivelySealed(owningType) + && !owningType.ConvertToCanonForm(CanonicalFormKind.Specific).IsCanonicalSubtype(CanonicalFormKind.Any)) + { + return false; + } + + // Newslot final methods go into the sealed vtable. + if (method.IsNewSlot && factory.DevirtualizationManager.IsEffectivelySealed(method)) return true; return false;