From 698bc15409896d2f9963d1208437ef5548c3a927 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Thu, 9 Sep 2021 21:10:56 +0300 Subject: [PATCH 01/12] Eliminate pinning in StringBuilder.Append(ReadOnlySpan). --- .../src/System/Text/StringBuilder.cs | 56 +++++++++---------- 1 file changed, 25 insertions(+), 31 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs b/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs index 8835e0ccae7ce8..8c58bf3ee613b6 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs @@ -1218,15 +1218,35 @@ public StringBuilder Append(char[]? value) public StringBuilder Append(ReadOnlySpan value) { - if (value.Length > 0) + if (value.Length != 0) { - unsafe + // This case is so common we want to optimize for it heavily. + int newIndex = value.Length + m_ChunkLength; + if (newIndex <= m_ChunkChars.Length) + { + value.CopyTo(m_ChunkChars.AsSpan(m_ChunkLength)); + m_ChunkLength = newIndex; + } + else { - fixed (char* valueChars = &MemoryMarshal.GetReference(value)) + // Copy the first chunk + int firstLength = m_ChunkChars.Length - m_ChunkLength; + if (firstLength > 0) { - Append(valueChars, value.Length); + value.Slice(0, firstLength).CopyTo(m_ChunkChars.AsSpan(m_ChunkLength)); + m_ChunkLength = m_ChunkChars.Length; } + + // Expand the builder to add another chunk. + int restLength = value.Length - firstLength; + ExpandByABlock(restLength); + Debug.Assert(m_ChunkLength == 0, "A new block was not created."); + + // Copy the second chunk + value.Slice(firstLength).CopyTo(m_ChunkChars); + m_ChunkLength = restLength; } + AssertInvariants(); } return this; } @@ -2108,33 +2128,7 @@ public unsafe StringBuilder Append(char* value, int valueCount) throw new ArgumentOutOfRangeException(nameof(valueCount), SR.ArgumentOutOfRange_LengthGreaterThanCapacity); } - // This case is so common we want to optimize for it heavily. - int newIndex = valueCount + m_ChunkLength; - if (newIndex <= m_ChunkChars.Length) - { - new ReadOnlySpan(value, valueCount).CopyTo(m_ChunkChars.AsSpan(m_ChunkLength)); - m_ChunkLength = newIndex; - } - else - { - // Copy the first chunk - int firstLength = m_ChunkChars.Length - m_ChunkLength; - if (firstLength > 0) - { - new ReadOnlySpan(value, firstLength).CopyTo(m_ChunkChars.AsSpan(m_ChunkLength)); - m_ChunkLength = m_ChunkChars.Length; - } - - // Expand the builder to add another chunk. - int restLength = valueCount - firstLength; - ExpandByABlock(restLength); - Debug.Assert(m_ChunkLength == 0, "A new block was not created."); - - // Copy the second chunk - new ReadOnlySpan(value + firstLength, restLength).CopyTo(m_ChunkChars); - m_ChunkLength = restLength; - } - AssertInvariants(); + Append(new ReadOnlySpan(value, valueCount)); return this; } From a5ba30072ac1e7109582bbf3cc4235848fedd2f0 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Thu, 9 Sep 2021 21:24:36 +0300 Subject: [PATCH 02/12] Eliminate pinning in all StringBuilder.Append overloads. --- .../src/System/Text/StringBuilder.cs | 43 +++---------------- 1 file changed, 6 insertions(+), 37 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs b/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs index 8c58bf3ee613b6..2ea2e7cf60cc16 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs @@ -776,14 +776,8 @@ public StringBuilder Append(char[]? value, int startIndex, int charCount) return this; } - unsafe - { - fixed (char* valueChars = &value[startIndex]) - { - Append(valueChars, charCount); - return this; - } - } + Append(value.AsSpan(startIndex, charCount)); + return this; } /// @@ -824,26 +818,13 @@ ref value.GetRawStringData(), } else { - AppendHelper(value); + Append(value.AsSpan()); } } return this; } - // We put this fixed in its own helper to avoid the cost of zero-initing `valueChars` in the - // case we don't actually use it. - private void AppendHelper(string value) - { - unsafe - { - fixed (char* valueChars = value) - { - Append(valueChars, value.Length); - } - } - } - /// /// Appends part of a string to the end of this builder. /// @@ -880,14 +861,8 @@ public StringBuilder Append(string? value, int startIndex, int count) throw new ArgumentOutOfRangeException(nameof(startIndex), SR.ArgumentOutOfRange_Index); } - unsafe - { - fixed (char* valueChars = value) - { - Append(valueChars + startIndex, count); - return this; - } - } + Append(value.AsSpan(startIndex, count)); + return this; } public StringBuilder Append(StringBuilder? value) @@ -1205,13 +1180,7 @@ public StringBuilder Append(char[]? value) { if (value?.Length > 0) { - unsafe - { - fixed (char* valueChars = &value[0]) - { - Append(valueChars, value.Length); - } - } + Append(value.AsSpan()); } return this; } From 8cbdef564d32b459cbab7d979530376e7d9376c3 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Thu, 9 Sep 2021 21:33:21 +0300 Subject: [PATCH 03/12] Eliminate pinning in all StringBuilder.AppendJoin overloads. --- .../src/System/Text/StringBuilder.cs | 44 +++++++------------ 1 file changed, 16 insertions(+), 28 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs b/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs index 2ea2e7cf60cc16..222ce6b5999189 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs @@ -1246,53 +1246,41 @@ public StringBuilder Append(ReadOnlySpan value) #region AppendJoin - public unsafe StringBuilder AppendJoin(string? separator, params object?[] values) + public StringBuilder AppendJoin(string? separator, params object?[] values) { separator ??= string.Empty; - fixed (char* pSeparator = separator) - { - return AppendJoinCore(pSeparator, separator.Length, values); - } + return AppendJoinCore(separator.AsSpan(), values); } - public unsafe StringBuilder AppendJoin(string? separator, IEnumerable values) + public StringBuilder AppendJoin(string? separator, IEnumerable values) { separator ??= string.Empty; - fixed (char* pSeparator = separator) - { - return AppendJoinCore(pSeparator, separator.Length, values); - } + return AppendJoinCore(separator.AsSpan(), values); } - public unsafe StringBuilder AppendJoin(string? separator, params string?[] values) + public StringBuilder AppendJoin(string? separator, params string?[] values) { separator ??= string.Empty; - fixed (char* pSeparator = separator) - { - return AppendJoinCore(pSeparator, separator.Length, values); - } + return AppendJoinCore(separator.AsSpan(), values); } - public unsafe StringBuilder AppendJoin(char separator, params object?[] values) + public StringBuilder AppendJoin(char separator, params object?[] values) { - return AppendJoinCore(&separator, 1, values); + return AppendJoinCore(new ReadOnlySpan(ref separator, 1), values); } - public unsafe StringBuilder AppendJoin(char separator, IEnumerable values) + public StringBuilder AppendJoin(char separator, IEnumerable values) { - return AppendJoinCore(&separator, 1, values); + return AppendJoinCore(new ReadOnlySpan(ref separator, 1), values); } - public unsafe StringBuilder AppendJoin(char separator, params string?[] values) + public StringBuilder AppendJoin(char separator, params string?[] values) { - return AppendJoinCore(&separator, 1, values); + return AppendJoinCore(new ReadOnlySpan(ref separator, 1), values); } - private unsafe StringBuilder AppendJoinCore(char* separator, int separatorLength, IEnumerable values) + private StringBuilder AppendJoinCore(ReadOnlySpan separator, IEnumerable values) { - Debug.Assert(separator != null); - Debug.Assert(separatorLength >= 0); - if (values == null) { ThrowHelper.ThrowArgumentNullException(ExceptionArgument.values); @@ -1314,7 +1302,7 @@ private unsafe StringBuilder AppendJoinCore(char* separator, int separatorLen while (en.MoveNext()) { - Append(separator, separatorLength); + Append(separator); value = en.Current; if (value != null) { @@ -1325,7 +1313,7 @@ private unsafe StringBuilder AppendJoinCore(char* separator, int separatorLen return this; } - private unsafe StringBuilder AppendJoinCore(char* separator, int separatorLength, T[] values) + private StringBuilder AppendJoinCore(ReadOnlySpan separator, T[] values) { if (values == null) { @@ -1345,7 +1333,7 @@ private unsafe StringBuilder AppendJoinCore(char* separator, int separatorLen for (int i = 1; i < values.Length; i++) { - Append(separator, separatorLength); + Append(separator); if (values[i] != null) { Append(values[i]!.ToString()); From 683d29064e8bffe8959ce3386e53a4659fbcc3b7 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Thu, 9 Sep 2021 22:06:08 +0300 Subject: [PATCH 04/12] Eliminate pinning in all StringBuilder.Insert overloads. --- .../src/System/Text/StringBuilder.cs | 198 +++++++----------- 1 file changed, 71 insertions(+), 127 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs b/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs index 222ce6b5999189..e1b183c0a61374 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs @@ -690,7 +690,7 @@ public ManyChunkInfo(StringBuilder? stringBuilder, int chunkCount) _chunkPos = -1; } } -#endregion + #endregion } /// @@ -1048,19 +1048,13 @@ public StringBuilder Insert(int index, string? value, int count) Debug.Assert(insertingChars + this.Length < int.MaxValue); MakeRoom(index, (int)insertingChars, out StringBuilder chunk, out int indexInChunk, false); - unsafe + while (count > 0) { - fixed (char* valuePtr = value) - { - while (count > 0) - { - ReplaceInPlaceAtChunk(ref chunk!, ref indexInChunk, valuePtr, value.Length); - --count; - } - - return this; - } + ReplaceInPlaceAtChunk(ref chunk!, ref indexInChunk, value); + --count; } + + return this; } /// @@ -1353,11 +1347,7 @@ public StringBuilder Insert(int index, string? value) if (value != null) { - unsafe - { - fixed (char* sourcePtr = value) - Insert(index, sourcePtr, value.Length); - } + Insert(index, value.AsSpan()); } return this; } @@ -1373,10 +1363,7 @@ public StringBuilder Insert(int index, string? value) public StringBuilder Insert(int index, char value) { - unsafe - { - Insert(index, &value, 1); - } + Insert(index, new ReadOnlySpan(ref value, 1)); return this; } @@ -1428,11 +1415,7 @@ public StringBuilder Insert(int index, char[]? value, int startIndex, int charCo if (charCount > 0) { - unsafe - { - fixed (char* sourcePtr = &value[startIndex]) - Insert(index, sourcePtr, charCount); - } + Insert(index, value.AsSpan(startIndex, charCount)); } return this; } @@ -1467,11 +1450,8 @@ public StringBuilder Insert(int index, ReadOnlySpan value) if (value.Length > 0) { - unsafe - { - fixed (char* sourcePtr = &MemoryMarshal.GetReference(value)) - Insert(index, sourcePtr, value.Length); - } + MakeRoom(index, value.Length, out StringBuilder chunk, out int indexInChunk, false); + ReplaceInPlaceAtChunk(ref chunk!, ref indexInChunk, value); } return this; } @@ -2089,26 +2069,6 @@ public unsafe StringBuilder Append(char* value, int valueCount) return this; } - /// - /// Inserts a character buffer into this builder at the specified position. - /// - /// The index to insert in this builder. - /// The pointer to the start of the buffer. - /// The number of characters in the buffer. - private unsafe void Insert(int index, char* value, int valueCount) - { - if ((uint)index > (uint)Length) - { - throw new ArgumentOutOfRangeException(nameof(index), SR.ArgumentOutOfRange_Index); - } - - if (valueCount > 0) - { - MakeRoom(index, valueCount, out StringBuilder chunk, out int indexInChunk, false); - ReplaceInPlaceAtChunk(ref chunk!, ref indexInChunk, value, valueCount); - } - } - /// /// Replaces strings at specified indices with a new string in a chunk. /// @@ -2127,65 +2087,58 @@ private void ReplaceAllInChunk(int[]? replacements, int replacementsCount, Strin return; } - unsafe + Debug.Assert(replacements != null, "replacements was null when replacementsCount > 0"); + // calculate the total amount of extra space or space needed for all the replacements. + long longDelta = (value.Length - removeCount) * (long)replacementsCount; + int delta = (int)longDelta; + if (delta != longDelta) { - fixed (char* valuePtr = value) - { - Debug.Assert(replacements != null, "replacements was null when replacementsCount > 0"); - // calculate the total amount of extra space or space needed for all the replacements. - long longDelta = (value.Length - removeCount) * (long)replacementsCount; - int delta = (int)longDelta; - if (delta != longDelta) - { - throw new OutOfMemoryException(); - } - - StringBuilder targetChunk = sourceChunk; // the target as we copy chars down - int targetIndexInChunk = replacements[0]; + throw new OutOfMemoryException(); + } - // Make the room needed for all the new characters if needed. - if (delta > 0) - { - MakeRoom(targetChunk.m_ChunkOffset + targetIndexInChunk, delta, out targetChunk, out targetIndexInChunk, true); - } + StringBuilder targetChunk = sourceChunk; // the target as we copy chars down + int targetIndexInChunk = replacements[0]; - // We made certain that characters after the insertion point are not moved, - int i = 0; - while (true) - { - // Copy in the new string for the ith replacement - ReplaceInPlaceAtChunk(ref targetChunk!, ref targetIndexInChunk, valuePtr, value.Length); - int gapStart = replacements[i] + removeCount; - i++; - if (i >= replacementsCount) - { - break; - } + // Make the room needed for all the new characters if needed. + if (delta > 0) + { + MakeRoom(targetChunk.m_ChunkOffset + targetIndexInChunk, delta, out targetChunk, out targetIndexInChunk, true); + } - int gapEnd = replacements[i]; - Debug.Assert(gapStart < sourceChunk.m_ChunkChars.Length, "gap starts at end of buffer. Should not happen"); - Debug.Assert(gapStart <= gapEnd, "negative gap size"); - Debug.Assert(gapEnd <= sourceChunk.m_ChunkLength, "gap too big"); - if (delta != 0) // can skip the sliding of gaps if source an target string are the same size. - { - // Copy the gap data between the current replacement and the next replacement - fixed (char* sourcePtr = &sourceChunk.m_ChunkChars[gapStart]) - ReplaceInPlaceAtChunk(ref targetChunk!, ref targetIndexInChunk, sourcePtr, gapEnd - gapStart); - } - else - { - targetIndexInChunk += gapEnd - gapStart; - Debug.Assert(targetIndexInChunk <= targetChunk.m_ChunkLength, "gap not in chunk"); - } - } + // We made certain that characters after the insertion point are not moved, + int i = 0; + while (true) + { + // Copy in the new string for the ith replacement + ReplaceInPlaceAtChunk(ref targetChunk!, ref targetIndexInChunk, value); + int gapStart = replacements[i] + removeCount; + i++; + if (i >= replacementsCount) + { + break; + } - // Remove extra space if necessary. - if (delta < 0) - { - Remove(targetChunk.m_ChunkOffset + targetIndexInChunk, -delta, out targetChunk, out targetIndexInChunk); - } + int gapEnd = replacements[i]; + Debug.Assert(gapStart < sourceChunk.m_ChunkChars.Length, "gap starts at end of buffer. Should not happen"); + Debug.Assert(gapStart <= gapEnd, "negative gap size"); + Debug.Assert(gapEnd <= sourceChunk.m_ChunkLength, "gap too big"); + if (delta != 0) // can skip the sliding of gaps if source an target string are the same size. + { + // Copy the gap data between the current replacement and the next replacement + ReplaceInPlaceAtChunk(ref targetChunk!, ref targetIndexInChunk, sourceChunk.m_ChunkChars.AsSpan(gapStart, gapEnd - gapStart)); + } + else + { + targetIndexInChunk += gapEnd - gapStart; + Debug.Assert(targetIndexInChunk <= targetChunk.m_ChunkLength, "gap not in chunk"); } } + + // Remove extra space if necessary. + if (delta < 0) + { + Remove(targetChunk.m_ChunkOffset + targetIndexInChunk, -delta, out targetChunk, out targetIndexInChunk); + } } /// @@ -2238,35 +2191,26 @@ private bool StartsWith(StringBuilder chunk, int indexInChunk, int count, string /// The index in to start replacing characters at. /// Receives the index at which character replacement ends. /// - /// The pointer to the start of the character buffer. - /// The number of characters in the buffer. - private unsafe void ReplaceInPlaceAtChunk(ref StringBuilder? chunk, ref int indexInChunk, char* value, int count) + /// The character buffer. + private void ReplaceInPlaceAtChunk(ref StringBuilder? chunk, ref int indexInChunk, ReadOnlySpan value) { - if (count != 0) + while (!value.IsEmpty) { - while (true) - { - Debug.Assert(chunk != null, "chunk should not be null at this point"); - int lengthInChunk = chunk.m_ChunkLength - indexInChunk; - Debug.Assert(lengthInChunk >= 0, "Index isn't in the chunk."); + Debug.Assert(chunk != null, "chunk should not be null at this point"); + int lengthInChunk = chunk.m_ChunkLength - indexInChunk; + Debug.Assert(lengthInChunk >= 0, "Index isn't in the chunk."); - int lengthToCopy = Math.Min(lengthInChunk, count); - new ReadOnlySpan(value, lengthToCopy).CopyTo(chunk.m_ChunkChars.AsSpan(indexInChunk)); + int lengthToCopy = Math.Min(lengthInChunk, value.Length); + value.Slice(0, lengthToCopy).CopyTo(chunk.m_ChunkChars.AsSpan(indexInChunk)); - // Advance the index. - indexInChunk += lengthToCopy; - if (indexInChunk >= chunk.m_ChunkLength) - { - chunk = Next(chunk); - indexInChunk = 0; - } - count -= lengthToCopy; - if (count == 0) - { - break; - } - value += lengthToCopy; + // Advance the index. + indexInChunk += lengthToCopy; + if (indexInChunk >= chunk.m_ChunkLength) + { + chunk = Next(chunk); + indexInChunk = 0; } + value = value.Slice(lengthToCopy); } } From 99d9fbc8b0c3cd5c713b8ea968b543f412402eb5 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Thu, 9 Sep 2021 22:08:42 +0300 Subject: [PATCH 05/12] Eliminate pinning in StringBuilder.ToString(int, int). --- .../src/System/Text/StringBuilder.cs | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs b/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs index e1b183c0a61374..f7c6fb437ca4cf 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs @@ -408,14 +408,8 @@ public string ToString(int startIndex, int length) AssertInvariants(); string result = string.FastAllocateString(length); - unsafe - { - fixed (char* destinationPtr = result) - { - this.CopyTo(startIndex, new Span(destinationPtr, length), length); - return result; - } - } + this.CopyTo(startIndex, new Span(ref result.GetRawStringData(), length), length); + return result; } public StringBuilder Clear() From 85bcf34b867ba9d93a6c41d5a811f632c080d5df Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Thu, 9 Sep 2021 23:09:08 +0300 Subject: [PATCH 06/12] Optimize the StringBuilder.Insert overloads of primitives. And streamline the character array Insert overload. --- .../src/System/Text/StringBuilder.cs | 51 +++++++++++-------- 1 file changed, 31 insertions(+), 20 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs b/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs index f7c6fb437ca4cf..04d592fbd8f06c 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs @@ -782,7 +782,7 @@ public StringBuilder Append(string? value) { if (value != null) { - // We could have just called AppendHelper here; this is a hand-specialization of that code. + // We could have just appended the span here; this is a hand-specialization of that code. char[] chunkChars = m_ChunkChars; int chunkLength = m_ChunkLength; int valueLen = value.Length; @@ -1346,20 +1346,18 @@ public StringBuilder Insert(int index, string? value) return this; } - public StringBuilder Insert(int index, bool value) => Insert(index, value.ToString(), 1); +#pragma warning disable CA1830 // Prefer strongly-typed Append and Insert method overloads on StringBuilder. No need to fix for the builder itself + public StringBuilder Insert(int index, bool value) => Insert(index, value.ToString()); +#pragma warning restore CA1830 [CLSCompliant(false)] - public StringBuilder Insert(int index, sbyte value) => Insert(index, value.ToString(), 1); + public StringBuilder Insert(int index, sbyte value) => InsertSpanFormattable(index, value); - public StringBuilder Insert(int index, byte value) => Insert(index, value.ToString(), 1); + public StringBuilder Insert(int index, byte value) => InsertSpanFormattable(index, value); - public StringBuilder Insert(int index, short value) => Insert(index, value.ToString(), 1); + public StringBuilder Insert(int index, short value) => InsertSpanFormattable(index, value); - public StringBuilder Insert(int index, char value) - { - Insert(index, new ReadOnlySpan(ref value, 1)); - return this; - } + public StringBuilder Insert(int index, char value) => Insert(index, new ReadOnlySpan(ref value, 1)); public StringBuilder Insert(int index, char[]? value) { @@ -1370,7 +1368,7 @@ public StringBuilder Insert(int index, char[]? value) if (value != null) { - Insert(index, value, 0, value.Length); + Insert(index, value.AsSpan()); } return this; } @@ -1414,26 +1412,26 @@ public StringBuilder Insert(int index, char[]? value, int startIndex, int charCo return this; } - public StringBuilder Insert(int index, int value) => Insert(index, value.ToString(), 1); + public StringBuilder Insert(int index, int value) => InsertSpanFormattable(index, value); - public StringBuilder Insert(int index, long value) => Insert(index, value.ToString(), 1); + public StringBuilder Insert(int index, long value) => InsertSpanFormattable(index, value); - public StringBuilder Insert(int index, float value) => Insert(index, value.ToString(), 1); + public StringBuilder Insert(int index, float value) => InsertSpanFormattable(index, value); - public StringBuilder Insert(int index, double value) => Insert(index, value.ToString(), 1); + public StringBuilder Insert(int index, double value) => InsertSpanFormattable(index, value); - public StringBuilder Insert(int index, decimal value) => Insert(index, value.ToString(), 1); + public StringBuilder Insert(int index, decimal value) => InsertSpanFormattable(index, value); [CLSCompliant(false)] - public StringBuilder Insert(int index, ushort value) => Insert(index, value.ToString(), 1); + public StringBuilder Insert(int index, ushort value) => InsertSpanFormattable(index, value); [CLSCompliant(false)] - public StringBuilder Insert(int index, uint value) => Insert(index, value.ToString(), 1); + public StringBuilder Insert(int index, uint value) => InsertSpanFormattable(index, value); [CLSCompliant(false)] - public StringBuilder Insert(int index, ulong value) => Insert(index, value.ToString(), 1); + public StringBuilder Insert(int index, ulong value) => InsertSpanFormattable(index, value); - public StringBuilder Insert(int index, object? value) => (value == null) ? this : Insert(index, value.ToString(), 1); + public StringBuilder Insert(int index, object? value) => (value == null) ? this : Insert(index, value.ToString()); public StringBuilder Insert(int index, ReadOnlySpan value) { @@ -1450,6 +1448,19 @@ public StringBuilder Insert(int index, ReadOnlySpan value) return this; } + private StringBuilder InsertSpanFormattable(int index, T value) where T : ISpanFormattable + { + Debug.Assert(typeof(T).Assembly.Equals(typeof(object).Assembly), "Implementation trusts the results of TryFormat because T is expected to be something known"); + + Span buffer = stackalloc char[256]; + if (value.TryFormat(buffer, out int charsWritten, format: default, provider: null)) + { + return Insert(index, buffer.Slice(0, charsWritten)); + } + + return Insert(index, value.ToString()); + } + public StringBuilder AppendFormat(string format, object? arg0) => AppendFormatHelper(null, format, new ParamsArray(arg0)); public StringBuilder AppendFormat(string format, object? arg0, object? arg1) => AppendFormatHelper(null, format, new ParamsArray(arg0, arg1)); From 3e5700ca40fcd742c5ab40a8385f5c048bd15337 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Fri, 10 Sep 2021 11:39:04 +0300 Subject: [PATCH 07/12] Throw OOM when StringBuilder.Insert exceeds capacity, like before. And move the capacity check in Append(ReadOnlySpan). --- .../src/System/Text/StringBuilder.cs | 32 +++++++++++-------- .../tests/System/Text/StringBuilderTests.cs | 2 +- 2 files changed, 19 insertions(+), 15 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs b/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs index 04d592fbd8f06c..8e976589ed9e8e 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs @@ -1014,7 +1014,9 @@ public void CopyTo(int sourceIndex, Span destination, int count) /// The index to insert in this builder. /// The string to insert. /// The number of times to insert the string. - public StringBuilder Insert(int index, string? value, int count) + public StringBuilder Insert(int index, string? value, int count) => Insert(index, value.AsSpan(), count); + + private StringBuilder Insert(int index, ReadOnlySpan value, int count) { if (count < 0) { @@ -1027,7 +1029,7 @@ public StringBuilder Insert(int index, string? value, int count) throw new ArgumentOutOfRangeException(nameof(index), SR.ArgumentOutOfRange_Index); } - if (string.IsNullOrEmpty(value) || count == 0) + if (value.IsEmpty || count == 0) { return this; } @@ -1175,6 +1177,14 @@ public StringBuilder Append(char[]? value) public StringBuilder Append(ReadOnlySpan value) { + // This is where we can check if adding value will put us over m_MaxCapacity. + // We are doing the check here to prevent the corruption of the StringBuilder. + int newLength = Length + value.Length; + if (newLength > m_MaxCapacity || newLength < value.Length) + { + throw new ArgumentOutOfRangeException(nameof(value), SR.ArgumentOutOfRange_LengthGreaterThanCapacity); + } + if (value.Length != 0) { // This case is so common we want to optimize for it heavily. @@ -1347,7 +1357,7 @@ public StringBuilder Insert(int index, string? value) } #pragma warning disable CA1830 // Prefer strongly-typed Append and Insert method overloads on StringBuilder. No need to fix for the builder itself - public StringBuilder Insert(int index, bool value) => Insert(index, value.ToString()); + public StringBuilder Insert(int index, bool value) => Insert(index, value.ToString().AsSpan(), 1); #pragma warning restore CA1830 [CLSCompliant(false)] @@ -1431,7 +1441,7 @@ public StringBuilder Insert(int index, char[]? value, int startIndex, int charCo [CLSCompliant(false)] public StringBuilder Insert(int index, ulong value) => InsertSpanFormattable(index, value); - public StringBuilder Insert(int index, object? value) => (value == null) ? this : Insert(index, value.ToString()); + public StringBuilder Insert(int index, object? value) => (value == null) ? this : Insert(index, value.ToString().AsSpan(), 1); public StringBuilder Insert(int index, ReadOnlySpan value) { @@ -1455,10 +1465,12 @@ private StringBuilder InsertSpanFormattable(int index, T value) where T : ISp Span buffer = stackalloc char[256]; if (value.TryFormat(buffer, out int charsWritten, format: default, provider: null)) { - return Insert(index, buffer.Slice(0, charsWritten)); + // We don't use Insert(int, ReadOnlySpan) for exception compatibility; + // we want exceeding the maximum capacity to throw an OutOfMemoryException. + return Insert(index, buffer.Slice(0, charsWritten), 1); } - return Insert(index, value.ToString()); + return Insert(index, value.ToString().AsSpan(), 1); } public StringBuilder AppendFormat(string format, object? arg0) => AppendFormatHelper(null, format, new ParamsArray(arg0)); @@ -2062,14 +2074,6 @@ public unsafe StringBuilder Append(char* value, int valueCount) throw new ArgumentOutOfRangeException(nameof(valueCount), SR.ArgumentOutOfRange_NegativeCount); } - // this is where we can check if the valueCount will put us over m_MaxCapacity - // We are doing the check here to prevent the corruption of the StringBuilder. - int newLength = Length + valueCount; - if (newLength > m_MaxCapacity || newLength < valueCount) - { - throw new ArgumentOutOfRangeException(nameof(valueCount), SR.ArgumentOutOfRange_LengthGreaterThanCapacity); - } - Append(new ReadOnlySpan(value, valueCount)); return this; } diff --git a/src/libraries/System.Runtime/tests/System/Text/StringBuilderTests.cs b/src/libraries/System.Runtime/tests/System/Text/StringBuilderTests.cs index 89bf734808fe28..9899d3a572f644 100644 --- a/src/libraries/System.Runtime/tests/System/Text/StringBuilderTests.cs +++ b/src/libraries/System.Runtime/tests/System/Text/StringBuilderTests.cs @@ -14,7 +14,7 @@ namespace System.Text.Tests public partial class StringBuilderTests { private static readonly string s_chunkSplitSource = new string('a', 30); - private static readonly string s_noCapacityParamName = "valueCount"; + private static readonly string s_noCapacityParamName = "value"; private static StringBuilder StringBuilderWithMultipleChunks() => new StringBuilder(20).Append(s_chunkSplitSource); From 16488d52b7e266c39eb5a9b26ce627825b679444 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Fri, 10 Sep 2021 10:34:54 +0300 Subject: [PATCH 08/12] Use ReadOnlySpan.TryCopyTo in one occasion, saving a branch. --- .../System.Private.CoreLib/src/System/Text/StringBuilder.cs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs b/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs index 8e976589ed9e8e..bf2bcd89f53367 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs @@ -1188,11 +1188,9 @@ public StringBuilder Append(ReadOnlySpan value) if (value.Length != 0) { // This case is so common we want to optimize for it heavily. - int newIndex = value.Length + m_ChunkLength; - if (newIndex <= m_ChunkChars.Length) + if (value.TryCopyTo(m_ChunkChars.AsSpan(m_ChunkLength))) { - value.CopyTo(m_ChunkChars.AsSpan(m_ChunkLength)); - m_ChunkLength = newIndex; + m_ChunkLength += value.Length; } else { From 2d79d36907ee096413e78bb708dbff8903383b6d Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Fri, 10 Sep 2021 13:00:56 +0300 Subject: [PATCH 09/12] Fix the last failing test. --- .../System.Runtime/tests/System/Text/StringBuilderTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Runtime/tests/System/Text/StringBuilderTests.cs b/src/libraries/System.Runtime/tests/System/Text/StringBuilderTests.cs index 9899d3a572f644..00ce83dda42d8b 100644 --- a/src/libraries/System.Runtime/tests/System/Text/StringBuilderTests.cs +++ b/src/libraries/System.Runtime/tests/System/Text/StringBuilderTests.cs @@ -703,8 +703,8 @@ public static void Append_CharArray_Invalid() AssertExtensions.Throws("charCount", () => builder.Append(new char[5], 6, 0)); // Start index + count > value.Length AssertExtensions.Throws("charCount", () => builder.Append(new char[5], 5, 1)); // Start index + count > value.Length - AssertExtensions.Throws("valueCount", () => builder.Append(new char[] { 'a' })); // New length > builder.MaxCapacity - AssertExtensions.Throws("valueCount", () => builder.Append(new char[] { 'a' }, 0, 1)); // New length > builder.MaxCapacity + AssertExtensions.Throws(s_noCapacityParamName, () => builder.Append(new char[] { 'a' })); // New length > builder.MaxCapacity + AssertExtensions.Throws(s_noCapacityParamName, () => builder.Append(new char[] { 'a' }, 0, 1)); // New length > builder.MaxCapacity } public static IEnumerable AppendFormat_TestData() From b9f886e960ffaca276e80984f606f4210de721a6 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Mon, 13 Sep 2021 16:59:04 +0300 Subject: [PATCH 10/12] Remove all explicit usages of the this keyword. --- .../src/System/Text/StringBuilder.cs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs b/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs index bf2bcd89f53367..18df63c31d431f 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs @@ -265,7 +265,7 @@ private void AssertInvariants() Debug.Assert(m_ChunkOffset + m_ChunkChars.Length >= m_ChunkOffset, "The length of the string is greater than int.MaxValue."); StringBuilder currentBlock = this; - int maxCapacity = this.m_MaxCapacity; + int maxCapacity = m_MaxCapacity; while (true) { // All blocks have the same max capacity. @@ -388,7 +388,7 @@ ref MemoryMarshal.GetArrayDataReference(sourceArray), /// The number of characters to read in this builder. public string ToString(int startIndex, int length) { - int currentLength = this.Length; + int currentLength = Length; if (startIndex < 0) { throw new ArgumentOutOfRangeException(nameof(startIndex), SR.ArgumentOutOfRange_StartIndex); @@ -408,13 +408,13 @@ public string ToString(int startIndex, int length) AssertInvariants(); string result = string.FastAllocateString(length); - this.CopyTo(startIndex, new Span(ref result.GetRawStringData(), length), length); + CopyTo(startIndex, new Span(ref result.GetRawStringData(), length), length); return result; } public StringBuilder Clear() { - this.Length = 0; + Length = 0; return this; } @@ -1037,11 +1037,11 @@ private StringBuilder Insert(int index, ReadOnlySpan value, int count) // Ensure we don't insert more chars than we can hold, and we don't // have any integer overflow in our new length. long insertingChars = (long)value.Length * count; - if (insertingChars > MaxCapacity - this.Length) + if (insertingChars > MaxCapacity - currentLength) { throw new OutOfMemoryException(); } - Debug.Assert(insertingChars + this.Length < int.MaxValue); + Debug.Assert(insertingChars + currentLength < int.MaxValue); MakeRoom(index, (int)insertingChars, out StringBuilder chunk, out int indexInChunk, false); while (count > 0) From 9c09d44a499b74e369dc084ea6caeb2b8e2cc6b7 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Mon, 13 Sep 2021 17:12:47 +0300 Subject: [PATCH 11/12] Address PR feedback. --- .../src/System/Text/StringBuilder.cs | 135 ++++++------------ 1 file changed, 41 insertions(+), 94 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs b/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs index 18df63c31d431f..44bf0dfb0fd037 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs @@ -259,7 +259,7 @@ void ISerializable.GetObjectData(SerializationInfo info, StreamingContext contex info.AddValue(ThreadIDField, 0); } - [System.Diagnostics.Conditional("DEBUG")] + [Conditional("DEBUG")] private void AssertInvariants() { Debug.Assert(m_ChunkOffset + m_ChunkChars.Length >= m_ChunkOffset, "The length of the string is greater than int.MaxValue."); @@ -1166,53 +1166,43 @@ internal StringBuilder AppendSpanFormattable(T value, string? format, IFormat public StringBuilder Append(object? value) => (value == null) ? this : Append(value.ToString()); - public StringBuilder Append(char[]? value) - { - if (value?.Length > 0) - { - Append(value.AsSpan()); - } - return this; - } + public StringBuilder Append(char[]? value) => Append(value.AsSpan()); public StringBuilder Append(ReadOnlySpan value) { - // This is where we can check if adding value will put us over m_MaxCapacity. - // We are doing the check here to prevent the corruption of the StringBuilder. - int newLength = Length + value.Length; - if (newLength > m_MaxCapacity || newLength < value.Length) + // This case is so common we want to optimize for it heavily. + if (value.TryCopyTo(m_ChunkChars.AsSpan(m_ChunkLength))) { - throw new ArgumentOutOfRangeException(nameof(value), SR.ArgumentOutOfRange_LengthGreaterThanCapacity); + m_ChunkLength += value.Length; } - - if (value.Length != 0) + else { - // This case is so common we want to optimize for it heavily. - if (value.TryCopyTo(m_ChunkChars.AsSpan(m_ChunkLength))) + // This is where we can check if adding value will put us over m_MaxCapacity. + // We are doing the check here to prevent the corruption of the StringBuilder. + int newLength = Length + value.Length; + if (newLength > m_MaxCapacity || newLength < value.Length) { - m_ChunkLength += value.Length; + throw new ArgumentOutOfRangeException(nameof(value), SR.ArgumentOutOfRange_LengthGreaterThanCapacity); } - else + + // Copy the first chunk + int firstLength = m_ChunkChars.Length - m_ChunkLength; + if (firstLength > 0) { - // Copy the first chunk - int firstLength = m_ChunkChars.Length - m_ChunkLength; - if (firstLength > 0) - { - value.Slice(0, firstLength).CopyTo(m_ChunkChars.AsSpan(m_ChunkLength)); - m_ChunkLength = m_ChunkChars.Length; - } + value.Slice(0, firstLength).CopyTo(m_ChunkChars.AsSpan(m_ChunkLength)); + m_ChunkLength = m_ChunkChars.Length; + } - // Expand the builder to add another chunk. - int restLength = value.Length - firstLength; - ExpandByABlock(restLength); - Debug.Assert(m_ChunkLength == 0, "A new block was not created."); + // Expand the builder to add another chunk. + int restLength = value.Length - firstLength; + ExpandByABlock(restLength); + Debug.Assert(m_ChunkLength == 0, "A new block was not created."); - // Copy the second chunk - value.Slice(firstLength).CopyTo(m_ChunkChars); - m_ChunkLength = restLength; - } - AssertInvariants(); + // Copy the second chunk + value.Slice(firstLength).CopyTo(m_ChunkChars); + m_ChunkLength = restLength; } + AssertInvariants(); return this; } @@ -1242,38 +1232,23 @@ public StringBuilder Append(ReadOnlySpan value) #region AppendJoin - public StringBuilder AppendJoin(string? separator, params object?[] values) - { - separator ??= string.Empty; - return AppendJoinCore(separator.AsSpan(), values); - } + public StringBuilder AppendJoin(string? separator, params object?[] values) => + AppendJoinCore(separator.AsSpan(), values); - public StringBuilder AppendJoin(string? separator, IEnumerable values) - { - separator ??= string.Empty; - return AppendJoinCore(separator.AsSpan(), values); - } + public StringBuilder AppendJoin(string? separator, IEnumerable values) => + AppendJoinCore(separator.AsSpan(), values); - public StringBuilder AppendJoin(string? separator, params string?[] values) - { - separator ??= string.Empty; - return AppendJoinCore(separator.AsSpan(), values); - } + public StringBuilder AppendJoin(string? separator, params string?[] values) => + AppendJoinCore(separator.AsSpan(), values); - public StringBuilder AppendJoin(char separator, params object?[] values) - { - return AppendJoinCore(new ReadOnlySpan(ref separator, 1), values); - } + public StringBuilder AppendJoin(char separator, params object?[] values) => + AppendJoinCore(new ReadOnlySpan(ref separator, 1), values); - public StringBuilder AppendJoin(char separator, IEnumerable values) - { - return AppendJoinCore(new ReadOnlySpan(ref separator, 1), values); - } + public StringBuilder AppendJoin(char separator, IEnumerable values) => + AppendJoinCore(new ReadOnlySpan(ref separator, 1), values); - public StringBuilder AppendJoin(char separator, params string?[] values) - { - return AppendJoinCore(new ReadOnlySpan(ref separator, 1), values); - } + public StringBuilder AppendJoin(char separator, params string?[] values) => + AppendJoinCore(new ReadOnlySpan(ref separator, 1), values); private StringBuilder AppendJoinCore(ReadOnlySpan separator, IEnumerable values) { @@ -1340,19 +1315,7 @@ private StringBuilder AppendJoinCore(ReadOnlySpan separator, T[] values #endregion - public StringBuilder Insert(int index, string? value) - { - if ((uint)index > (uint)Length) - { - throw new ArgumentOutOfRangeException(nameof(index), SR.ArgumentOutOfRange_Index); - } - - if (value != null) - { - Insert(index, value.AsSpan()); - } - return this; - } + public StringBuilder Insert(int index, string? value) => Insert(index, value.AsSpan()); #pragma warning disable CA1830 // Prefer strongly-typed Append and Insert method overloads on StringBuilder. No need to fix for the builder itself public StringBuilder Insert(int index, bool value) => Insert(index, value.ToString().AsSpan(), 1); @@ -1367,19 +1330,7 @@ public StringBuilder Insert(int index, string? value) public StringBuilder Insert(int index, char value) => Insert(index, new ReadOnlySpan(ref value, 1)); - public StringBuilder Insert(int index, char[]? value) - { - if ((uint)index > (uint)Length) - { - throw new ArgumentOutOfRangeException(nameof(index), SR.ArgumentOutOfRange_Index); - } - - if (value != null) - { - Insert(index, value.AsSpan()); - } - return this; - } + public StringBuilder Insert(int index, char[]? value) => Insert(index, value.AsSpan()); public StringBuilder Insert(int index, char[]? value, int startIndex, int charCount) { @@ -1413,11 +1364,7 @@ public StringBuilder Insert(int index, char[]? value, int startIndex, int charCo throw new ArgumentOutOfRangeException(nameof(startIndex), SR.ArgumentOutOfRange_Index); } - if (charCount > 0) - { - Insert(index, value.AsSpan(startIndex, charCount)); - } - return this; + return Insert(index, value.AsSpan(startIndex, charCount)); } public StringBuilder Insert(int index, int value) => InsertSpanFormattable(index, value); From 3cbbc8291a3d6895886e9082fd28ceb139c5c585 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Mon, 13 Sep 2021 17:16:45 +0300 Subject: [PATCH 12/12] Use ReadOnlySpan.IsEmpty uniformly. --- .../System.Private.CoreLib/src/System/Text/StringBuilder.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs b/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs index 44bf0dfb0fd037..f17d91501e1d8c 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs @@ -1395,7 +1395,7 @@ public StringBuilder Insert(int index, ReadOnlySpan value) throw new ArgumentOutOfRangeException(nameof(index), SR.ArgumentOutOfRange_Index); } - if (value.Length > 0) + if (!value.IsEmpty) { MakeRoom(index, value.Length, out StringBuilder chunk, out int indexInChunk, false); ReplaceInPlaceAtChunk(ref chunk!, ref indexInChunk, value); @@ -1677,7 +1677,7 @@ internal StringBuilder AppendFormatHelper(IFormatProvider? provider, string form if (cf != null) { - if (itemFormatSpan.Length != 0) + if (!itemFormatSpan.IsEmpty) { itemFormat = new string(itemFormatSpan); } @@ -1716,7 +1716,7 @@ internal StringBuilder AppendFormatHelper(IFormatProvider? provider, string form // Otherwise, fallback to trying IFormattable or calling ToString. if (arg is IFormattable formattableArg) { - if (itemFormatSpan.Length != 0) + if (!itemFormatSpan.IsEmpty) { itemFormat ??= new string(itemFormatSpan); }