From f91b5feaf8be48fb9b03e94cbd82d5f3ad4d846e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Tue, 20 Dec 2022 16:07:40 +0900 Subject: [PATCH 1/4] Fix size regression from enum sorting Use a specialized comparer instead of `Comparer.Default` that brings the implementation of `IComparable.ComparerTo` on everything. Also get rid of the generic virtual method call. --- .../Core/Execution/ExecutionEnvironment.cs | 4 +-- .../ReflectionCoreCallbacksImplementation.cs | 31 ++++++++++++++++++- ...cutionEnvironmentImplementation.Runtime.cs | 20 +++++++++--- .../Execution/NativeFormatEnumInfo.cs | 30 ++++-------------- 4 files changed, 53 insertions(+), 32 deletions(-) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Reflection/Core/Execution/ExecutionEnvironment.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Reflection/Core/Execution/ExecutionEnvironment.cs index 61713b1c654a4..e42fbeea9dfd2 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Reflection/Core/Execution/ExecutionEnvironment.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Reflection/Core/Execution/ExecutionEnvironment.cs @@ -98,9 +98,7 @@ public abstract class ExecutionEnvironment // Other //============================================================================================== public abstract FieldAccessor CreateLiteralFieldAccessor(object value, RuntimeTypeHandle fieldTypeHandle); - public abstract EnumInfo GetEnumInfo(RuntimeTypeHandle typeHandle) - where TUnderlyingValue : struct, INumber; - + public abstract void GetEnumInfo(RuntimeTypeHandle typeHandle, out string[] names, out object[] values, out bool isFlags); public abstract IntPtr GetDynamicInvokeThunk(MethodInvoker invoker); //============================================================================================== diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/General/ReflectionCoreCallbacksImplementation.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/General/ReflectionCoreCallbacksImplementation.cs index 53f932b4f99d3..bd3d2918a95d9 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/General/ReflectionCoreCallbacksImplementation.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/General/ReflectionCoreCallbacksImplementation.cs @@ -412,11 +412,40 @@ public sealed override EnumInfo GetEnumInfo( if (info != null) return info; - info = ReflectionCoreExecution.ExecutionDomain.ExecutionEnvironment.GetEnumInfo(runtimeType.TypeHandle); + ReflectionCoreExecution.ExecutionDomain.ExecutionEnvironment.GetEnumInfo(runtimeType.TypeHandle, out string[] unsortedNames, out object[] unsortedValues, out bool isFlags); + + // Call into IntrospectiveSort directly to avoid the Comparer.Default codepath. + // That codepath would bring functionality to compare everything that was ever allocated in the program. + ArraySortHelper.IntrospectiveSort(unsortedValues, unsortedNames, EnumUnderlyingTypeComparer.Instance); + + // Only after we've sorted, create the underlying array. + var values = new TUnderlyingValue[unsortedValues.Length]; + for (int i = 0; i < unsortedValues.Length; i++) + values[i] = (TUnderlyingValue)unsortedValues[i]; + + info = new EnumInfo(type, values, unsortedNames, isFlags); runtimeType.GenericCache = info; return info; } + private class EnumUnderlyingTypeComparer : IComparer + { + public static EnumUnderlyingTypeComparer Instance = new EnumUnderlyingTypeComparer(); + + public int Compare(object? x, object? y) + => x switch + { + byte b => b.CompareTo((byte)y!), + sbyte sb => sb.CompareTo((sbyte)y!), + short s => s.CompareTo((short)y!), + ushort us => us.CompareTo((ushort)y!), + int i => i.CompareTo((int)y!), + uint ui => ui.CompareTo((uint)y!), + long l => l.CompareTo((long)y!), + _ => ((ulong)x!).CompareTo((ulong)y!), + }; + } + public sealed override DynamicInvokeInfo GetDelegateDynamicInvokeInfo(Type type) { RuntimeTypeInfo runtimeType = type.CastToRuntimeTypeInfo(); diff --git a/src/coreclr/nativeaot/System.Private.Reflection.Execution/src/Internal/Reflection/Execution/ExecutionEnvironmentImplementation.Runtime.cs b/src/coreclr/nativeaot/System.Private.Reflection.Execution/src/Internal/Reflection/Execution/ExecutionEnvironmentImplementation.Runtime.cs index 5abd1bc94521b..c47e5f72a1504 100644 --- a/src/coreclr/nativeaot/System.Private.Reflection.Execution/src/Internal/Reflection/Execution/ExecutionEnvironmentImplementation.Runtime.cs +++ b/src/coreclr/nativeaot/System.Private.Reflection.Execution/src/Internal/Reflection/Execution/ExecutionEnvironmentImplementation.Runtime.cs @@ -128,7 +128,7 @@ public sealed override FieldAccessor CreateLiteralFieldAccessor(object value, Ru return new LiteralFieldAccessor(value, fieldTypeHandle); } - public sealed override EnumInfo GetEnumInfo(RuntimeTypeHandle typeHandle) + public sealed override void GetEnumInfo(RuntimeTypeHandle typeHandle, out string[] names, out object[] values, out bool isFlags) { // Handle the weird case of an enum type nested under a generic type that makes the // enum itself generic @@ -141,7 +141,10 @@ public sealed override EnumInfo GetEnumInfo( // If the type is reflection blocked, we pretend there are no enum values defined if (ReflectionExecution.ExecutionEnvironment.IsReflectionBlocked(typeDefHandle)) { - return new EnumInfo(RuntimeAugments.GetEnumUnderlyingType(typeHandle), Array.Empty(), Array.Empty(), false); + names = Array.Empty(); + values = Array.Empty(); + isFlags = false; + return; } QTypeDefinition qTypeDefinition; @@ -152,7 +155,13 @@ public sealed override EnumInfo GetEnumInfo( if (qTypeDefinition.IsNativeFormatMetadataBased) { - return NativeFormatEnumInfo.Create(typeHandle, qTypeDefinition.NativeFormatReader, qTypeDefinition.NativeFormatHandle); + NativeFormatEnumInfo.GetEnumValuesAndNames( + qTypeDefinition.NativeFormatReader, + qTypeDefinition.NativeFormatHandle, + out values, + out names, + out isFlags); + return; } #if ECMA_METADATA_SUPPORT if (qTypeDefinition.IsEcmaFormatMetadataBased) @@ -160,7 +169,10 @@ public sealed override EnumInfo GetEnumInfo( return EcmaFormatEnumInfo.Create(typeHandle, qTypeDefinition.EcmaFormatReader, qTypeDefinition.EcmaFormatHandle); } #endif - return null; + names = Array.Empty(); + values = Array.Empty(); + isFlags = false; + return; } public override IntPtr GetDynamicInvokeThunk(MethodInvoker invoker) diff --git a/src/coreclr/nativeaot/System.Private.Reflection.Execution/src/Internal/Reflection/Execution/NativeFormatEnumInfo.cs b/src/coreclr/nativeaot/System.Private.Reflection.Execution/src/Internal/Reflection/Execution/NativeFormatEnumInfo.cs index 015630857005e..b7378bd531c18 100644 --- a/src/coreclr/nativeaot/System.Private.Reflection.Execution/src/Internal/Reflection/Execution/NativeFormatEnumInfo.cs +++ b/src/coreclr/nativeaot/System.Private.Reflection.Execution/src/Internal/Reflection/Execution/NativeFormatEnumInfo.cs @@ -13,8 +13,8 @@ namespace Internal.Reflection.Execution { static class NativeFormatEnumInfo { - private static void GetEnumValuesAndNames(MetadataReader reader, TypeDefinitionHandle typeDefHandle, - out object[] sortedBoxedValues, out string[] sortedNames, out bool isFlags) + public static void GetEnumValuesAndNames(MetadataReader reader, TypeDefinitionHandle typeDefHandle, + out object[] unsortedBoxedValues, out string[] unsortedNames, out bool isFlags) { TypeDefinition typeDef = reader.GetTypeDefinition(typeDefHandle); @@ -30,8 +30,8 @@ private static void GetEnumValuesAndNames(MetadataReader reader, TypeDefinitionH } } - var names = new string[staticFieldCount]; - var boxedValues = new object[staticFieldCount]; // TODO: Avoid boxing the values + unsortedNames = new string[staticFieldCount]; + unsortedBoxedValues = new object[staticFieldCount]; // TODO: Avoid boxing the values int i = 0; foreach (FieldHandle fieldHandle in typeDef.Fields) @@ -39,18 +39,12 @@ private static void GetEnumValuesAndNames(MetadataReader reader, TypeDefinitionH Field field = fieldHandle.GetField(reader); if (0 != (field.Flags & FieldAttributes.Static)) { - names[i] = field.Name.GetString(reader); - boxedValues[i] = field.DefaultValue.ParseConstantNumericValue(reader); + unsortedNames[i] = field.Name.GetString(reader); + unsortedBoxedValues[i] = field.DefaultValue.ParseConstantNumericValue(reader); i++; } } - // Using object overload to avoid generic expansion for every underlying enum type - Array.Sort(boxedValues, names); - - sortedBoxedValues = boxedValues; - sortedNames = names; - isFlags = false; foreach (CustomAttributeHandle cah in typeDef.CustomAttributes) { @@ -58,17 +52,5 @@ private static void GetEnumValuesAndNames(MetadataReader reader, TypeDefinitionH isFlags = true; } } - - public static EnumInfo Create(RuntimeTypeHandle typeHandle, MetadataReader reader, TypeDefinitionHandle typeDefHandle) - where TUnderlyingValue : struct, INumber - { - GetEnumValuesAndNames(reader, typeDefHandle, out object[] boxedValues, out string[] names, out bool isFlags); - - var values = new TUnderlyingValue[boxedValues.Length]; - for (int i = 0; i < boxedValues.Length; i++) - values[i] = (TUnderlyingValue)boxedValues[i]; - - return new EnumInfo(RuntimeAugments.GetEnumUnderlyingType(typeHandle), values, names, isFlags); - } } } From 7e4a4c934828aa4ac32ebb2b1cf8b15c1dcce73f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Tue, 20 Dec 2022 17:00:27 +0900 Subject: [PATCH 2/4] Update ReflectionCoreCallbacksImplementation.cs --- .../Runtime/General/ReflectionCoreCallbacksImplementation.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/General/ReflectionCoreCallbacksImplementation.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/General/ReflectionCoreCallbacksImplementation.cs index bd3d2918a95d9..ae7494fc39a79 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/General/ReflectionCoreCallbacksImplementation.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/General/ReflectionCoreCallbacksImplementation.cs @@ -423,7 +423,7 @@ public sealed override EnumInfo GetEnumInfo( for (int i = 0; i < unsortedValues.Length; i++) values[i] = (TUnderlyingValue)unsortedValues[i]; - info = new EnumInfo(type, values, unsortedNames, isFlags); + info = new EnumInfo(RuntimeAugments.GetEnumUnderlyingType(runtimeType.TypeHandle), values, unsortedNames, isFlags); runtimeType.GenericCache = info; return info; } From 8e53547dfe555dfc1ec3df9ff91231adb5691838 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Thu, 22 Dec 2022 06:47:41 +0900 Subject: [PATCH 3/4] Apply suggestions from code review Co-authored-by: Jan Kotas --- .../General/ReflectionCoreCallbacksImplementation.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/General/ReflectionCoreCallbacksImplementation.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/General/ReflectionCoreCallbacksImplementation.cs index ae7494fc39a79..5b9f8492e0544 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/General/ReflectionCoreCallbacksImplementation.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/General/ReflectionCoreCallbacksImplementation.cs @@ -430,17 +430,17 @@ public sealed override EnumInfo GetEnumInfo( private class EnumUnderlyingTypeComparer : IComparer { - public static EnumUnderlyingTypeComparer Instance = new EnumUnderlyingTypeComparer(); + public readonly static EnumUnderlyingTypeComparer Instance = new EnumUnderlyingTypeComparer(); public int Compare(object? x, object? y) => x switch { - byte b => b.CompareTo((byte)y!), - sbyte sb => sb.CompareTo((sbyte)y!), - short s => s.CompareTo((short)y!), - ushort us => us.CompareTo((ushort)y!), int i => i.CompareTo((int)y!), uint ui => ui.CompareTo((uint)y!), + byte b => b.CompareTo((byte)y!), + ushort us => us.CompareTo((ushort)y!), + short s => s.CompareTo((short)y!), + sbyte sb => sb.CompareTo((sbyte)y!), long l => l.CompareTo((long)y!), _ => ((ulong)x!).CompareTo((ulong)y!), }; From ad77588cf3f4522896241433192b3124fd7027bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Thu, 22 Dec 2022 09:05:34 +0900 Subject: [PATCH 4/4] Update src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/General/ReflectionCoreCallbacksImplementation.cs --- .../Runtime/General/ReflectionCoreCallbacksImplementation.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/General/ReflectionCoreCallbacksImplementation.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/General/ReflectionCoreCallbacksImplementation.cs index 5b9f8492e0544..996f77b66b38b 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/General/ReflectionCoreCallbacksImplementation.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/General/ReflectionCoreCallbacksImplementation.cs @@ -430,7 +430,7 @@ public sealed override EnumInfo GetEnumInfo( private class EnumUnderlyingTypeComparer : IComparer { - public readonly static EnumUnderlyingTypeComparer Instance = new EnumUnderlyingTypeComparer(); + public static readonly EnumUnderlyingTypeComparer Instance = new EnumUnderlyingTypeComparer(); public int Compare(object? x, object? y) => x switch