-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Refactor ValueStringBuilder #119599
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor ValueStringBuilder #119599
Conversation
|
How does this affect StringBuilder.AppendFormat performance? |
Despite the unintuitive behavior of TryCopyTo also calling Dispose, it’s only being used in a single place.
|
The benchmark results for
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using BenchmarkDotNet.Configs;
using System;
using System.Globalization;
using System.Runtime.InteropServices;
using System.Linq;
using System.Text;
[DisassemblyDiagnoser]
[MemoryDiagnoser(false)]
[HideColumns("Job", "Error", "StdDev", "Median", "RatioSD")]
[GroupBenchmarksBy(BenchmarkLogicalGroupRule.ByMethod)]
public class StringBuilderTest
{
object[] formatArgs;
string formatSmall, formatLarge;
StringBuilder sb;
[GlobalSetup]
public void Setup()
{
sb = new StringBuilder();
formatSmall = "Text={0}, DateTime={1:G}, Number={2}, NumberX={2:X}, EnumD={3:D}, EnumF={3:F}";
formatLarge = formatSmall + ", " + string.Join(", ", Enumerable.Repeat("DateTime={1:yyyyMMdd-HH:mm:ss}", 100));
formatArgs = ["Text", new DateTime(2025, 4, 5, 6, 7, 5), long.MinValue, DayOfWeek.Friday];
StringBuilderLarge(); // allocation
}
[Benchmark] public string ValueStringBuilderSmall() => string.Format(CultureInfo.InvariantCulture, formatSmall, formatArgs);
[Benchmark]
public StringBuilder StringBuilderSmall()
{
sb.Clear();
return sb.AppendFormat(CultureInfo.InvariantCulture, formatSmall, formatArgs);
}
[Benchmark] public string ValueStringBuilderLarge() => string.Format(CultureInfo.InvariantCulture, formatLarge, formatArgs);
[Benchmark]
public StringBuilder StringBuilderLarge()
{
sb.Clear();
return sb.AppendFormat(CultureInfo.InvariantCulture, formatLarge, formatArgs);
}
} |
|
@EgorBot -intel using BenchmarkDotNet.Attributes;
using System.Text;
public class Bench
{
StringBuilder sb = new StringBuilder(100);
[IterationSetup]
public void IterationSetup() => sb.Clear();
[Benchmark]
public void AppendFormatSimple() => sb.AppendFormat("{0}{1}{2}", 2, 3, 4);
} |
|
30+% regression in the simple micro-benchmark for StringBuilder.AppendFormat: EgorBot/runtime-utils#484 (comment) |
|
@jkotas I reverted the changes related to AppendFormat. I’ll look into whether this can be addressed separately. |
src/libraries/Common/src/System/Text/ValueStringBuilder.EnsureTerminated.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/System/Text/ValueStringBuilder.EnsureTerminated.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
Extracted the logic into
EnsureTerminated()This allowed removing the overloads of
AsSpan(terminate)andGetPinnableReference(terminate).Unified the implementation ofAppendFormatwith that ofStringBuilder.In .NET 9, generics can use ref structs.This is reverted due to observed performance regression.
Remove ValueStringBuilder.TryCopyTo.
Even though TryCopyTo has the unintuitive behavior of also calling Dispose, it’s only used in a single place—Number.BigInteger.cs.