From 15d859d15fbc53024733d8c8030e6a6feb59ee86 Mon Sep 17 00:00:00 2001 From: ntr Date: Thu, 9 Dec 2021 14:39:26 +0100 Subject: [PATCH 1/8] Improve String.Substring performance --- .../src/System/String.Manipulation.cs | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs index d208eb3bedf63a..ca77cb41625430 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs @@ -1846,7 +1846,19 @@ private static void CheckStringSplitOptions(StringSplitOptions options) // Returns a substring of this string. // - public string Substring(int startIndex) => Substring(startIndex, Length - startIndex); + public string Substring(int startIndex) + { + if (startIndex == 0) + { + return this; + } + int length = Length; + if ((uint)startIndex > (uint)length) + { + throw new ArgumentOutOfRangeException(nameof(startIndex), SR.ArgumentOutOfRange_StartIndex); + } + return InternalSubString(startIndex, length - startIndex); + } public string Substring(int startIndex, int length) { From 626c6750805f6bd163315956ce2e6a4b2ac70075 Mon Sep 17 00:00:00 2001 From: ntr Date: Thu, 9 Dec 2021 15:08:11 +0100 Subject: [PATCH 2/8] copy Span.Slice checks for perf --- .../src/System/String.Manipulation.cs | 62 ++++++++++++------- 1 file changed, 39 insertions(+), 23 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs index ca77cb41625430..29626ce068d5a7 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs @@ -1852,42 +1852,58 @@ public string Substring(int startIndex) { return this; } + int length = Length; + // Copy Span.Slice checks for perf if ((uint)startIndex > (uint)length) - { - throw new ArgumentOutOfRangeException(nameof(startIndex), SR.ArgumentOutOfRange_StartIndex); - } + ThrowHelper.ThrowArgumentOutOfRangeException(); + return InternalSubString(startIndex, length - startIndex); } public string Substring(int startIndex, int length) { - if (startIndex < 0) - { - throw new ArgumentOutOfRangeException(nameof(startIndex), SR.ArgumentOutOfRange_StartIndex); - } - - if (startIndex > Length) - { - throw new ArgumentOutOfRangeException(nameof(startIndex), SR.ArgumentOutOfRange_StartIndexLargerThanLength); - } - - if (length < 0) - { - throw new ArgumentOutOfRangeException(nameof(length), SR.ArgumentOutOfRange_NegativeLength); - } - - if (startIndex > Length - length) - { - throw new ArgumentOutOfRangeException(nameof(length), SR.ArgumentOutOfRange_IndexLength); - } + // Copy Span.Slice checks for perf + int _length = Length; +#if TARGET_64BIT + // Since start and length are both 32-bit, their sum can be computed across a 64-bit domain + // without loss of fidelity. The cast to uint before the cast to ulong ensures that the + // extension from 32- to 64-bit is zero-extending rather than sign-extending. The end result + // of this is that if either input is negative or if the input sum overflows past Int32.MaxValue, + // that information is captured correctly in the comparison against the backing _length field. + // We don't use this same mechanism in a 32-bit process due to the overhead of 64-bit arithmetic. + if ((ulong)(uint)startIndex + (ulong)(uint)length > (ulong)(uint)_length) + ThrowHelper.ThrowArgumentOutOfRangeException(); +#else + if ((uint)startIndex > (uint)_length || (uint)length > (uint)(_length - startIndex)) + ThrowHelper.ThrowArgumentOutOfRangeException(); +#endif + //if (startIndex < 0) + //{ + // throw new ArgumentOutOfRangeException(nameof(startIndex), SR.ArgumentOutOfRange_StartIndex); + //} + + //if (startIndex > Length) + //{ + // throw new ArgumentOutOfRangeException(nameof(startIndex), SR.ArgumentOutOfRange_StartIndexLargerThanLength); + //} + + //if (length < 0) + //{ + // throw new ArgumentOutOfRangeException(nameof(length), SR.ArgumentOutOfRange_NegativeLength); + //} + + //if (startIndex > Length - length) + //{ + // throw new ArgumentOutOfRangeException(nameof(length), SR.ArgumentOutOfRange_IndexLength); + //} if (length == 0) { return string.Empty; } - if (startIndex == 0 && length == this.Length) + if (startIndex == 0 && length == _length) { return this; } From 5550268b9978e2d3981c1f5515a1c51e85871f83 Mon Sep 17 00:00:00 2001 From: ntr Date: Fri, 10 Dec 2021 15:22:21 +0100 Subject: [PATCH 3/8] address feedback, separate detailed throw --- .../src/System/String.Manipulation.cs | 63 ++++++++++--------- 1 file changed, 35 insertions(+), 28 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs index 29626ce068d5a7..0732e26f5d76d5 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs @@ -3,6 +3,7 @@ using System.Collections.Generic; using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; using System.Globalization; using System.Numerics; using System.Runtime.InteropServices; @@ -1853,18 +1854,23 @@ public string Substring(int startIndex) return this; } - int length = Length; + int thisLength = Length; + if (startIndex == thisLength) + { + return string.Empty; + } + // Copy Span.Slice checks for perf - if ((uint)startIndex > (uint)length) - ThrowHelper.ThrowArgumentOutOfRangeException(); + if ((uint)startIndex > (uint)thisLength) + ThrowSubstringArgumentOutRangeException(startIndex, thisLength - startIndex); - return InternalSubString(startIndex, length - startIndex); + return InternalSubString(startIndex, thisLength - startIndex); } public string Substring(int startIndex, int length) { // Copy Span.Slice checks for perf - int _length = Length; + int thisLength = Length; #if TARGET_64BIT // Since start and length are both 32-bit, their sum can be computed across a 64-bit domain // without loss of fidelity. The cast to uint before the cast to ulong ensures that the @@ -1872,38 +1878,19 @@ public string Substring(int startIndex, int length) // of this is that if either input is negative or if the input sum overflows past Int32.MaxValue, // that information is captured correctly in the comparison against the backing _length field. // We don't use this same mechanism in a 32-bit process due to the overhead of 64-bit arithmetic. - if ((ulong)(uint)startIndex + (ulong)(uint)length > (ulong)(uint)_length) + if ((ulong)(uint)startIndex + (ulong)(uint)length > (ulong)(uint)thisLength) ThrowHelper.ThrowArgumentOutOfRangeException(); #else - if ((uint)startIndex > (uint)_length || (uint)length > (uint)(_length - startIndex)) - ThrowHelper.ThrowArgumentOutOfRangeException(); + if ((uint)startIndex > (uint)thisLength || (uint)length > (uint)(thisLength - startIndex)) + ThrowSubstringArgumentOutRangeException(startIndex, length); #endif - //if (startIndex < 0) - //{ - // throw new ArgumentOutOfRangeException(nameof(startIndex), SR.ArgumentOutOfRange_StartIndex); - //} - - //if (startIndex > Length) - //{ - // throw new ArgumentOutOfRangeException(nameof(startIndex), SR.ArgumentOutOfRange_StartIndexLargerThanLength); - //} - - //if (length < 0) - //{ - // throw new ArgumentOutOfRangeException(nameof(length), SR.ArgumentOutOfRange_NegativeLength); - //} - - //if (startIndex > Length - length) - //{ - // throw new ArgumentOutOfRangeException(nameof(length), SR.ArgumentOutOfRange_IndexLength); - //} if (length == 0) { return string.Empty; } - if (startIndex == 0 && length == _length) + if (length == thisLength) { return this; } @@ -1911,6 +1898,26 @@ public string Substring(int startIndex, int length) return InternalSubString(startIndex, length); } + [DoesNotReturn] + private void ThrowSubstringArgumentOutRangeException(int startIndex, int length) + { + if (startIndex < 0) + { + throw new ArgumentOutOfRangeException(nameof(startIndex), SR.ArgumentOutOfRange_StartIndex); + } + if (startIndex > Length) + { + throw new ArgumentOutOfRangeException(nameof(startIndex), SR.ArgumentOutOfRange_StartIndexLargerThanLength); + } + if (length < 0) + { + throw new ArgumentOutOfRangeException(nameof(length), SR.ArgumentOutOfRange_NegativeLength); + } + // Check not needed since we only call this when startIndex, length are out of range + //if (startIndex > Length - length) + throw new ArgumentOutOfRangeException(nameof(length), SR.ArgumentOutOfRange_IndexLength); + } + private string InternalSubString(int startIndex, int length) { Debug.Assert(startIndex >= 0 && startIndex <= this.Length, "StartIndex is out of range!"); From bdb0ae3fa8e394fdc04fb356936737739aef091b Mon Sep 17 00:00:00 2001 From: ntr Date: Sat, 11 Dec 2021 14:33:19 +0100 Subject: [PATCH 4/8] short length copy --- .../src/System/String.Manipulation.cs | 43 ++++++++++++++++--- 1 file changed, 38 insertions(+), 5 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs index 0732e26f5d76d5..e4378d24adbb6d 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs @@ -6,6 +6,7 @@ using System.Diagnostics.CodeAnalysis; using System.Globalization; using System.Numerics; +using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Runtime.Intrinsics; using System.Runtime.Intrinsics.X86; @@ -1918,6 +1919,7 @@ private void ThrowSubstringArgumentOutRangeException(int startIndex, int length) throw new ArgumentOutOfRangeException(nameof(length), SR.ArgumentOutOfRange_IndexLength); } + [MethodImpl(MethodImplOptions.AggressiveInlining)] private string InternalSubString(int startIndex, int length) { Debug.Assert(startIndex >= 0 && startIndex <= this.Length, "StartIndex is out of range!"); @@ -1925,11 +1927,42 @@ private string InternalSubString(int startIndex, int length) string result = FastAllocateString(length); - Buffer.Memmove( - elementCount: (uint)result.Length, // derefing Length now allows JIT to prove 'result' not null below - destination: ref result._firstChar, - source: ref Unsafe.Add(ref _firstChar, (nint)(uint)startIndex /* force zero-extension */)); - + ref char source = ref Unsafe.Add(ref _firstChar, (nint)(uint)startIndex); /* force zero-extension */ + ref char destination = ref result._firstChar; + if (length < 16) + { + int offset = 0; + if (length >= 8) + { + Unsafe.As(ref destination) = Unsafe.As(ref source); + Unsafe.As(ref Unsafe.Add(ref destination, 4)) = Unsafe.As(ref Unsafe.Add(ref source, 4)); + length -= 8; + offset += 8; + } + if (length >= 4) + { + Unsafe.As(ref Unsafe.Add(ref destination, offset)) = Unsafe.As(ref Unsafe.Add(ref source, offset)); + length -= 4; + offset += 4; + } + if (length >= 2) + { + Unsafe.As(ref Unsafe.Add(ref destination, offset)) = Unsafe.As(ref Unsafe.Add(ref source, offset)); + length -= 2; + offset += 2; + } + if (length > 0) + { + Unsafe.Add(ref destination, offset) = Unsafe.Add(ref source, offset); + } + } + else + { + Buffer.Memmove( + elementCount: (uint)result.Length, // derefing Length now allows JIT to prove 'result' not null below + destination: ref destination, + source: ref source); + } return result; } From 81aa0087216020801efbdfb585587e2e553b2217 Mon Sep 17 00:00:00 2001 From: ntr Date: Tue, 14 Dec 2021 12:54:44 +0100 Subject: [PATCH 5/8] revert small copy code --- .../src/System/String.Manipulation.cs | 42 +++---------------- 1 file changed, 5 insertions(+), 37 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs index e4378d24adbb6d..ee77fdd5ef238a 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs @@ -1919,7 +1919,6 @@ private void ThrowSubstringArgumentOutRangeException(int startIndex, int length) throw new ArgumentOutOfRangeException(nameof(length), SR.ArgumentOutOfRange_IndexLength); } - [MethodImpl(MethodImplOptions.AggressiveInlining)] private string InternalSubString(int startIndex, int length) { Debug.Assert(startIndex >= 0 && startIndex <= this.Length, "StartIndex is out of range!"); @@ -1927,42 +1926,11 @@ private string InternalSubString(int startIndex, int length) string result = FastAllocateString(length); - ref char source = ref Unsafe.Add(ref _firstChar, (nint)(uint)startIndex); /* force zero-extension */ - ref char destination = ref result._firstChar; - if (length < 16) - { - int offset = 0; - if (length >= 8) - { - Unsafe.As(ref destination) = Unsafe.As(ref source); - Unsafe.As(ref Unsafe.Add(ref destination, 4)) = Unsafe.As(ref Unsafe.Add(ref source, 4)); - length -= 8; - offset += 8; - } - if (length >= 4) - { - Unsafe.As(ref Unsafe.Add(ref destination, offset)) = Unsafe.As(ref Unsafe.Add(ref source, offset)); - length -= 4; - offset += 4; - } - if (length >= 2) - { - Unsafe.As(ref Unsafe.Add(ref destination, offset)) = Unsafe.As(ref Unsafe.Add(ref source, offset)); - length -= 2; - offset += 2; - } - if (length > 0) - { - Unsafe.Add(ref destination, offset) = Unsafe.Add(ref source, offset); - } - } - else - { - Buffer.Memmove( - elementCount: (uint)result.Length, // derefing Length now allows JIT to prove 'result' not null below - destination: ref destination, - source: ref source); - } + Buffer.Memmove( + elementCount: (uint)result.Length, // derefing Length now allows JIT to prove 'result' not null below + destination: ref result._firstChar, + source: ref Unsafe.Add(ref _firstChar, (nint)(uint)startIndex) /* force zero-extension */); + return result; } From 4fc810848116d5b238db2059c2543497c0215574 Mon Sep 17 00:00:00 2001 From: ntr Date: Tue, 14 Dec 2021 13:01:10 +0100 Subject: [PATCH 6/8] nit --- .../System.Private.CoreLib/src/System/String.Manipulation.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs index ee77fdd5ef238a..e98720f5d65017 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs @@ -1880,7 +1880,7 @@ public string Substring(int startIndex, int length) // that information is captured correctly in the comparison against the backing _length field. // We don't use this same mechanism in a 32-bit process due to the overhead of 64-bit arithmetic. if ((ulong)(uint)startIndex + (ulong)(uint)length > (ulong)(uint)thisLength) - ThrowHelper.ThrowArgumentOutOfRangeException(); + ThrowSubstringArgumentOutRangeException(startIndex, length); #else if ((uint)startIndex > (uint)thisLength || (uint)length > (uint)(thisLength - startIndex)) ThrowSubstringArgumentOutRangeException(startIndex, length); From 7389f9b84ff04139898759e0761fa02376ad4ad3 Mon Sep 17 00:00:00 2001 From: ntr Date: Tue, 14 Dec 2021 13:02:01 +0100 Subject: [PATCH 7/8] paren nit --- .../System.Private.CoreLib/src/System/String.Manipulation.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs index e98720f5d65017..9c1e4f6a984d06 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs @@ -1929,7 +1929,7 @@ private string InternalSubString(int startIndex, int length) Buffer.Memmove( elementCount: (uint)result.Length, // derefing Length now allows JIT to prove 'result' not null below destination: ref result._firstChar, - source: ref Unsafe.Add(ref _firstChar, (nint)(uint)startIndex) /* force zero-extension */); + source: ref Unsafe.Add(ref _firstChar, (nint)(uint)startIndex /* force zero-extension */)); return result; } From 15ce699638e66d9da39c1445bd8b749406a2fb57 Mon Sep 17 00:00:00 2001 From: ntr Date: Tue, 14 Dec 2021 13:02:56 +0100 Subject: [PATCH 8/8] clean usings --- .../System.Private.CoreLib/src/System/String.Manipulation.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs index 9c1e4f6a984d06..74c0c243d480bc 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs @@ -6,7 +6,6 @@ using System.Diagnostics.CodeAnalysis; using System.Globalization; using System.Numerics; -using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Runtime.Intrinsics; using System.Runtime.Intrinsics.X86;