Skip to content

Conversation

@teo-tsirpanis
Copy link
Contributor

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.

@ghost
Copy link

ghost commented Sep 9, 2021

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.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Sep 9, 2021
@GrabYourPitchforks
Copy link
Member

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
Copy link

ghost commented Sep 9, 2021

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: -

And streamline the character array Insert overload.
@teo-tsirpanis
Copy link
Contributor Author

OK @GrabYourPitchforks, I reverted it.

@danmoseley
Copy link
Member

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

Some tests are failing. I will investigate it soon.

@danmoseley
Copy link
Member

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
Copy link
Contributor Author

Feedback was addressed.

@danmoseley
Copy link
Member

@stephentoub do you have further feedback?

@stephentoub
Copy link
Member

@teo-tsirpanis
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

tannergooding commented Oct 6, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

@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
Copy link
Contributor Author

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

@teo-tsirpanis teo-tsirpanis deleted the stringbuilder-span branch February 7, 2022 18:17
@ghost ghost locked as resolved and limited conversation to collaborators Mar 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Runtime community-contribution Indicates that the PR has been added by a community member tenet-performance Performance related issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants