Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Use ValueStringBuilder in preference to StringBuilderCache #26934

Closed
wants to merge 7 commits into from
Closed

Use ValueStringBuilder in preference to StringBuilderCache #26934

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3d6a7a3
Use StringBuilderCache for Exceptions
benaadams Sep 28, 2019
861e503
Feedback
benaadams Sep 28, 2019
8a93a3f
Use ValueStringBuilder instead
benaadams Sep 28, 2019
d2be6bc
Add .AppendFormat to ValueStringBuilder
benaadams Sep 28, 2019
f0c2a71
Use ValueStringBuilder in preference to StringBuilderCache
benaadams Sep 29, 2019
3ac1100
Increase path to 260
benaadams Sep 29, 2019
b232228
Adjust naullablitiy
benaadams Sep 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/System.Private.CoreLib/shared/System.Private.CoreLib.Shared.projitems
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -822,6 +822,7 @@
<Compile Include="$(MSBuildThisFileDirectory)System\Text\UTF7Encoding.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Text\UTF8Encoding.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Text\ValueStringBuilder.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Text\ValueStringBuilder.AppendFormat.cs" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove StringBuilderCache.cs from corelib as well, or are there still other uses?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's still a few other uses; they are more problematic ones as one method gets the cached string builder, then it returns it, passes to other methods, then releases it later. Didn't tackle them as it would over-complicate the review.

<Compile Include="$(MSBuildThisFileDirectory)System\Text\Unicode\Utf16Utility.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Text\Unicode\Utf16Utility.Validation.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Text\Unicode\Utf8.cs" />
Expand Down
4 changes: 2 additions & 2 deletions src/System.Private.CoreLib/shared/System/AggregateException.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,7 +415,7 @@ public override string Message
return base.Message;
}

StringBuilder sb = StringBuilderCache.Acquire();
ValueStringBuilder sb = new ValueStringBuilder();
sb.Append(base.Message);
sb.Append(' ');
for (int i = 0; i < m_innerExceptions.Count; i++)
Expand All @@ -425,7 +425,7 @@ public override string Message
sb.Append(") ");
}
sb.Length--;
return StringBuilderCache.GetStringAndRelease(sb);
return sb.ToString();
}
}

Expand Down
10 changes: 6 additions & 4 deletions src/System.Private.CoreLib/shared/System/Collections/Generic/KeyValuePair.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,24 +21,26 @@ public static KeyValuePair<TKey, TValue> Create<TKey, TValue>(TKey key, TValue v
/// </summary>
internal static string PairToString(object? key, object? value)
{
StringBuilder s = StringBuilderCache.Acquire();
Span<char> initialBuffer = stackalloc char[64];
var s = new ValueStringBuilder(initialBuffer);

s.Append('[');

if (key != null)
{
s.Append(key);
s.Append(key.ToString());
}

s.Append(", ");

if (value != null)
{
s.Append(value);
s.Append(value.ToString());
}

s.Append(']');

return StringBuilderCache.GetStringAndRelease(s);
return s.ToString();
}
}

Expand Down
4 changes: 1 addition & 3 deletions src/System.Private.CoreLib/shared/System/Diagnostics/StackTrace.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,9 +334,7 @@ internal string ToString(TraceFormat traceFormat)
if (traceFormat == TraceFormat.TrailingNewLine)
sb.Append(Environment.NewLine);

string returnValue = sb.ToString();
sb.Dispose();
return returnValue;
return sb.ToString();
}
#endif // !CORERT

Expand Down
4 changes: 1 addition & 3 deletions src/System.Private.CoreLib/shared/System/Exception.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,9 +165,7 @@ public override string ToString()
sb.Append(stackTrace);
}

string returnValue = sb.ToString();
sb.Dispose();
return returnValue;
return sb.ToString();
}

protected event EventHandler<SafeSerializationEventArgs>? SerializeObjectState
Expand Down
8 changes: 5 additions & 3 deletions src/System.Private.CoreLib/shared/System/IO/Path.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -861,7 +861,9 @@ private static string GetRelativePath(string relativeTo, string path, StringComp
// C:\Foo\Bar C:\Bar\Bar L3, S2 -> ..\..\Bar\Bar
// C:\Foo\Foo C:\Foo\Bar L7, S1 -> ..\Bar

StringBuilder sb = StringBuilderCache.Acquire(Math.Max(relativeTo.Length, path.Length));
Span<char> initialBuffer = stackalloc char[64];
benaadams marked this conversation as resolved.
Show resolved Hide resolved
var sb = new ValueStringBuilder(initialBuffer);
sb.EnsureCapacity(Math.Max(relativeTo.Length, path.Length));

// Add parent segments for segments past the common on the "from" path
if (commonLength < relativeToLength)
Expand Down Expand Up @@ -896,10 +898,10 @@ private static string GetRelativePath(string relativeTo, string path, StringComp
sb.Append(DirectorySeparatorChar);
}

sb.Append(path, commonLength, differenceLength);
sb.Append(path.AsSpan(commonLength, differenceLength));
}

return StringBuilderCache.GetStringAndRelease(sb);
return sb.ToString();
}

/// <summary>Returns a comparison that can be used to compare file and directory names for equality.</summary>
Expand Down
15 changes: 8 additions & 7 deletions src/System.Private.CoreLib/shared/System/Runtime/Intrinsics/Vector128_1.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,20 +173,21 @@ public override string ToString()
ThrowHelper.ThrowForUnsupportedVectorBaseType<T>();

int lastElement = Count - 1;
StringBuilder sb = StringBuilderCache.Acquire();
Span<char> initialBuffer = stackalloc char[64];
var sb = new ValueStringBuilder(initialBuffer);
CultureInfo invariant = CultureInfo.InvariantCulture;

sb.Append('<');
for (int i = 0; i < lastElement; i++)
{
sb.Append(((IFormattable)this.GetElement(i)).ToString("G", invariant))
.Append(',')
.Append(' ');
sb.Append(((IFormattable)this.GetElement(i)).ToString("G", invariant));
sb.Append(',');
sb.Append(' ');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question/comment as earlier... I wonder if sb.Append(", "); would be better, just for simplicity reasons.

}
sb.Append(((IFormattable)this.GetElement(lastElement)).ToString("G", invariant))
.Append('>');
sb.Append(((IFormattable)this.GetElement(lastElement)).ToString("G", invariant));
sb.Append('>');

return StringBuilderCache.GetStringAndRelease(sb);
return sb.ToString();
}
}
}
15 changes: 8 additions & 7 deletions src/System.Private.CoreLib/shared/System/Runtime/Intrinsics/Vector256_1.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,20 +174,21 @@ public override string ToString()
ThrowHelper.ThrowForUnsupportedVectorBaseType<T>();

int lastElement = Count - 1;
StringBuilder sb = StringBuilderCache.Acquire();
Span<char> initialBuffer = stackalloc char[64];
var sb = new ValueStringBuilder(initialBuffer);
CultureInfo invariant = CultureInfo.InvariantCulture;

sb.Append('<');
for (int i = 0; i < lastElement; i++)
{
sb.Append(((IFormattable)this.GetElement(i)).ToString("G", invariant))
.Append(',')
.Append(' ');
sb.Append(((IFormattable)this.GetElement(i)).ToString("G", invariant));
sb.Append(',');
sb.Append(' ');
}
sb.Append(((IFormattable)this.GetElement(lastElement)).ToString("G", invariant))
.Append('>');
sb.Append(((IFormattable)this.GetElement(lastElement)).ToString("G", invariant));
sb.Append('>');

return StringBuilderCache.GetStringAndRelease(sb);
return sb.ToString();
}
}
}
15 changes: 8 additions & 7 deletions src/System.Private.CoreLib/shared/System/Runtime/Intrinsics/Vector64_1.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,20 +127,21 @@ public override string ToString()
ThrowHelper.ThrowForUnsupportedVectorBaseType<T>();

int lastElement = Count - 1;
StringBuilder sb = StringBuilderCache.Acquire();
Span<char> initialBuffer = stackalloc char[64];
var sb = new ValueStringBuilder(initialBuffer);
CultureInfo invariant = CultureInfo.InvariantCulture;

sb.Append('<');
for (int i = 0; i < lastElement; i++)
{
sb.Append(((IFormattable)this.GetElement(i)).ToString("G", invariant))
.Append(',')
.Append(' ');
sb.Append(((IFormattable)this.GetElement(i)).ToString("G", invariant));
sb.Append(',');
sb.Append(' ');
}
sb.Append(((IFormattable)this.GetElement(lastElement)).ToString("G", invariant))
.Append('>');
sb.Append(((IFormattable)this.GetElement(lastElement)).ToString("G", invariant));
sb.Append('>');

return StringBuilderCache.GetStringAndRelease(sb);
return sb.ToString();
}
}
}
53 changes: 32 additions & 21 deletions src/System.Private.CoreLib/shared/System/String.Manipulation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,15 +159,16 @@ public static string Concat<T>(IEnumerable<T> values)

// Create the StringBuilder, add the chars we've already enumerated,
// add the rest, and then get the resulting string.
StringBuilder result = StringBuilderCache.Acquire();
Span<char> initialBuffer = stackalloc char[64];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The stack reserved sizes in this file can be probably be a bit higher, like 128 chars.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've been fairly loose / non-prescriptive with how we do stackallocs like this in the past. In various places around corelib, we use various limits for stackalloc char when we don't know the actual upper limit, including 256, 128, 64, and 32 (lots of other sizes are used of course when we do have an upper bound). Not as part of this change, but it seems we should probably standardize with some const somewhere that everyone just uses.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that a general reasonable sized buffer constant would be useful.

We often know the typical maximum size, e.g. 32-chars for a number formatting. I assume that we would keep these scenario specific maximum sizes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that we would keep these scenario specific maximum sizes.

Yup.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might also want to harmonize with the jit's threshold for converting fixed sized locallocs to locals, which is currently 32. Back when I set this I did not see many > 32 cases.

Copy link
Member

@gfoidl gfoidl Sep 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we have char, so that threshold would be 16 (= 32 bytes)?

var result = new ValueStringBuilder(initialBuffer);
result.Append(c); // first value
do
{
c = en.Current;
result.Append(c);
}
while (en.MoveNext());
return StringBuilderCache.GetStringAndRelease(result);
return result.ToString();
}
}
else
Expand Down Expand Up @@ -195,7 +196,8 @@ public static string Concat<T>(IEnumerable<T> values)
return firstString ?? string.Empty;
}

StringBuilder result = StringBuilderCache.Acquire();
Span<char> initialBuffer = stackalloc char[64];
var result = new ValueStringBuilder(initialBuffer);

result.Append(firstString);

Expand All @@ -210,7 +212,7 @@ public static string Concat<T>(IEnumerable<T> values)
}
while (en.MoveNext());

return StringBuilderCache.GetStringAndRelease(result);
return result.ToString();
}
}
}
Expand All @@ -232,7 +234,8 @@ public static string Concat(IEnumerable<string?> values)
return firstValue ?? string.Empty;
}

StringBuilder result = StringBuilderCache.Acquire();
Span<char> initialBuffer = stackalloc char[64];
var result = new ValueStringBuilder(initialBuffer);
result.Append(firstValue);

do
Expand All @@ -241,7 +244,7 @@ public static string Concat(IEnumerable<string?> values)
}
while (en.MoveNext());

return StringBuilderCache.GetStringAndRelease(result);
return result.ToString();
}
}

Expand Down Expand Up @@ -522,12 +525,13 @@ private static string FormatHelper(IFormatProvider? provider, string format, Par
if (format == null)
throw new ArgumentNullException(nameof(format));

ValueStringBuilder sb = new ValueStringBuilder(format.Length + args.Length * 8);
Span<char> initialBuffer = stackalloc char[64];
var sb = new ValueStringBuilder(initialBuffer);

sb.EnsureCapacity(format.Length + args.Length * 8);
sb.AppendFormatHelper(provider, format, args);

string returnValue = sb.ToString();
sb.Dispose();
return returnValue;
return sb.ToString();
}

public string Insert(int startIndex, string value)
Expand Down Expand Up @@ -648,7 +652,9 @@ public static string Join(string? separator, IEnumerable<string?> values)
}

// Null separator and values are handled by the StringBuilder
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per the other conversation we should probably change Append to accept nulls again. But if we don't, this comment will need to be updated.

StringBuilder result = StringBuilderCache.Acquire();
Span<char> initialBuffer = stackalloc char[64];
var result = new ValueStringBuilder(initialBuffer);

result.Append(firstValue);

do
Expand All @@ -658,7 +664,7 @@ public static string Join(string? separator, IEnumerable<string?> values)
}
while (en.MoveNext());

return StringBuilderCache.GetStringAndRelease(result);
return result.ToString();
}
}

Expand Down Expand Up @@ -693,7 +699,9 @@ private static unsafe string JoinCore(char* separator, int separatorLength, obje
return firstString ?? string.Empty;
}

StringBuilder result = StringBuilderCache.Acquire();
Span<char> initialBuffer = stackalloc char[64];
var result = new ValueStringBuilder(initialBuffer);

result.Append(firstString);

for (int i = 1; i < values.Length; i++)
Expand All @@ -706,7 +714,7 @@ private static unsafe string JoinCore(char* separator, int separatorLength, obje
}
}

return StringBuilderCache.GetStringAndRelease(result);
return result.ToString();
}

private static unsafe string JoinCore<T>(char* separator, int separatorLength, IEnumerable<T> values)
Expand Down Expand Up @@ -741,7 +749,8 @@ private static unsafe string JoinCore<T>(char* separator, int separatorLength, I
return firstString ?? string.Empty;
}

StringBuilder result = StringBuilderCache.Acquire();
Span<char> initialBuffer = stackalloc char[64];
var result = new ValueStringBuilder(initialBuffer);

result.Append(firstString);

Expand All @@ -757,7 +766,7 @@ private static unsafe string JoinCore<T>(char* separator, int separatorLength, I
}
while (en.MoveNext());

return StringBuilderCache.GetStringAndRelease(result);
return result.ToString();
}
}

Expand Down Expand Up @@ -1005,7 +1014,9 @@ private unsafe string ReplaceCore(string oldValue, string? newValue, CultureInfo
newValue ??= string.Empty;

CultureInfo referenceCulture = culture ?? CultureInfo.CurrentCulture;
StringBuilder result = StringBuilderCache.Acquire();
Span<char> initialBuffer = stackalloc char[64];
var result = new ValueStringBuilder(initialBuffer);
result.EnsureCapacity(this.Length);

int startIndex = 0;
int index = 0;
Expand All @@ -1021,7 +1032,7 @@ private unsafe string ReplaceCore(string oldValue, string? newValue, CultureInfo
if (index >= 0)
{
// append the unmodified portion of string
result.Append(this, startIndex, index - startIndex);
result.Append(this.AsSpan(startIndex, index - startIndex));

// append the replacement
result.Append(newValue);
Expand All @@ -1034,16 +1045,16 @@ private unsafe string ReplaceCore(string oldValue, string? newValue, CultureInfo
// small optimization,
// if we have not done any replacements,
// we will return the original string
StringBuilderCache.Release(result);
result.Dispose();
return this;
}
else
{
result.Append(this, startIndex, this.Length - startIndex);
result.Append(this.AsSpan(startIndex, this.Length - startIndex));
}
} while (index >= 0);

return StringBuilderCache.GetStringAndRelease(result);
return result.ToString();
}

// Replaces all instances of oldChar with newChar.
Expand Down
Loading