From 0945de5cf78b84b281d9053668c7a4716fe6ce62 Mon Sep 17 00:00:00 2001 From: yesmey Date: Wed, 10 May 2023 15:27:06 +0200 Subject: [PATCH 1/4] StringBuilder: use Span.Fill when adding repeating characters --- .../src/System/Text/StringBuilder.cs | 72 +++++++++++++------ 1 file changed, 50 insertions(+), 22 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 395e5094e6d5cd..e6a7840329b838 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs @@ -659,36 +659,55 @@ public StringBuilder Append(char value, int repeatCount) return this; } - // this is where we can check if the repeatCount will put us over m_MaxCapacity - // We are doing the check here to prevent the corruption of the StringBuilder. - int newLength = Length + repeatCount; - if (newLength > m_MaxCapacity || newLength < repeatCount) + char[] chunkChars = m_ChunkChars; + int chunkLength = m_ChunkLength; + + if (((uint)chunkLength + (uint)repeatCount) <= (uint)chunkChars.Length) { - throw new ArgumentOutOfRangeException(nameof(repeatCount), SR.ArgumentOutOfRange_LengthGreaterThanCapacity); + chunkChars.AsSpan(chunkLength, repeatCount).Fill(value); + m_ChunkLength += repeatCount; } - - int index = m_ChunkLength; - while (repeatCount > 0) + else { - if (index < m_ChunkChars.Length) - { - m_ChunkChars[index++] = value; - --repeatCount; - } - else - { - m_ChunkLength = index; - ExpandByABlock(repeatCount); - Debug.Assert(m_ChunkLength == 0); - index = 0; - } + AppendWithExpansion(value, repeatCount); } - m_ChunkLength = index; AssertInvariants(); return this; } + [MethodImpl(MethodImplOptions.NoInlining)] + private void AppendWithExpansion(char value, int repeatCount) + { + Debug.Assert(repeatCount > 0, "Invalid length; should have been validated by caller."); + + // Check if the repeatCount will put us over m_MaxCapacity + if ((uint)(repeatCount + Length) > (uint)m_MaxCapacity) + { + throw new ArgumentOutOfRangeException(nameof(repeatCount), SR.ArgumentOutOfRange_LengthGreaterThanCapacity); + } + + char[] chunkChars = m_ChunkChars; + int chunkLength = m_ChunkLength; + + // Fill the rest of the current chunk + int firstLength = chunkChars.Length - chunkLength; + if (firstLength > 0) + { + chunkChars.AsSpan(chunkLength, firstLength).Fill(value); + m_ChunkLength = chunkChars.Length; + } + + // Expand the builder to add another chunk + int restLength = repeatCount - firstLength; + ExpandByABlock(restLength); + Debug.Assert(m_ChunkLength == 0, "A new block was not created."); + + // Fill the new chunk with the remaining part of repeatCount + m_ChunkChars.AsSpan(0, restLength).Fill(value); + m_ChunkLength = restLength; + } + /// /// Appends a range of characters to the end of this builder. /// @@ -990,12 +1009,21 @@ public StringBuilder Append(char value) } else { - Append(value, 1); + AppendWithExpansion(value); } return this; } + [MethodImpl(MethodImplOptions.NoInlining)] + private void AppendWithExpansion(char value) + { + ExpandByABlock(1); + Debug.Assert(m_ChunkLength == 0, "A new block was not created."); + m_ChunkChars[0] = value; + m_ChunkLength++; + } + [CLSCompliant(false)] public StringBuilder Append(sbyte value) => AppendSpanFormattable(value); From 6acfd239ac9a01a703228916cef326a5ad281cda Mon Sep 17 00:00:00 2001 From: yesmey Date: Tue, 16 May 2023 07:26:07 +0200 Subject: [PATCH 2/4] Update test --- .../System.Runtime/tests/System/Text/StringBuilderTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Runtime/tests/System/Text/StringBuilderTests.cs b/src/libraries/System.Runtime/tests/System/Text/StringBuilderTests.cs index cbe6cda0093f85..655ea981fb46db 100644 --- a/src/libraries/System.Runtime/tests/System/Text/StringBuilderTests.cs +++ b/src/libraries/System.Runtime/tests/System/Text/StringBuilderTests.cs @@ -550,7 +550,7 @@ public static void Append_Char_NoSpareCapacity_ThrowsArgumentOutOfRangeException var builder = new StringBuilder(0, 5); builder.Append("Hello"); - AssertExtensions.Throws("repeatCount", "requiredLength", () => builder.Append('a')); + AssertExtensions.Throws("requiredLength", () => builder.Append('a')); AssertExtensions.Throws("repeatCount", "requiredLength", () => builder.Append('a', 1)); } From 7ee3ffabd342bf4ad69da81b5920bf2b0be4bbf4 Mon Sep 17 00:00:00 2001 From: yesmey Date: Sun, 28 May 2023 13:53:23 +0200 Subject: [PATCH 3/4] Use Span.Slice check --- .../System.Private.CoreLib/src/System/Text/StringBuilder.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) 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 e6a7840329b838..aeba0c49da3f15 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs @@ -662,7 +662,10 @@ public StringBuilder Append(char value, int repeatCount) char[] chunkChars = m_ChunkChars; int chunkLength = m_ChunkLength; - if (((uint)chunkLength + (uint)repeatCount) <= (uint)chunkChars.Length) + // Try to fit the whole repeatCount in the current chunk + // Use the same check as Span.Slice for 64-bit so it can be folded + // Since repeatCount can't be negative, there's no risk for it to overflow on 32 bit + if (((nuint)(uint)chunkLength + (nuint)(uint)repeatCount) <= (nuint)(uint)chunkChars.Length) { chunkChars.AsSpan(chunkLength, repeatCount).Fill(value); m_ChunkLength += repeatCount; From 8222f5768e913e2687696d5ee4e0d64e55283d48 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 7 Jul 2023 14:51:50 +0200 Subject: [PATCH 4/4] Apply suggestions from code review --- .../System.Private.CoreLib/src/System/Text/StringBuilder.cs | 1 - 1 file changed, 1 deletion(-) 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 aeba0c49da3f15..7a384f850e987b 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs @@ -679,7 +679,6 @@ public StringBuilder Append(char value, int repeatCount) return this; } - [MethodImpl(MethodImplOptions.NoInlining)] private void AppendWithExpansion(char value, int repeatCount) { Debug.Assert(repeatCount > 0, "Invalid length; should have been validated by caller.");