From cb5a55fa85234ec21aaa52d7f1942abd895f7c05 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Fri, 30 Nov 2018 10:16:25 -0500 Subject: [PATCH] A few more Enum perf improvements (dotnet/coreclr#21284) * A few more Enum perf improvements I started out to just remove the unnecessary boxing allocation that happens when you do enum.ToString("D") or, if the value doesn't map to a named enum entry, just enum.ToString() (there's an internal boxing of the value that happens in addition to any boxing that happens of the enum itself). As part of that, I added a ValueToString method that replaces the GetValue().ToString() calls that appear several times, and in writing that ValueToString, I opted to avoid pinning, instead using Unsafe.As. Once I did that, I then removed pinning everywhere else it was being done, standardizing on Unsafe.As. * Address PR feedback Commit migrated from https://github.com/dotnet/coreclr/commit/7972722e3a3788fe96056d0a5398f90466588b8f --- .../src/System.Private.CoreLib/ILLinkTrim.xml | 2 + .../src/System.Private.CoreLib/src/System/Enum.cs | 415 +++++++++++---------- .../src/System/Reflection/Emit/TypeBuilder.cs | 2 +- .../Runtime/CompilerServices/RuntimeHelpers.cs | 2 +- .../System/Runtime/CompilerServices/jithelpers.cs | 20 +- .../WindowsRuntime/CLRIPropertyValueImpl.cs | 16 +- 6 files changed, 226 insertions(+), 231 deletions(-) diff --git a/src/coreclr/src/System.Private.CoreLib/ILLinkTrim.xml b/src/coreclr/src/System.Private.CoreLib/ILLinkTrim.xml index 5445899..50548b8 100644 --- a/src/coreclr/src/System.Private.CoreLib/ILLinkTrim.xml +++ b/src/coreclr/src/System.Private.CoreLib/ILLinkTrim.xml @@ -39,5 +39,7 @@ + + diff --git a/src/coreclr/src/System.Private.CoreLib/src/System/Enum.cs b/src/coreclr/src/System.Private.CoreLib/src/System/Enum.cs index 9b28aca..81a6a3c 100644 --- a/src/coreclr/src/System.Private.CoreLib/src/System/Enum.cs +++ b/src/coreclr/src/System.Private.CoreLib/src/System/Enum.cs @@ -55,39 +55,73 @@ namespace System return entry; } - private unsafe string InternalFormattedHexString() + private string ValueToString() { - fixed (void* pValue = &JitHelpers.GetPinningHelper(this).m_data) + ref byte data = ref this.GetRawData(); + switch (InternalGetCorElementType()) { - switch (InternalGetCorElementType()) - { - case CorElementType.I1: - case CorElementType.U1: - return (*(byte*)pValue).ToString("X2", null); - case CorElementType.Boolean: - // direct cast from bool to byte is not allowed - return Convert.ToByte(*(bool*)pValue).ToString("X2", null); - case CorElementType.I2: - case CorElementType.U2: - case CorElementType.Char: - return (*(ushort*)pValue).ToString("X4", null); - case CorElementType.I4: - case CorElementType.U4: - return (*(uint*)pValue).ToString("X8", null); - case CorElementType.I8: - case CorElementType.U8: - return (*(ulong*)pValue).ToString("X16", null); - default: - throw new InvalidOperationException(SR.InvalidOperation_UnknownEnumType); - } + case CorElementType.I1: + return Unsafe.As(ref data).ToString(); + case CorElementType.U1: + return data.ToString(); + case CorElementType.Boolean: + return Unsafe.As(ref data).ToString(); + case CorElementType.I2: + return Unsafe.As(ref data).ToString(); + case CorElementType.U2: + return Unsafe.As(ref data).ToString(); + case CorElementType.Char: + return Unsafe.As(ref data).ToString(); + case CorElementType.I4: + return Unsafe.As(ref data).ToString(); + case CorElementType.U4: + return Unsafe.As(ref data).ToString(); + case CorElementType.R4: + return Unsafe.As(ref data).ToString(); + case CorElementType.I8: + return Unsafe.As(ref data).ToString(); + case CorElementType.U8: + return Unsafe.As(ref data).ToString(); + case CorElementType.R8: + return Unsafe.As(ref data).ToString(); + case CorElementType.I: + return Unsafe.As(ref data).ToString(); + case CorElementType.U: + return Unsafe.As(ref data).ToString(); + default: + Debug.Fail("Invalid primitive type"); + return null; } } - private static string InternalFormattedHexString(object value) + private string ValueToHexString() { - TypeCode typeCode = Convert.GetTypeCode(value); + ref byte data = ref this.GetRawData(); + switch (InternalGetCorElementType()) + { + case CorElementType.I1: + case CorElementType.U1: + return data.ToString("X2", null); + case CorElementType.Boolean: + return Convert.ToByte(Unsafe.As(ref data)).ToString("X2", null); + case CorElementType.I2: + case CorElementType.U2: + case CorElementType.Char: + return Unsafe.As(ref data).ToString("X4", null); + case CorElementType.I4: + case CorElementType.U4: + return Unsafe.As(ref data).ToString("X8", null); + case CorElementType.I8: + case CorElementType.U8: + return Unsafe.As(ref data).ToString("X16", null); + default: + throw new InvalidOperationException(SR.InvalidOperation_UnknownEnumType); + } + } - switch (typeCode) + private static string ValueToHexString(object value) + { + switch (Convert.GetTypeCode(value)) { case TypeCode.SByte: return ((byte)(sbyte)value).ToString("X2", null); @@ -346,18 +380,7 @@ namespace System { // Validation on the enum type itself. Failures here are considered non-parsing failures // and thus always throw rather than returning false. - if (enumType == null) - { - throw new ArgumentNullException(nameof(enumType)); - } - if (!(enumType is RuntimeType rt)) - { - throw new ArgumentException(SR.Arg_MustBeType, nameof(enumType)); - } - if (!rt.IsEnum) - { - throw new ArgumentException(SR.Arg_MustBeEnum, nameof(enumType)); - } + RuntimeType rt = ValidateRuntimeType(enumType); ReadOnlySpan valueSpan = value.AsSpan().TrimStart(); if (valueSpan.Length == 0) @@ -872,28 +895,16 @@ namespace System public static string Format(Type enumType, object value, string format) { - if (enumType == null) - throw new ArgumentNullException(nameof(enumType)); - - if (!enumType.IsEnum) - throw new ArgumentException(SR.Arg_MustBeEnum, nameof(enumType)); + RuntimeType rtType = ValidateRuntimeType(enumType); if (value == null) throw new ArgumentNullException(nameof(value)); if (format == null) throw new ArgumentNullException(nameof(format)); - - RuntimeType rtType = enumType as RuntimeType; - if (rtType == null) - throw new ArgumentException(SR.Arg_MustBeType, nameof(enumType)); - - // Check if both of them are of the same type - Type valueType = value.GetType(); - - Type underlyingType = GetUnderlyingType(enumType); - + // If the value is an Enum then we need to extract the underlying value from it + Type valueType = value.GetType(); if (valueType.IsEnum) { if (!valueType.IsEquivalentTo(enumType)) @@ -906,33 +917,38 @@ namespace System } return ((Enum)value).ToString(format); } + // The value must be of the same type as the Underlying type of the Enum - else if (valueType != underlyingType) + Type underlyingType = GetUnderlyingType(enumType); + if (valueType != underlyingType) { throw new ArgumentException(SR.Format(SR.Arg_EnumFormatUnderlyingTypeAndObjectMustBeSameType, valueType.ToString(), underlyingType.ToString())); } - if (format.Length != 1) - { - // all acceptable format string are of length 1 - throw new FormatException(SR.Format_InvalidEnumFormatSpecification); - } - char formatCh = format[0]; - if (formatCh == 'G' || formatCh == 'g') - return GetEnumName(rtType, ToUInt64(value)) ?? value.ToString(); + if (format.Length == 1) + { + switch (format[0]) + { + case 'G': + case 'g': + return GetEnumName(rtType, ToUInt64(value)) ?? value.ToString(); - if (formatCh == 'D' || formatCh == 'd') - return value.ToString(); + case 'D': + case 'd': + return value.ToString(); - if (formatCh == 'X' || formatCh == 'x') - return InternalFormattedHexString(value); + case 'X': + case 'x': + return ValueToHexString(value); - if (formatCh == 'F' || formatCh == 'f') - return Enum.InternalFlagsFormat(rtType, ToUInt64(value)) ?? value.ToString(); + case 'F': + case 'f': + return InternalFlagsFormat(rtType, ToUInt64(value)) ?? value.ToString(); + } + } throw new FormatException(SR.Format_InvalidEnumFormatSpecification); } - #endregion #region Definitions @@ -953,96 +969,78 @@ namespace System #endregion #region Private Methods - internal unsafe object GetValue() + internal object GetValue() { - fixed (void* pValue = &JitHelpers.GetPinningHelper(this).m_data) + ref byte data = ref this.GetRawData(); + switch (InternalGetCorElementType()) { - switch (InternalGetCorElementType()) - { - case CorElementType.I1: - return *(sbyte*)pValue; - case CorElementType.U1: - return *(byte*)pValue; - case CorElementType.Boolean: - return *(bool*)pValue; - case CorElementType.I2: - return *(short*)pValue; - case CorElementType.U2: - return *(ushort*)pValue; - case CorElementType.Char: - return *(char*)pValue; - case CorElementType.I4: - return *(int*)pValue; - case CorElementType.U4: - return *(uint*)pValue; - case CorElementType.R4: - return *(float*)pValue; - case CorElementType.I8: - return *(long*)pValue; - case CorElementType.U8: - return *(ulong*)pValue; - case CorElementType.R8: - return *(double*)pValue; - case CorElementType.I: - return *(IntPtr*)pValue; - case CorElementType.U: - return *(UIntPtr*)pValue; - default: - Debug.Fail("Invalid primitive type"); - return null; - } + case CorElementType.I1: + return Unsafe.As(ref data); + case CorElementType.U1: + return data; + case CorElementType.Boolean: + return Unsafe.As(ref data); + case CorElementType.I2: + return Unsafe.As(ref data); + case CorElementType.U2: + return Unsafe.As(ref data); + case CorElementType.Char: + return Unsafe.As(ref data); + case CorElementType.I4: + return Unsafe.As(ref data); + case CorElementType.U4: + return Unsafe.As(ref data); + case CorElementType.R4: + return Unsafe.As(ref data); + case CorElementType.I8: + return Unsafe.As(ref data); + case CorElementType.U8: + return Unsafe.As(ref data); + case CorElementType.R8: + return Unsafe.As(ref data); + case CorElementType.I: + return Unsafe.As(ref data); + case CorElementType.U: + return Unsafe.As(ref data); + default: + Debug.Fail("Invalid primitive type"); + return null; } } - private unsafe ulong ToUInt64() + private ulong ToUInt64() { - fixed (void* pValue = &JitHelpers.GetPinningHelper(this).m_data) + ref byte data = ref this.GetRawData(); + switch (InternalGetCorElementType()) { - switch (InternalGetCorElementType()) - { - case CorElementType.I1: - return (ulong)*(sbyte*)pValue; - case CorElementType.U1: - return *(byte*)pValue; - case CorElementType.Boolean: - return Convert.ToUInt64(*(bool*)pValue, CultureInfo.InvariantCulture); - case CorElementType.I2: - return (ulong)*(short*)pValue; - case CorElementType.U2: - case CorElementType.Char: - return *(ushort*)pValue; - case CorElementType.I4: - return (ulong)*(int*)pValue; - case CorElementType.U4: - case CorElementType.R4: - return *(uint*)pValue; - case CorElementType.I8: - return (ulong)*(long*)pValue; - case CorElementType.U8: - case CorElementType.R8: - return *(ulong*)pValue; - case CorElementType.I: - if (IntPtr.Size == 8) - { - return *(ulong*)pValue; - } - else - { - return (ulong)*(int*)pValue; - } - case CorElementType.U: - if (IntPtr.Size == 8) - { - return *(ulong*)pValue; - } - else - { - return *(uint*)pValue; - } - default: - Debug.Fail("Invalid primitive type"); - return 0; - } + case CorElementType.I1: + return (ulong)Unsafe.As(ref data); + case CorElementType.U1: + return data; + case CorElementType.Boolean: + return Convert.ToUInt64(Unsafe.As(ref data), CultureInfo.InvariantCulture); + case CorElementType.I2: + return (ulong)Unsafe.As(ref data); + case CorElementType.U2: + case CorElementType.Char: + return Unsafe.As(ref data); + case CorElementType.I4: + return (ulong)Unsafe.As(ref data); + case CorElementType.U4: + case CorElementType.R4: + return Unsafe.As(ref data); + case CorElementType.I8: + return (ulong)Unsafe.As(ref data); + case CorElementType.U8: + case CorElementType.R8: + return Unsafe.As(ref data); + case CorElementType.I: + return (ulong)Unsafe.As(ref data); + case CorElementType.U: + return (ulong)Unsafe.As(ref data); + default: + Debug.Fail("Invalid primitive type"); + return 0; } } @@ -1058,48 +1056,45 @@ namespace System [MethodImplAttribute(MethodImplOptions.InternalCall)] public extern override bool Equals(object obj); - public override unsafe int GetHashCode() + public override int GetHashCode() { // CONTRACT with the runtime: GetHashCode of enum types is implemented as GetHashCode of the underlying type. // The runtime can bypass calls to Enum::GetHashCode and call the underlying type's GetHashCode directly // to avoid boxing the enum. - - fixed (void* pValue = &JitHelpers.GetPinningHelper(this).m_data) + ref byte data = ref this.GetRawData(); + switch (InternalGetCorElementType()) { - switch (InternalGetCorElementType()) - { - case CorElementType.I1: - return (*(sbyte*)pValue).GetHashCode(); - case CorElementType.U1: - return (*(byte*)pValue).GetHashCode(); - case CorElementType.Boolean: - return (*(bool*)pValue).GetHashCode(); - case CorElementType.I2: - return (*(short*)pValue).GetHashCode(); - case CorElementType.U2: - return (*(ushort*)pValue).GetHashCode(); - case CorElementType.Char: - return (*(char*)pValue).GetHashCode(); - case CorElementType.I4: - return (*(int*)pValue).GetHashCode(); - case CorElementType.U4: - return (*(uint*)pValue).GetHashCode(); - case CorElementType.R4: - return (*(float*)pValue).GetHashCode(); - case CorElementType.I8: - return (*(long*)pValue).GetHashCode(); - case CorElementType.U8: - return (*(ulong*)pValue).GetHashCode(); - case CorElementType.R8: - return (*(double*)pValue).GetHashCode(); - case CorElementType.I: - return (*(IntPtr*)pValue).GetHashCode(); - case CorElementType.U: - return (*(UIntPtr*)pValue).GetHashCode(); - default: - Debug.Fail("Invalid primitive type"); - return 0; - } + case CorElementType.I1: + return Unsafe.As(ref data).GetHashCode(); + case CorElementType.U1: + return data.GetHashCode(); + case CorElementType.Boolean: + return Unsafe.As(ref data).GetHashCode(); + case CorElementType.I2: + return Unsafe.As(ref data).GetHashCode(); + case CorElementType.U2: + return Unsafe.As(ref data).GetHashCode(); + case CorElementType.Char: + return Unsafe.As(ref data).GetHashCode(); + case CorElementType.I4: + return Unsafe.As(ref data).GetHashCode(); + case CorElementType.U4: + return Unsafe.As(ref data).GetHashCode(); + case CorElementType.R4: + return Unsafe.As(ref data).GetHashCode(); + case CorElementType.I8: + return Unsafe.As(ref data).GetHashCode(); + case CorElementType.U8: + return Unsafe.As(ref data).GetHashCode(); + case CorElementType.R8: + return Unsafe.As(ref data).GetHashCode(); + case CorElementType.I: + return Unsafe.As(ref data).GetHashCode(); + case CorElementType.U: + return Unsafe.As(ref data).GetHashCode(); + default: + Debug.Fail("Invalid primitive type"); + return 0; } } @@ -1108,12 +1103,11 @@ namespace System // Returns the value in a human readable format. For PASCAL style enums who's value maps directly the name of the field is returned. // For PASCAL style enums who's values do not map directly the decimal value of the field is returned. // For BitFlags (indicated by the Flags custom attribute): If for each bit that is set in the value there is a corresponding constant - //(a pure power of 2), then the OR string (ie "Red | Yellow") is returned. Otherwise, if the value is zero or if you can't create a string that consists of + // (a pure power of 2), then the OR string (ie "Red, Yellow") is returned. Otherwise, if the value is zero or if you can't create a string that consists of // pure powers of 2 OR-ed together, you return a hex value - // Try to see if its one of the enum values, then we return a String back else the value - return Enum.InternalFormat((RuntimeType)GetType(), ToUInt64()) ?? GetValue().ToString(); + return InternalFormat((RuntimeType)GetType(), ToUInt64()) ?? ValueToString(); } #endregion @@ -1161,25 +1155,32 @@ namespace System #region Public Methods public string ToString(string format) { - char formatCh; - if (format == null || format.Length == 0) - formatCh = 'G'; - else if (format.Length != 1) - throw new FormatException(SR.Format_InvalidEnumFormatSpecification); - else - formatCh = format[0]; - - if (formatCh == 'G' || formatCh == 'g') + if (string.IsNullOrEmpty(format)) + { return ToString(); + } - if (formatCh == 'D' || formatCh == 'd') - return GetValue().ToString(); + if (format.Length == 1) + { + switch (format[0]) + { + case 'G': + case 'g': + return ToString(); - if (formatCh == 'X' || formatCh == 'x') - return InternalFormattedHexString(); + case 'D': + case 'd': + return ValueToString(); - if (formatCh == 'F' || formatCh == 'f') - return InternalFlagsFormat((RuntimeType)GetType(), ToUInt64()) ?? GetValue().ToString(); + case 'X': + case 'x': + return ValueToHexString(); + + case 'F': + case 'f': + return InternalFlagsFormat((RuntimeType)GetType(), ToUInt64()) ?? ValueToString(); + } + } throw new FormatException(SR.Format_InvalidEnumFormatSpecification); } diff --git a/src/coreclr/src/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.cs b/src/coreclr/src/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.cs index 16a64d9..5b62e81 100644 --- a/src/coreclr/src/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.cs +++ b/src/coreclr/src/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.cs @@ -366,7 +366,7 @@ namespace System.Reflection.Emit case CorElementType.I8: case CorElementType.U8: case CorElementType.R8: - fixed (byte* pData = &JitHelpers.GetPinningHelper(value).m_data) + fixed (byte* pData = &value.GetRawData()) SetConstantValue(module.GetNativeHandle(), tk, (int)corType, pData); break; diff --git a/src/coreclr/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.cs b/src/coreclr/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.cs index 93490d9..fe101e2 100644 --- a/src/coreclr/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.cs +++ b/src/coreclr/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.cs @@ -227,7 +227,7 @@ namespace System.Runtime.CompilerServices // method table pointer, so just back up one pointer and immediately deref. // This is not ideal in terms of minimizing instruction count but is the best we can do at the moment. - return Unsafe.Add(ref Unsafe.As(ref JitHelpers.GetPinningHelper(obj).m_data), -1); + return Unsafe.Add(ref Unsafe.As(ref obj.GetRawData()), -1); // The JIT currently implements this as: // lea tmp, [rax + 8h] ; assume rax contains the object reference, tmp is type IntPtr& diff --git a/src/coreclr/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/jithelpers.cs b/src/coreclr/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/jithelpers.cs index dd491dc..1c00970 100644 --- a/src/coreclr/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/jithelpers.cs +++ b/src/coreclr/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/jithelpers.cs @@ -7,13 +7,8 @@ // Low-level Jit Helpers //////////////////////////////////////////////////////////////////////////////// -using System; using System.Threading; -using System.Runtime; -using System.Runtime.Versioning; using System.Diagnostics; -using System.Runtime.InteropServices; -using System.Security; using Internal.Runtime.CompilerServices; namespace System.Runtime.CompilerServices @@ -51,11 +46,8 @@ namespace System.Runtime.CompilerServices } } - // Helper class to assist with unsafe pinning of arbitrary objects. The typical usage pattern is: - // fixed (byte * pData = &JitHelpers.GetPinningHelper(value).m_data) - // { - // ... pData is what Object::GetData() returns in VM ... - // } + // Helper class to assist with unsafe pinning of arbitrary objects. + // It's used by VM code. internal class PinningHelper { public byte m_data; @@ -173,10 +165,12 @@ namespace System.Runtime.CompilerServices [MethodImplAttribute(MethodImplOptions.InternalCall)] internal static extern void UnsafeSetArrayElement(object[] target, int index, object element); - // Used for unsafe pinning of arbitrary objects. - internal static PinningHelper GetPinningHelper(object o) + internal static ref byte GetRawData(this object obj) => + ref Unsafe.As(obj).Data; + + private sealed class RawData { - return Unsafe.As(o); + public byte Data; } #if DEBUG diff --git a/src/coreclr/src/System.Private.CoreLib/src/System/Runtime/InteropServices/WindowsRuntime/CLRIPropertyValueImpl.cs b/src/coreclr/src/System.Private.CoreLib/src/System/Runtime/InteropServices/WindowsRuntime/CLRIPropertyValueImpl.cs index 17bb6f4..57de152 100644 --- a/src/coreclr/src/System.Private.CoreLib/src/System/Runtime/InteropServices/WindowsRuntime/CLRIPropertyValueImpl.cs +++ b/src/coreclr/src/System.Private.CoreLib/src/System/Runtime/InteropServices/WindowsRuntime/CLRIPropertyValueImpl.cs @@ -477,7 +477,7 @@ namespace System.Runtime.InteropServices.WindowsRuntime return false; } - // Unbox the data stored in the property value to a structurally equivilent type + // Unbox the data stored in the property value to a structurally equivalent type private unsafe T Unbox(Type expectedBoxedType) where T : struct { Debug.Assert(expectedBoxedType != null); @@ -490,7 +490,7 @@ namespace System.Runtime.InteropServices.WindowsRuntime T unboxed = new T(); - fixed (byte* pData = &JitHelpers.GetPinningHelper(_data).m_data) + fixed (byte* pData = &_data.GetRawData()) { byte* pUnboxed = (byte*)JitHelpers.UnsafeCastToStackPointer(ref unboxed); Buffer.Memcpy(pUnboxed, pData, Marshal.SizeOf(unboxed)); @@ -515,15 +515,13 @@ namespace System.Runtime.InteropServices.WindowsRuntime if (converted.Length > 0) { - fixed (byte* dataPin = &JitHelpers.GetPinningHelper(dataArray).m_data) + fixed (byte* dataPin = &dataArray.GetRawData()) + fixed (byte* convertedPin = &converted.GetRawData()) { - fixed (byte* convertedPin = &JitHelpers.GetPinningHelper(converted).m_data) - { - byte* pData = (byte*)Marshal.UnsafeAddrOfPinnedArrayElement(dataArray, 0); - byte* pConverted = (byte*)Marshal.UnsafeAddrOfPinnedArrayElement(converted, 0); + byte* pData = (byte*)Marshal.UnsafeAddrOfPinnedArrayElement(dataArray, 0); + byte* pConverted = (byte*)Marshal.UnsafeAddrOfPinnedArrayElement(converted, 0); - Buffer.Memcpy(pConverted, pData, checked(Marshal.SizeOf(typeof(T)) * converted.Length)); - } + Buffer.Memcpy(pConverted, pData, checked(Marshal.SizeOf(typeof(T)) * converted.Length)); } } -- 2.7.4