From 1ae45dff9ad0d75199897dc4bcce9033221b08de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Thu, 30 Mar 2023 05:57:42 +0900 Subject: [PATCH 1/2] Switch attribute equality to the valuetype equality plan Attribute.Equals/GetHashCode is currently implemented using reflection. The implementation iterates over all instance fields and calls Equals/GetHashCode as needed. This has problems because the code is not actually trim safe (requires special casing in the compiler to work around an invalid suppression in CoreLib) and has extra problems for reflection blocking (we need to make sure the fields are never subject to blocking). In this PR I'm switching Attribute to a similar plan that .NET Native had. In .NET Native we injected a pair of virtual methods to read and access fields on System.Attribute descendants. We can actually get away with one method - I'm reusing all the infrastructure we have for ValueType.Equals/GetHashCode since the problem is the same. Saves 5.8% on a Hello World. For BasicMinimalApi, this is a wash (almost to the byte). It will become an improvement once RyuJIT starts generating better code for the field offset computations we're doing in the injected method. Contributes to #83069. --- .../src/System.Private.CoreLib.csproj | 1 + .../src/System/Attribute.NativeAot.cs | 89 +++++++++++++++++++ .../ValueTypeGetFieldHelperMethodOverride.cs | 42 ++++++++- .../BlockedInternalsBlockingPolicy.cs | 22 +---- .../Compiler/CompilerTypeSystemContext.Aot.cs | 7 ++ ...pilerTypeSystemContext.ValueTypeMethods.cs | 40 ++++++++- .../Compiler/UsageBasedMetadataManager.cs | 38 +------- .../src/System/Attribute.cs | 2 + 8 files changed, 181 insertions(+), 60 deletions(-) create mode 100644 src/coreclr/nativeaot/System.Private.CoreLib/src/System/Attribute.NativeAot.cs diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System.Private.CoreLib.csproj b/src/coreclr/nativeaot/System.Private.CoreLib/src/System.Private.CoreLib.csproj index 9625b051d1fd06..c8933b34f95fd7 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System.Private.CoreLib.csproj +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System.Private.CoreLib.csproj @@ -130,6 +130,7 @@ + diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Attribute.NativeAot.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Attribute.NativeAot.cs new file mode 100644 index 00000000000000..d4b59427335312 --- /dev/null +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Attribute.NativeAot.cs @@ -0,0 +1,89 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; +using System.Runtime; +using System.Runtime.CompilerServices; + +using Internal.Runtime; + +namespace System +{ + public abstract partial class Attribute + { + // An override of this method will be injected by the compiler into all attributes that have fields. + // This API is a bit awkward because we want to avoid burning more than one vtable slot on this. + // When index is negative, this method is expected to return the number of fields of this + // valuetype. Otherwise, it returns the offset and type handle of the index-th field on this type. + internal virtual unsafe int __GetFieldHelper(int index, out MethodTable* mt) + { + Debug.Assert(index < 0); + mt = default; + return 0; + } + + public override unsafe bool Equals([NotNullWhen(true)] object? obj) + { + if (obj == null) + return false; + + if (this.GetType() != obj.GetType()) + return false; + + int numFields = __GetFieldHelper(-1, out _); + + // Subtract nint size to get to the m_pEEType field. Can't get from "ref MethodTable*" to "ref byte" currently. + ref byte thisRawData = ref Unsafe.Subtract(ref this.GetRawData(), nint.Size); + ref byte thatRawData = ref Unsafe.Subtract(ref obj.GetRawData(), nint.Size); + + for (int i = 0; i < numFields; i++) + { + int fieldOffset = __GetFieldHelper(i, out MethodTable* fieldType); + + // Fetch the value of the field on both types + object thisResult = RuntimeImports.RhBoxAny(ref Unsafe.Add(ref thisRawData, fieldOffset), fieldType); + object thatResult = RuntimeImports.RhBoxAny(ref Unsafe.Add(ref thatRawData, fieldOffset), fieldType); + + if (!AreFieldValuesEqual(thisResult, thatResult)) + { + return false; + } + } + + return true; + } + + public override unsafe int GetHashCode() + { + int numFields = __GetFieldHelper(-1, out _); + + // Subtract nint size to get to the m_pEEType field. Can't get from "ref MethodTable*" to "ref byte" currently. + ref byte thisRawData = ref Unsafe.Subtract(ref this.GetRawData(), nint.Size); + + object? vThis = null; + + for (int i = 0; i < numFields; i++) + { + int fieldOffset = __GetFieldHelper(i, out MethodTable* fieldType); + object? fieldValue = RuntimeImports.RhBoxAny(ref Unsafe.Add(ref thisRawData, fieldOffset), fieldType); + + // The hashcode of an array ignores the contents of the array, so it can produce + // different hashcodes for arrays with the same contents. + // Since we do deep comparisons of arrays in Equals(), this means Equals and GetHashCode will + // be inconsistent for arrays. Therefore, we ignore hashes of arrays. + if (fieldValue != null && !fieldValue.GetType().IsArray) + vThis = fieldValue; + + if (vThis != null) + break; + } + + if (vThis != null) + return vThis.GetHashCode(); + + // Matches the reflection-based implementation in other runtimes + return typeof(Attribute).GetHashCode(); + } + } +} diff --git a/src/coreclr/tools/Common/TypeSystem/IL/Stubs/ValueTypeGetFieldHelperMethodOverride.cs b/src/coreclr/tools/Common/TypeSystem/IL/Stubs/ValueTypeGetFieldHelperMethodOverride.cs index 532087549e2379..55d464df75ba95 100644 --- a/src/coreclr/tools/Common/TypeSystem/IL/Stubs/ValueTypeGetFieldHelperMethodOverride.cs +++ b/src/coreclr/tools/Common/TypeSystem/IL/Stubs/ValueTypeGetFieldHelperMethodOverride.cs @@ -67,6 +67,9 @@ public override MethodIL EmitIL() TypeDesc methodTableType = Context.SystemModule.GetKnownType("Internal.Runtime", "MethodTable"); MethodDesc methodTableOfMethod = methodTableType.GetKnownMethod("Of", null); + ILToken firstFieldToken = owningType.IsValueType ? default : + emitter.NewToken(Context.GetWellKnownType(WellKnownType.Object).GetKnownField("m_pEEType")); + var switchStream = emitter.NewCodeStream(); var getFieldStream = emitter.NewCodeStream(); @@ -107,6 +110,10 @@ public override MethodIL EmitIL() getFieldStream.EmitLdArg(0); + // If this is a reference type, we subtract from the first field. Otherwise subtract from `ref this`. + if (!owningType.IsValueType) + getFieldStream.Emit(ILOpcode.ldflda, firstFieldToken); + getFieldStream.Emit(ILOpcode.sub); getFieldStream.Emit(ILOpcode.ret); @@ -118,9 +125,38 @@ public override MethodIL EmitIL() switchStream.EmitSwitch(fieldGetters.ToArray()); } - switchStream.EmitLdc(fieldGetters.Count); - - switchStream.Emit(ILOpcode.ret); + if (!owningType.IsValueType + && MetadataVirtualMethodAlgorithm.FindSlotDefiningMethodForVirtualMethod(this) is MethodDesc slotMethod + && owningType.BaseType.FindVirtualFunctionTargetMethodOnObjectType(slotMethod) is MethodDesc baseMethod + && slotMethod != baseMethod) + { + // On reference types, we recurse into base implementation too, handling both the case of asking + // for number of fields (add number of fields on the current class before returning), and + // handling field indices higher than what we handle (subtract number of fields handled here first). + switchStream.EmitLdArg(0); + switchStream.EmitLdArg(1); + switchStream.EmitLdc(fieldGetters.Count); + switchStream.Emit(ILOpcode.sub); + switchStream.EmitLdArg(2); + switchStream.Emit(ILOpcode.call, emitter.NewToken(baseMethod)); + switchStream.EmitLdArg(1); + switchStream.EmitLdc(0); + ILCodeLabel lGotBaseFieldAddress = emitter.NewCodeLabel(); + switchStream.Emit(ILOpcode.bge, lGotBaseFieldAddress); + switchStream.EmitLdc(fieldGetters.Count); + ILCodeLabel lGotNumFieldsInBase = emitter.NewCodeLabel(); + switchStream.Emit(ILOpcode.br, lGotNumFieldsInBase); + switchStream.EmitLabel(lGotBaseFieldAddress); + switchStream.EmitLdc(0); + switchStream.EmitLabel(lGotNumFieldsInBase); + switchStream.Emit(ILOpcode.add); + switchStream.Emit(ILOpcode.ret); + } + else + { + switchStream.EmitLdc(fieldGetters.Count); + switchStream.Emit(ILOpcode.ret); + } return emitter.Link(this); } diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/BlockedInternalsBlockingPolicy.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/BlockedInternalsBlockingPolicy.cs index 7abb3b45155cb5..32145754e99883 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/BlockedInternalsBlockingPolicy.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/BlockedInternalsBlockingPolicy.cs @@ -144,14 +144,12 @@ private bool ComputeIsBlocked(EcmaType type, ModuleBlockingMode blockingMode) private BlockedTypeHashtable _blockedTypes; private MetadataType ArrayOfTType { get; } - private MetadataType AttributeType { get; } public BlockedInternalsBlockingPolicy(TypeSystemContext context) { _blockedTypes = new BlockedTypeHashtable(_blockedModules); ArrayOfTType = context.SystemModule.GetType("System", "Array`1", throwIfNotFound: false); - AttributeType = context.SystemModule.GetType("System", "Attribute", throwIfNotFound: false); } public override bool IsBlocked(MetadataType type) @@ -227,25 +225,9 @@ public override bool IsBlocked(FieldDesc field) return true; FieldAttributes accessibility = ecmaField.Attributes & FieldAttributes.FieldAccessMask; - if (accessibility != FieldAttributes.Family + return accessibility != FieldAttributes.Family && accessibility != FieldAttributes.FamORAssem - && accessibility != FieldAttributes.Public) - { - // Exempt fields on custom attributes from blocking. - // Attribute.Equals and Attribute.GetHashCode depends on being able to - // walk all fields on custom attributes using reflection. - // We're opening this hole in hopes that the fields won't have any "interesting" - // types (that would be themselves blocked). Doing that could be problematic. - if (AttributeType != null - && owningType.CanCastTo(AttributeType)) - { - return false; - } - - return true; - } - - return false; + && accessibility != FieldAttributes.Public; } } } diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/CompilerTypeSystemContext.Aot.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/CompilerTypeSystemContext.Aot.cs index 97176e404cb82d..a022eb556e4d69 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/CompilerTypeSystemContext.Aot.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/CompilerTypeSystemContext.Aot.cs @@ -38,6 +38,7 @@ public SharedGenericsConfiguration GenericsConfig private TypeDesc[] _arrayOfTInterfaces; private ArrayOfTRuntimeInterfacesAlgorithm _arrayOfTRuntimeInterfacesAlgorithm; private MetadataType _arrayOfTType; + private MetadataType _attributeType; public CompilerTypeSystemContext(TargetDetails details, SharedGenericsMode genericsMode, DelegateFeature delegateFeatures, int genericCycleCutoffPoint = DefaultGenericCycleCutoffPoint) : base(details) @@ -132,6 +133,8 @@ protected override IEnumerable GetAllVirtualMethods(TypeDesc type) [MethodImpl(MethodImplOptions.AggressiveInlining)] private IEnumerable GetAllMethods(TypeDesc type, bool virtualOnly) { + MetadataType attributeType = _attributeType ??= SystemModule.GetType("System", "Attribute"); + if (type.IsDelegate) { return GetAllMethodsForDelegate(type, virtualOnly); @@ -144,6 +147,10 @@ private IEnumerable GetAllMethods(TypeDesc type, bool virtualOnly) { return GetAllMethodsForValueType(type, virtualOnly); } + else if (type.CanCastTo(attributeType)) + { + return GetAllMethodsForAttribute(type, virtualOnly); + } return virtualOnly ? type.GetVirtualMethods() : type.GetMethods(); } diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/CompilerTypeSystemContext.ValueTypeMethods.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/CompilerTypeSystemContext.ValueTypeMethods.cs index f142691d682620..0e03ed4a6a843c 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/CompilerTypeSystemContext.ValueTypeMethods.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/CompilerTypeSystemContext.ValueTypeMethods.cs @@ -35,7 +35,7 @@ protected virtual IEnumerable GetAllMethodsForValueType(TypeDesc val { TypeDesc valueTypeDefinition = valueType.GetTypeDefinition(); - if (RequiresGetFieldHelperMethod((MetadataType)valueTypeDefinition)) + if (RequiresValueTypeGetFieldHelperMethod((MetadataType)valueTypeDefinition)) { MethodDesc getFieldHelperMethod = _valueTypeMethodHashtable.GetOrCreateValue((DefType)valueTypeDefinition); @@ -54,7 +54,30 @@ protected virtual IEnumerable GetAllMethodsForValueType(TypeDesc val yield return method; } - private bool RequiresGetFieldHelperMethod(MetadataType valueType) + protected virtual IEnumerable GetAllMethodsForAttribute(TypeDesc attributeType, bool virtualOnly) + { + TypeDesc attributeTypeDefinition = attributeType.GetTypeDefinition(); + + if (RequiresAttributeGetFieldHelperMethod(attributeTypeDefinition)) + { + MethodDesc getFieldHelperMethod = _valueTypeMethodHashtable.GetOrCreateValue((DefType)attributeTypeDefinition); + + if (attributeType != attributeTypeDefinition) + { + yield return GetMethodForInstantiatedType(getFieldHelperMethod, (InstantiatedType)attributeType); + } + else + { + yield return getFieldHelperMethod; + } + } + + IEnumerable metadataMethods = virtualOnly ? attributeType.GetVirtualMethods() : attributeType.GetMethods(); + foreach (MethodDesc method in metadataMethods) + yield return method; + } + + private bool RequiresValueTypeGetFieldHelperMethod(MetadataType valueType) { _objectEqualsMethod ??= GetWellKnownType(WellKnownType.Object).GetMethod("Equals", null); @@ -90,6 +113,19 @@ public bool IsAsyncStateMachineType(MetadataType type) && Array.IndexOf(type.RuntimeInterfaces, _iAsyncStateMachineType) >= 0; } + private static bool RequiresAttributeGetFieldHelperMethod(TypeDesc attributeTypeDef) + { + foreach (FieldDesc field in attributeTypeDef.GetFields()) + { + if (field.IsStatic) + continue; + + return true; + } + + return false; + } + private sealed class TypeState { private enum Flags diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs index de0ecf7c45684e..5403534d411eff 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs @@ -663,40 +663,8 @@ public override void GetDependenciesDueToAccess(ref DependencyList dependencies, AddDataflowDependency(ref dependencies, factory, methodIL, "Access to interesting field"); } - string reason = "Use of a field"; - - bool generatesMetadata = false; - if (!IsReflectionBlocked(writtenField)) - { - if ((_generationOptions & UsageBasedMetadataGenerationOptions.CreateReflectableArtifacts) != 0) - { - // If access to the field should trigger metadata generation, we should generate the field - generatesMetadata = true; - } - else - { - // There's an invalid suppression in the CoreLib that assumes used fields on attributes will be kept. - // It's used in the reflection-based implementation of Attribute.Equals and Attribute.GetHashCode. - // .NET Native used to have a non-reflection based implementation of Equals/GetHashCode to get around - // this problem. We could explore that as well, but for now, emulate the fact that accessed fields - // on custom attributes will be visible in reflection metadata. - MetadataType currentType = (MetadataType)writtenField.OwningType.BaseType; - while (currentType != null) - { - if (currentType.Module == factory.TypeSystemContext.SystemModule - && currentType.Name == "Attribute" && currentType.Namespace == "System") - { - generatesMetadata = true; - reason = "Field of an attribute"; - break; - } - - currentType = currentType.MetadataBaseType; - } - } - } - - if (generatesMetadata) + if ((_generationOptions & UsageBasedMetadataGenerationOptions.CreateReflectableArtifacts) != 0 + && !IsReflectionBlocked(writtenField)) { FieldDesc fieldToReport = writtenField; @@ -714,7 +682,7 @@ public override void GetDependenciesDueToAccess(ref DependencyList dependencies, } dependencies ??= new DependencyList(); - dependencies.Add(factory.ReflectedField(fieldToReport), reason); + dependencies.Add(factory.ReflectedField(fieldToReport), "Use of a field"); } if (writtenField.GetTypicalFieldDefinition() is EcmaField ecmaField) diff --git a/src/libraries/System.Private.CoreLib/src/System/Attribute.cs b/src/libraries/System.Private.CoreLib/src/System/Attribute.cs index 2bc115b9f686dc..d9e2e0e43dfd07 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Attribute.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Attribute.cs @@ -14,6 +14,7 @@ public abstract partial class Attribute { protected Attribute() { } +#if !NATIVEAOT [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2075:UnrecognizedReflectionPattern", Justification = "Unused fields don't make a difference for equality")] public override bool Equals([NotNullWhen(true)] object? obj) @@ -82,6 +83,7 @@ public override int GetHashCode() return type.GetHashCode(); } +#endif // Compares values of custom-attribute fields. private static bool AreFieldValuesEqual(object? thisValue, object? thatValue) From 6e551586ec40860aa79995b73acd800b222a46fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Thu, 30 Mar 2023 15:17:06 +0900 Subject: [PATCH 2/2] FB --- .../src/System.Private.CoreLib.csproj | 2 +- .../src/System/Attribute.NativeAot.cs | 8 +++----- .../IL/Stubs/ValueTypeGetFieldHelperMethodOverride.cs | 6 +++--- 3 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System.Private.CoreLib.csproj b/src/coreclr/nativeaot/System.Private.CoreLib/src/System.Private.CoreLib.csproj index c8933b34f95fd7..1faf04ea61a651 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System.Private.CoreLib.csproj +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System.Private.CoreLib.csproj @@ -130,7 +130,6 @@ - @@ -159,6 +158,7 @@ + diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Attribute.NativeAot.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Attribute.NativeAot.cs index d4b59427335312..0774e107f7d231 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Attribute.NativeAot.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Attribute.NativeAot.cs @@ -33,9 +33,8 @@ public override unsafe bool Equals([NotNullWhen(true)] object? obj) int numFields = __GetFieldHelper(-1, out _); - // Subtract nint size to get to the m_pEEType field. Can't get from "ref MethodTable*" to "ref byte" currently. - ref byte thisRawData = ref Unsafe.Subtract(ref this.GetRawData(), nint.Size); - ref byte thatRawData = ref Unsafe.Subtract(ref obj.GetRawData(), nint.Size); + ref byte thisRawData = ref this.GetRawData(); + ref byte thatRawData = ref obj.GetRawData(); for (int i = 0; i < numFields; i++) { @@ -58,8 +57,7 @@ public override unsafe int GetHashCode() { int numFields = __GetFieldHelper(-1, out _); - // Subtract nint size to get to the m_pEEType field. Can't get from "ref MethodTable*" to "ref byte" currently. - ref byte thisRawData = ref Unsafe.Subtract(ref this.GetRawData(), nint.Size); + ref byte thisRawData = ref this.GetRawData(); object? vThis = null; diff --git a/src/coreclr/tools/Common/TypeSystem/IL/Stubs/ValueTypeGetFieldHelperMethodOverride.cs b/src/coreclr/tools/Common/TypeSystem/IL/Stubs/ValueTypeGetFieldHelperMethodOverride.cs index 55d464df75ba95..a66ce62ea00ef1 100644 --- a/src/coreclr/tools/Common/TypeSystem/IL/Stubs/ValueTypeGetFieldHelperMethodOverride.cs +++ b/src/coreclr/tools/Common/TypeSystem/IL/Stubs/ValueTypeGetFieldHelperMethodOverride.cs @@ -67,8 +67,8 @@ public override MethodIL EmitIL() TypeDesc methodTableType = Context.SystemModule.GetKnownType("Internal.Runtime", "MethodTable"); MethodDesc methodTableOfMethod = methodTableType.GetKnownMethod("Of", null); - ILToken firstFieldToken = owningType.IsValueType ? default : - emitter.NewToken(Context.GetWellKnownType(WellKnownType.Object).GetKnownField("m_pEEType")); + ILToken rawDataToken = owningType.IsValueType ? default : + emitter.NewToken(Context.SystemModule.GetKnownType("System.Runtime.CompilerServices", "RawData").GetKnownField("Data")); var switchStream = emitter.NewCodeStream(); var getFieldStream = emitter.NewCodeStream(); @@ -112,7 +112,7 @@ public override MethodIL EmitIL() // If this is a reference type, we subtract from the first field. Otherwise subtract from `ref this`. if (!owningType.IsValueType) - getFieldStream.Emit(ILOpcode.ldflda, firstFieldToken); + getFieldStream.Emit(ILOpcode.ldflda, rawDataToken); getFieldStream.Emit(ILOpcode.sub);