-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Optimize StringBuilder by embracing spans and ISpanFormattable more.
#58907
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
Conversation
|
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. |
|
A note on See also #47186. |
|
Tagging subscribers to this area: @dotnet/area-system-runtime Issue DetailsThis PR changes the implementation of For example, the Speaking of
|
And streamline the character array Insert overload.
acbbb00 to
85bcf34
Compare
|
OK @GrabYourPitchforks, I reverted it. |
|
@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? |
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 |
|
Some tests are failing. I will investigate it soon. |
|
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. |
And move the capacity check in Append(ReadOnlySpan<char>).
10efdae to
16488d5
Compare
src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs
Outdated
Show resolved
Hide resolved
|
Feedback was addressed. |
|
@stephentoub do you have further feedback? |
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 ? |
|
Unfortunately I can't provide benchmarks, as explained earlier. I even tried copy-pasting |
Ok
@danmoseley, we need benchmarks validated before this can be merged. |
|
You should be able to run the existing benchmarks if you do: And then in the performance repo (not 100% positive the filter is correct below): You can also do it if you build the |
|
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, 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! |
|
OK, your solution seems better, great work. I will open a new PR soon. |
This PR changes the implementation of
StringBuilderto completely eliminate pinning by moving all pointer-based implementations to spans.For example, the
Appendoverload that takes aReadOnlySpanof characters used to be implemented by pinning the span and forwarding to theAppendoverload that takes a pointer and a length. Now, the roles are reversed: the pointer overload calls theReadOnlySpanoverload, which does the real work, and to which other overloads ofAppendare now forwarded as well. Something similar also happened with theInsertfamily of methods.Speaking of
Insert, the overloads that take a primitive were optimized to take advantage of theISpanFormattableinterface, eliminating a temporary string allocation most of the time.