Optimize StringBuilder by embracing spans and ISpanFormattable more.

[teo-tsirpanis]

This PR changes the implementation of StringBuilder to completely eliminate pinning by moving all pointer-based implementations to spans.

For example, the Append overload that takes a ReadOnlySpan of characters used to be implemented by pinning the span and forwarding to the Append overload that takes a pointer and a length. Now, the roles are reversed: the pointer overload calls the ReadOnlySpan overload, which does the real work, and to which other overloads of Append are now forwarded as well. Something similar also happened with the Insert family of methods.

Speaking of Insert, the overloads that take a primitive were optimized to take advantage of the ISpanFormattable interface, eliminating a temporary string allocation most of the time.

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

[GrabYourPitchforks]

A note on AllocateUninitializedArray usage: this could end up exposing uninitialized memory through the StringBuilder instance if multiple threads begin operating simultaneously on the instance and corrupt its internal state. While this is obviously an invalid operation, it is an observable behavioral change from the previous iteration, which would only ever expose zero-inited memory.

See also #47186.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

---

This PR changes the implementation of StringBuilder to completely eliminate pinning by moving all pointer-based implementations to spans.

For example, the Append overload that takes a ReadOnlySpan of characters used to be implemented by pinning the span and forwarding to the Append overload that takes a pointer and a length. Now, the roles are reversed: the pointer overload calls the ReadOnlySpan overload, which does the real work, and to which other overloads of Append are now forwarded as well. Something similar also happened with the Insert family of methods.

Speaking of Insert, the overloads that take a primitive were optimized to take advantage of the ISpanFormattable interface, eliminating a temporary string allocation most of the time.

Author:	teo-tsirpanis
Assignees:	-
Labels:	area-System.Runtime, tenet-performance, community-contribution
Milestone:	-

[teo-tsirpanis]

OK @GrabYourPitchforks, I reverted it.

[danmoseley]

@teo-tsirpanis could you please check StringBuilder has decent coverage in dotnet/performance and use the instructions in that repo to generate a before/after table?

[teo-tsirpanis]

and use the instructions in that repo to generate a before/after table

Unfortunately I can't do that; when I had tried to build the runtime, my laptop ran out of space.

I will however take a look at the StringBuilder benchmarks soon.

[teo-tsirpanis]

Some tests are failing. I will investigate it soon.

[danmoseley]

The only relevant failure seems to be Append_Bool_NoSpareCapacity_ThrowsArgumentOutOfRangeException where the parameter name changed. We're usually OK changing the parameter name if there's a reason.

[teo-tsirpanis]

Feedback was addressed.

[danmoseley]

@stephentoub do you have further feedback?

[stephentoub]

Optimize StringBuilder

The changes LGTM, but can you share before/after numbers from running https://github.com/dotnet/performance/blob/main/src/benchmarks/micro/libraries/System.Text/Perf.StringBuilder.cs ?

[teo-tsirpanis]

Unfortunately I can't provide benchmarks, as explained earlier. I even tried copy-pasting StringBuilder's source file to the benchmark's project but it uses too many internal corelib constructs.

[stephentoub]

Unfortunately I can't provide benchmarks, as explained earlier

Ok

@stephentoub do you have further feedback?

@danmoseley, we need benchmarks validated before this can be merged.

[tannergooding]

You should be able to run the existing benchmarks if you do:

.\build.cmd -subset clr+libs -config Release
.\src\tests\build.cmd x64 release generatelayoutonly

And then in the performance repo (not 100% positive the filter is correct below):

dotnet run -c Release -f net6.0 --filter "System.Text.Tests.Perf_StringBuilder" --coreRun <path-to-runtime-repo>\artifacts\tests\coreclr\windows.x64.Release\Tests\Core_Root\CoreRun.exe

You can also do it if you build the libs.tests and following the path and other info given here: https://github.com/dotnet/performance/blob/main/docs/benchmarking-workflow-dotnet-runtime.md

[teo-tsirpanis]

Sorry, still can't do it. My machine's SSD runs out of space when I try to build the runtime. I tried to do it in a Codespace but still ran into some issues.

[teo-tsirpanis]

Greetings from my new and bigger SSD.

I run the benchmarks (before and after) and it regressed? 😲

The most extreme example is Append_Span which went from 19.48 nanoseconds to 131.18.

[stephentoub]

@teo-tsirpanis, I think much of this PR was obsoleted by #64405. Shall we close it and you can open a new one focusing on just the portions you think are still relevant? Thanks!

[teo-tsirpanis]

OK, your solution seems better, great work. I will open a new PR soon.