From 92408358a76eb9b908b9e9ce317f2386617f64d1 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Thu, 5 Oct 2017 17:23:41 -0400 Subject: [PATCH] Address perf issues Workaround regressions from the span switch and also fix a few other issues that popped while profiling. - Specialize Compare{String} methods to take one span and one string - Ensure frequently called methods get inlined - Avoid unnecessary writes to a ref - Avoid unnecessary Math.Pow calls --- .../System/Globalization/DateTimeFormatInfo.cs | 115 +++++++++------------ .../shared/System/Globalization/DateTimeParse.cs | 64 ++++-------- .../src/System/Globalization/CompareInfo.Unix.cs | 17 +++ .../System/Globalization/CompareInfo.Windows.cs | 36 +++++++ .../src/System/Globalization/CompareInfo.cs | 43 +++++++- 5 files changed, 165 insertions(+), 110 deletions(-) diff --git a/src/mscorlib/shared/System/Globalization/DateTimeFormatInfo.cs b/src/mscorlib/shared/System/Globalization/DateTimeFormatInfo.cs index 4a8059f..de26079 100644 --- a/src/mscorlib/shared/System/Globalization/DateTimeFormatInfo.cs +++ b/src/mscorlib/shared/System/Globalization/DateTimeFormatInfo.cs @@ -4,6 +4,7 @@ using System.Collections.Generic; using System.Diagnostics; +using System.Runtime.CompilerServices; using System.Runtime.Serialization; namespace System.Globalization @@ -214,18 +215,16 @@ namespace System.Globalization // //////////////////////////////////////////////////////////////////////////// - private String[] internalGetAbbreviatedDayOfWeekNames() + private string[] internalGetAbbreviatedDayOfWeekNames() => this.abbreviatedDayNames ?? internalGetAbbreviatedDayOfWeekNamesCore(); + [MethodImpl(MethodImplOptions.NoInlining)] + private string[] internalGetAbbreviatedDayOfWeekNamesCore() { - if (this.abbreviatedDayNames == null) - { - // Get the abbreviated day names for our current calendar - this.abbreviatedDayNames = _cultureData.AbbreviatedDayNames(Calendar.ID); - Debug.Assert(this.abbreviatedDayNames.Length == 7, "[DateTimeFormatInfo.GetAbbreviatedDayOfWeekNames] Expected 7 day names in a week"); - } - return (this.abbreviatedDayNames); + // Get the abbreviated day names for our current calendar + this.abbreviatedDayNames = _cultureData.AbbreviatedDayNames(Calendar.ID); + Debug.Assert(this.abbreviatedDayNames.Length == 7, "[DateTimeFormatInfo.GetAbbreviatedDayOfWeekNames] Expected 7 day names in a week"); + return this.abbreviatedDayNames; } - //////////////////////////////////////////////////////////////////////// // // Action: Returns the string array of the one-letter day of week names. @@ -238,15 +237,14 @@ namespace System.Globalization // //////////////////////////////////////////////////////////////////////// - private String[] internalGetSuperShortDayNames() + private string[] internalGetSuperShortDayNames() => this.m_superShortDayNames ?? internalGetSuperShortDayNamesCore(); + [MethodImpl(MethodImplOptions.NoInlining)] + private string[] internalGetSuperShortDayNamesCore() { - if (this.m_superShortDayNames == null) - { - // Get the super short day names for our current calendar - this.m_superShortDayNames = _cultureData.SuperShortDayNames(Calendar.ID); - Debug.Assert(this.m_superShortDayNames.Length == 7, "[DateTimeFormatInfo.internalGetSuperShortDayNames] Expected 7 day names in a week"); - } - return (this.m_superShortDayNames); + // Get the super short day names for our current calendar + this.m_superShortDayNames = _cultureData.SuperShortDayNames(Calendar.ID); + Debug.Assert(this.m_superShortDayNames.Length == 7, "[DateTimeFormatInfo.internalGetSuperShortDayNames] Expected 7 day names in a week"); + return this.m_superShortDayNames; } //////////////////////////////////////////////////////////////////////////// @@ -255,15 +253,14 @@ namespace System.Globalization // //////////////////////////////////////////////////////////////////////////// - private String[] internalGetDayOfWeekNames() + private string[] internalGetDayOfWeekNames() => this.dayNames ?? internalGetDayOfWeekNamesCore(); + [MethodImpl(MethodImplOptions.NoInlining)] + private string[] internalGetDayOfWeekNamesCore() { - if (this.dayNames == null) - { - // Get the day names for our current calendar - this.dayNames = _cultureData.DayNames(Calendar.ID); - Debug.Assert(this.dayNames.Length == 7, "[DateTimeFormatInfo.GetDayOfWeekNames] Expected 7 day names in a week"); - } - return (this.dayNames); + // Get the day names for our current calendar + this.dayNames = _cultureData.DayNames(Calendar.ID); + Debug.Assert(this.dayNames.Length == 7, "[DateTimeFormatInfo.GetDayOfWeekNames] Expected 7 day names in a week"); + return this.dayNames; } //////////////////////////////////////////////////////////////////////////// @@ -272,16 +269,15 @@ namespace System.Globalization // //////////////////////////////////////////////////////////////////////////// - private String[] internalGetAbbreviatedMonthNames() + private String[] internalGetAbbreviatedMonthNames() => this.abbreviatedMonthNames ?? internalGetAbbreviatedMonthNamesCore(); + [MethodImpl(MethodImplOptions.NoInlining)] + private String[] internalGetAbbreviatedMonthNamesCore() { - if (this.abbreviatedMonthNames == null) - { - // Get the month names for our current calendar - this.abbreviatedMonthNames = _cultureData.AbbreviatedMonthNames(Calendar.ID); - Debug.Assert(this.abbreviatedMonthNames.Length == 12 || this.abbreviatedMonthNames.Length == 13, - "[DateTimeFormatInfo.GetAbbreviatedMonthNames] Expected 12 or 13 month names in a year"); - } - return (this.abbreviatedMonthNames); + // Get the month names for our current calendar + this.abbreviatedMonthNames = _cultureData.AbbreviatedMonthNames(Calendar.ID); + Debug.Assert(this.abbreviatedMonthNames.Length == 12 || this.abbreviatedMonthNames.Length == 13, + "[DateTimeFormatInfo.GetAbbreviatedMonthNames] Expected 12 or 13 month names in a year"); + return this.abbreviatedMonthNames; } @@ -291,17 +287,15 @@ namespace System.Globalization // //////////////////////////////////////////////////////////////////////////// - private String[] internalGetMonthNames() + private string[] internalGetMonthNames() => this.monthNames ?? internalGetMonthNamesCore(); + [MethodImpl(MethodImplOptions.NoInlining)] + private string[] internalGetMonthNamesCore() { - if (this.monthNames == null) - { - // Get the month names for our current calendar - this.monthNames = _cultureData.MonthNames(Calendar.ID); - Debug.Assert(this.monthNames.Length == 12 || this.monthNames.Length == 13, - "[DateTimeFormatInfo.GetMonthNames] Expected 12 or 13 month names in a year"); - } - - return (this.monthNames); + // Get the month names for our current calendar + this.monthNames = _cultureData.MonthNames(Calendar.ID); + Debug.Assert(this.monthNames.Length == 12 || this.monthNames.Length == 13, + "[DateTimeFormatInfo.GetMonthNames] Expected 12 or 13 month names in a year"); + return this.monthNames; } @@ -1732,8 +1726,6 @@ namespace System.Globalization return (internalGetDayOfWeekNames()[(int)dayofweek]); } - - public String GetAbbreviatedMonthName(int month) { if (month < 1 || month > 13) @@ -1746,7 +1738,6 @@ namespace System.Globalization return (internalGetAbbreviatedMonthNames()[month - 1]); } - public String GetMonthName(int month) { if (month < 1 || month > 13) @@ -2207,23 +2198,19 @@ namespace System.Globalization // Actions: Return the internal flag used in formatting and parsing. // The flag can be used to indicate things like if genitive forms is used in this DTFi, or if leap year gets different month names. // - internal DateTimeFormatFlags FormatFlags + internal DateTimeFormatFlags FormatFlags => formatFlags != DateTimeFormatFlags.NotInitialized ? formatFlags : InitializeFormatFlags(); + [MethodImpl(MethodImplOptions.NoInlining)] + private DateTimeFormatFlags InitializeFormatFlags() { - get - { - if (formatFlags == DateTimeFormatFlags.NotInitialized) - { - // Build the format flags from the data in this DTFI - formatFlags = DateTimeFormatFlags.None; - formatFlags |= (DateTimeFormatFlags)DateTimeFormatInfoScanner.GetFormatFlagGenitiveMonth( - MonthNames, internalGetGenitiveMonthNames(false), AbbreviatedMonthNames, internalGetGenitiveMonthNames(true)); - formatFlags |= (DateTimeFormatFlags)DateTimeFormatInfoScanner.GetFormatFlagUseSpaceInMonthNames( - MonthNames, internalGetGenitiveMonthNames(false), AbbreviatedMonthNames, internalGetGenitiveMonthNames(true)); - formatFlags |= (DateTimeFormatFlags)DateTimeFormatInfoScanner.GetFormatFlagUseSpaceInDayNames(DayNames, AbbreviatedDayNames); - formatFlags |= (DateTimeFormatFlags)DateTimeFormatInfoScanner.GetFormatFlagUseHebrewCalendar((int)Calendar.ID); - } - return (formatFlags); - } + // Build the format flags from the data in this DTFI + formatFlags = + (DateTimeFormatFlags)DateTimeFormatInfoScanner.GetFormatFlagGenitiveMonth( + MonthNames, internalGetGenitiveMonthNames(false), AbbreviatedMonthNames, internalGetGenitiveMonthNames(true)) | + (DateTimeFormatFlags)DateTimeFormatInfoScanner.GetFormatFlagUseSpaceInMonthNames( + MonthNames, internalGetGenitiveMonthNames(false), AbbreviatedMonthNames, internalGetGenitiveMonthNames(true)) | + (DateTimeFormatFlags)DateTimeFormatInfoScanner.GetFormatFlagUseSpaceInDayNames(DayNames, AbbreviatedDayNames) | + (DateTimeFormatFlags)DateTimeFormatInfoScanner.GetFormatFlagUseHebrewCalendar((int)Calendar.ID); + return formatFlags; } internal Boolean HasForceTwoDigitYears @@ -2816,7 +2803,7 @@ namespace System.Globalization if (compareStrings && ((value.tokenString.Length == 1 && str.Value[str.Index] == value.tokenString[0]) || - Culture.CompareInfo.Compare(str.Value.Slice(str.Index, value.tokenString.Length), value.tokenString.AsReadOnlySpan(), CompareOptions.IgnoreCase) == 0)) + Culture.CompareInfo.Compare(str.Value.Slice(str.Index, value.tokenString.Length), value.tokenString, CompareOptions.IgnoreCase) == 0)) { tokenType = value.tokenType & TokenMask; tokenValue = value.tokenValue; diff --git a/src/mscorlib/shared/System/Globalization/DateTimeParse.cs b/src/mscorlib/shared/System/Globalization/DateTimeParse.cs index 0a1cf9e..2b0f41a 100644 --- a/src/mscorlib/shared/System/Globalization/DateTimeParse.cs +++ b/src/mscorlib/shared/System/Globalization/DateTimeParse.cs @@ -413,7 +413,7 @@ new DS[] { DS.ERROR, DS.TX_NNN, DS.TX_NNN, DS.TX_NNN, DS.ERROR, DS.ERROR, return false; } - if (str.CompareInfo.Compare(str.Value.Slice(str.Index, target.Length), target.AsReadOnlySpan(), CompareOptions.IgnoreCase) != 0) + if (str.CompareInfo.Compare(str.Value.Slice(str.Index, target.Length), target, CompareOptions.IgnoreCase) != 0) { return (false); } @@ -456,11 +456,7 @@ new DS[] { DS.ERROR, DS.TX_NNN, DS.TX_NNN, DS.TX_NNN, DS.ERROR, DS.ERROR, return (false); } - internal static bool IsDigit(char ch) - { - return (ch >= '0' && ch <= '9'); - } - + internal static bool IsDigit(char ch) => (uint)(ch - '0') <= 9; /*=================================ParseFraction========================== **Action: Starting at the str.Index, which should be a decimal symbol. @@ -3150,7 +3146,7 @@ new DS[] { DS.ERROR, DS.TX_NNN, DS.TX_NNN, DS.TX_NNN, DS.ERROR, DS.ERROR, Debug.Assert(minDigitLen > 0, "minDigitLen > 0"); Debug.Assert(maxDigitLen < 9, "maxDigitLen < 9"); Debug.Assert(minDigitLen <= maxDigitLen, "minDigitLen <= maxDigitLen"); - result = 0; + int localResult = 0; int startingIndex = str.Index; int tokenLength = 0; while (tokenLength < maxDigitLen) @@ -3160,9 +3156,10 @@ new DS[] { DS.ERROR, DS.TX_NNN, DS.TX_NNN, DS.TX_NNN, DS.ERROR, DS.ERROR, str.Index--; break; } - result = result * 10 + str.GetDigit(); + localResult = localResult * 10 + str.GetDigit(); tokenLength++; } + result = localResult; if (tokenLength < minDigitLen) { str.Index = startingIndex; @@ -3201,7 +3198,7 @@ new DS[] { DS.ERROR, DS.TX_NNN, DS.TX_NNN, DS.TX_NNN, DS.ERROR, DS.ERROR, result = result * 10 + str.GetDigit(); } - result = ((double)result / Math.Pow(10, digitLen)); + result /= TimeSpanParse.Pow10(digitLen); return (digitLen == maxDigitLen); } @@ -4801,8 +4798,7 @@ new DS[] { DS.ERROR, DS.TX_NNN, DS.TX_NNN, DS.TX_NNN, DS.ERROR, DS.ERROR, // It has a Index property which tracks // the current parsing pointer of the string. // - [IsByRefLike] - internal struct __DTString + internal ref struct __DTString { // // Value propery: stores the real string to be parsed. @@ -5016,36 +5012,19 @@ new DS[] { DS.ERROR, DS.TX_NNN, DS.TX_NNN, DS.TX_NNN, DS.ERROR, DS.ERROR, return (tokenType); } - internal bool MatchSpecifiedWord(String target) - { - return MatchSpecifiedWord(target, target.Length + Index); - } + [MethodImpl(MethodImplOptions.AggressiveInlining)] + internal bool MatchSpecifiedWord(String target) => + Index + target.Length <= Length && + m_info.Compare(Value.Slice(Index, target.Length), target, CompareOptions.IgnoreCase) == 0; - internal bool MatchSpecifiedWord(String target, int endIndex) - { - int count = endIndex - Index; - - if (count != target.Length) - { - return false; - } - - if (Index + count > Length) - { - return false; - } - - return (m_info.Compare(Value.Slice(Index, count), target.AsReadOnlySpan().Slice(0, count), CompareOptions.IgnoreCase) == 0); - } - - private static Char[] WhiteSpaceChecks = new Char[] { ' ', '\u00A0' }; + private static readonly Char[] WhiteSpaceChecks = new Char[] { ' ', '\u00A0' }; internal bool MatchSpecifiedWords(String target, bool checkWordBoundary, ref int matchLength) { int valueRemaining = Value.Length - Index; matchLength = target.Length; - if (matchLength > valueRemaining || m_info.Compare(Value.Slice(Index, matchLength), target.AsReadOnlySpan(), CompareOptions.IgnoreCase) != 0) + if (matchLength > valueRemaining || m_info.Compare(Value.Slice(Index, matchLength), target, CompareOptions.IgnoreCase) != 0) { // Check word by word int targetPosition = 0; // Where we are in the target string @@ -5140,7 +5119,7 @@ new DS[] { DS.ERROR, DS.TX_NNN, DS.TX_NNN, DS.TX_NNN, DS.ERROR, DS.ERROR, return false; } - if (m_info.Compare(Value.Slice(Index, str.Length), str.AsReadOnlySpan(), CompareOptions.Ordinal) == 0) + if (m_info.Compare(Value.Slice(Index, str.Length), str, CompareOptions.Ordinal) == 0) { // Update the Index to the end of the matching string. // So the following GetNext()/Match() opeartion will get @@ -5219,14 +5198,10 @@ new DS[] { DS.ERROR, DS.TX_NNN, DS.TX_NNN, DS.TX_NNN, DS.ERROR, DS.ERROR, } // Return false when end of string is encountered or a non-digit character is found. - internal bool GetNextDigit() - { - if (++Index >= Length) - { - return (false); - } - return (DateTimeParse.IsDigit(Value[Index])); - } + [MethodImpl(MethodImplOptions.AggressiveInlining)] + internal bool GetNextDigit() => + ++Index < Length && + DateTimeParse.IsDigit(Value[Index]); // // Get the current character. @@ -5437,8 +5412,7 @@ new DS[] { DS.ERROR, DS.TX_NNN, DS.TX_NNN, DS.TX_NNN, DS.ERROR, DS.ERROR, Other = 4, } - [IsByRefLike] - internal struct DTSubString + internal ref struct DTSubString { internal ReadOnlySpan s; internal Int32 index; diff --git a/src/mscorlib/src/System/Globalization/CompareInfo.Unix.cs b/src/mscorlib/src/System/Globalization/CompareInfo.Unix.cs index 0551648..7f22714 100644 --- a/src/mscorlib/src/System/Globalization/CompareInfo.Unix.cs +++ b/src/mscorlib/src/System/Globalization/CompareInfo.Unix.cs @@ -143,6 +143,23 @@ namespace System.Globalization return Interop.GlobalizationInterop.CompareStringOrdinalIgnoreCase(string1, count1, string2, count2); } + // TODO https://github.com/dotnet/coreclr/issues/13827: + // This method shouldn't be necessary, as we should be able to just use the overload + // that takes two spans. But due to this issue, that's adding significant overhead. + private unsafe int CompareString(ReadOnlySpan string1, string string2, CompareOptions options) + { + Debug.Assert(!_invariantMode); + + Debug.Assert(string2 != null); + Debug.Assert((options & (CompareOptions.Ordinal | CompareOptions.OrdinalIgnoreCase)) == 0); + + fixed (char* pString1 = &string1.DangerousGetPinnableReference()) + fixed (char* pString2 = &string2.GetRawStringData()) + { + return Interop.GlobalizationInterop.CompareString(_sortHandle, pString1, string1.Length, pString2, string2.Length, options); + } + } + private unsafe int CompareString(ReadOnlySpan string1, ReadOnlySpan string2, CompareOptions options) { Debug.Assert(!_invariantMode); diff --git a/src/mscorlib/src/System/Globalization/CompareInfo.Windows.cs b/src/mscorlib/src/System/Globalization/CompareInfo.Windows.cs index 83381e2..ff0c240 100644 --- a/src/mscorlib/src/System/Globalization/CompareInfo.Windows.cs +++ b/src/mscorlib/src/System/Globalization/CompareInfo.Windows.cs @@ -116,6 +116,42 @@ namespace System.Globalization return Interop.Kernel32.CompareStringOrdinal(string1, count1, string2, count2, true) - 2; } + // TODO https://github.com/dotnet/coreclr/issues/13827: + // This method shouldn't be necessary, as we should be able to just use the overload + // that takes two spans. But due to this issue, that's adding significant overhead. + private unsafe int CompareString(ReadOnlySpan string1, string string2, CompareOptions options) + { + Debug.Assert(string2 != null); + Debug.Assert(!_invariantMode); + Debug.Assert((options & (CompareOptions.Ordinal | CompareOptions.OrdinalIgnoreCase)) == 0); + + string localeName = _sortHandle != IntPtr.Zero ? null : _sortName; + + fixed (char* pLocaleName = localeName) + fixed (char* pString1 = &string1.DangerousGetPinnableReference()) + fixed (char* pString2 = &string2.GetRawStringData()) + { + int result = Interop.Kernel32.CompareStringEx( + pLocaleName, + (uint)GetNativeCompareFlags(options), + pString1, + string1.Length, + pString2, + string2.Length, + null, + null, + _sortHandle); + + if (result == 0) + { + Environment.FailFast("CompareStringEx failed"); + } + + // Map CompareStringEx return value to -1, 0, 1. + return result - 2; + } + } + private unsafe int CompareString(ReadOnlySpan string1, ReadOnlySpan string2, CompareOptions options) { Debug.Assert(!_invariantMode); diff --git a/src/mscorlib/src/System/Globalization/CompareInfo.cs b/src/mscorlib/src/System/Globalization/CompareInfo.cs index 581789e..47284f4 100644 --- a/src/mscorlib/src/System/Globalization/CompareInfo.cs +++ b/src/mscorlib/src/System/Globalization/CompareInfo.cs @@ -345,6 +345,48 @@ namespace System.Globalization return CompareString(string1.AsReadOnlySpan(), string2.AsReadOnlySpan(), options); } + // TODO https://github.com/dotnet/coreclr/issues/13827: + // This method shouldn't be necessary, as we should be able to just use the overload + // that takes two spans. But due to this issue, that's adding significant overhead. + internal unsafe int Compare(ReadOnlySpan string1, string string2, CompareOptions options) + { + if (options == CompareOptions.OrdinalIgnoreCase) + { + return CompareOrdinalIgnoreCase(string1, string2.AsReadOnlySpan()); + } + + // Verify the options before we do any real comparison. + if ((options & CompareOptions.Ordinal) != 0) + { + if (options != CompareOptions.Ordinal) + { + throw new ArgumentException(SR.Argument_CompareOptionOrdinal, nameof(options)); + } + + return string.CompareOrdinal(string1, string2.AsReadOnlySpan()); + } + + if ((options & ValidCompareMaskOffFlags) != 0) + { + throw new ArgumentException(SR.Argument_InvalidFlag, nameof(options)); + } + + // null sorts less than any other string. + if (string2 == null) + { + return 1; + } + + if (_invariantMode) + { + return (options & CompareOptions.IgnoreCase) != 0 ? + CompareOrdinalIgnoreCase(string1, string2.AsReadOnlySpan()) : + string.CompareOrdinal(string1, string2.AsReadOnlySpan()); + } + + return CompareString(string1, string2, options); + } + // TODO https://github.com/dotnet/corefx/issues/21395: Expose this publicly? internal unsafe virtual int Compare(ReadOnlySpan string1, ReadOnlySpan string2, CompareOptions options) { @@ -379,7 +421,6 @@ namespace System.Globalization return CompareString(string1, string2, options); } - //////////////////////////////////////////////////////////////////////// // // Compare -- 2.7.4