Add StringBuilder Span-based APIs #13163

kevingosse · 2017-08-02T08:59:19Z

Contributes to https://github.com/dotnet/corefx/issues/22430

Add span-based Append/Insert/CopyTo methods to StringBuilder
Rewrote the existing CopyTo method into an overload of the span-based version, to reduce code duplication

dnfclas · 2017-08-02T08:59:23Z

@KooKiz,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla2.dotnetfoundation.org.

It will cover your contributions to all .NET Foundation-managed open source projects.
Thanks,
.NET Foundation Pull Request Bot

kevingosse · 2017-08-02T09:11:41Z

src/mscorlib/shared/System/Text/StringBuilder.cs

@@ -2001,6 +2043,22 @@ private static unsafe void ThreadSafeCopy(char* sourcePtr, char[] destination, i
            }
        }

+        private static unsafe void ThreadSafeCopy(char* sourcePtr, Span<char> destination, int destinationIndex, int count)


I'm tentative about those overloads, as they significantly increase code duplication. Still, I have yet to come with an elegant way to factorize them (short of making all codepaths going through the Span version, with a performance penalty). Suggestions are much appreciated

Do you know for sure that there is a performance penalty? If so, and Span is the culprit, we'd love to see examples.

My take was that there is necessarily some overhead, since we would add the conversion from array to span on top of the existing code. But it's true that this overhead is probably not significant in this context. I'll run some benchmark to make sure.

After spending more time on it, I realized that the ThreadSafeCopy overload was only used by CopyTo, so I don't actually have to impact the other codepaths

kevingosse · 2017-08-02T13:15:41Z

@dotnet-bot test this please

The char[] based one was called only by CopyTo so it wasn't needed anymore. Replaced by a Span based version

benaadams · 2017-08-02T23:54:23Z

src/mscorlib/shared/System/Text/StringBuilder.cs

        {
            if (count > 0)
            {
-                if ((uint)sourceIndex <= (uint)source.Length && (sourceIndex + count) <= source.Length)
+                if ((uint)sourceIndex > (uint)source.Length || (sourceIndex + count) > source.Length)


int overflow risk

(sourceIndex + count) > source.Length => count > (source.Length - sourceIndex)

benaadams · 2017-08-02T23:55:10Z

src/mscorlib/shared/System/Text/StringBuilder.cs

                }
-                else
+
+                if ((uint)destinationIndex > (uint)destination.Length || (destinationIndex + count) > destination.Length)


int overflow risk?

(destinationIndex + count) > destination.Length) => count > (destination.Length - destinationIndex)

FWIW, the same condition exists on the other ThreadSafeCopy overload (that I didn't touch), line 2034.
Still, I agree on the risk. Gonna do some further testing.

source.Length - destinationIndex

@benaadams, copy and paste error?

morganbr · 2017-08-03T20:39:10Z

src/mscorlib/shared/System/Text/StringBuilder.cs

+            {
+                unsafe
+                {
+                    fixed (char* valueChars = &value.DangerousGetPinnableReference())


If possible, I'd prefer to see this flipped -- that is it would be preferable to have the pointer overload create a span and pass that throughout the class. That realizes part of the purpose of spans -- avoiding memory corruption while still being fast. If you're not comfortable doing that in this change, I do understand.

Calls string.wstrcpy though which wants pointers; would need to use equivalent span function in string to that?

Since there's not currently interop support for Span (@yizhang82, any plans?), I agree there are places where pointers are still necessary. However, if we minimize those places, it mitigates cases like the int overflows you pointed out.

Could add an internal wstrcpy that takes Span and call that instead?

i.e. have the pointers in string rather than elsewhere

/cc @stephentoub

Let's keep such things separate. There are two distinct tracks for span in coreclr/corefx:

Using it internally to clean up duplicated code, to make existing unsafe code safer, to make it easier to stack allocate in places we're currently heap allocating, etc.

Adding new APIs that accept span

I'd like to largely keep those separate. If it's easy to achieve the former while doing the latter, great, or if the best way to implement the API is by changing existing implementation, ok, but doing so also makes it more likely we'll incur performance regressions, especially at this early stage where we haven't shaken out all of the possible perf issues with span. Any such changes need to be measured and validated rigorously.

stephentoub · 2017-08-03T21:10:48Z

src/mscorlib/shared/System/Text/StringBuilder.cs

-            if (count < 0)
-            {
-                throw new ArgumentOutOfRangeException(nameof(count), SR.Arg_NegativeArgCount);
-            }


Technically this a breaking change, as if multiple invalid inputs were provided, it'll change the order in which we check and thus will change what exceptions are thrown. That said, I personally think that's an acceptable level of break, but others should weigh in... @weshaggard?

I think the change is acceptable as well.

stephentoub · 2017-08-04T12:47:37Z

Thanks, @kookiz. Have you run the corefx tests against this change? Assuming yes, LGTM. If not, instructions for doing so are at https://github.com/dotnet/corefx/blob/7059f8b316723f5db64e4b1d28e2b1e8b137ab1e/Documentation/project-docs/developer-guide.md#testing-with-private-coreclr-bits.

kevingosse · 2017-08-04T13:59:15Z

@stephentoub I did, but it's interesting that you're asking. Isn't the continuous delivery supposed to automatically run the tests when I push a commit?

On an unrelated note, it looks like the cla-required tag wasn't removed even though I signed the agreement (as visible in dotnet/corefx#22852)

dnfclas · 2017-08-06T13:45:14Z

@kookiz, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, .NET Foundation Pull Request Bot

kevingosse · 2017-08-08T12:44:49Z

Is there something else I need to do for this PR to be merged?

jkotas · 2017-08-08T17:24:23Z

@dotnet-bot test Windows_NT x64 corefx_baseline

* Add StringBuilder Span-based APIs * Factorized ThreadSafeCopy methods The char[] based one was called only by CopyTo so it wasn't needed anymore. Replaced by a Span based version * Fix possible overflow Signed-off-by: dotnet-bot <[email protected]>

Add StringBuilder Span-based APIs

997f4e8

dnfclas added the cla-required label Aug 2, 2017

kevingosse mentioned this pull request Aug 2, 2017

Add StringBuilder Span-based APIs dotnet/corefx#22852

Merged

kevingosse commented Aug 2, 2017

View reviewed changes

Factorized ThreadSafeCopy methods

e86c365

The char[] based one was called only by CopyTo so it wasn't needed anymore. Replaced by a Span based version

benaadams suggested changes Aug 2, 2017

View reviewed changes

morganbr reviewed Aug 3, 2017

View reviewed changes

stephentoub reviewed Aug 3, 2017

View reviewed changes

Fix possible overflow

1e41719

dnfclas added cla-signed and removed cla-required labels Aug 6, 2017

stephentoub approved these changes Aug 8, 2017

View reviewed changes

jkotas approved these changes Aug 8, 2017

View reviewed changes

stephentoub merged commit c7a10a5 into dotnet:master Aug 8, 2017

kevingosse deleted the stringbuilder_span2 branch August 11, 2017 20:32

karelz modified the milestone: 2.1.0 Aug 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add StringBuilder Span-based APIs #13163

Add StringBuilder Span-based APIs #13163

kevingosse commented Aug 2, 2017

dnfclas commented Aug 2, 2017

kevingosse Aug 2, 2017

AndyAyersMS Aug 2, 2017

kevingosse Aug 2, 2017

kevingosse Aug 2, 2017

kevingosse commented Aug 2, 2017

benaadams Aug 2, 2017

benaadams Aug 2, 2017 •

edited

Loading

kevingosse Aug 3, 2017

stephentoub Aug 3, 2017

benaadams Aug 4, 2017

morganbr Aug 3, 2017

benaadams Aug 3, 2017

morganbr Aug 3, 2017

benaadams Aug 3, 2017 •

edited

Loading

stephentoub Aug 3, 2017 •

edited

Loading

stephentoub Aug 3, 2017

weshaggard Aug 4, 2017

stephentoub commented Aug 4, 2017

kevingosse commented Aug 4, 2017 •

edited

Loading

dnfclas commented Aug 6, 2017

kevingosse commented Aug 8, 2017

jkotas commented Aug 8, 2017

Add StringBuilder Span-based APIs #13163

Add StringBuilder Span-based APIs #13163

Conversation

kevingosse commented Aug 2, 2017

dnfclas commented Aug 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevingosse commented Aug 2, 2017

Choose a reason for hiding this comment

benaadams Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benaadams Aug 3, 2017 • edited Loading

Choose a reason for hiding this comment

stephentoub Aug 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub commented Aug 4, 2017

kevingosse commented Aug 4, 2017 • edited Loading

dnfclas commented Aug 6, 2017

kevingosse commented Aug 8, 2017

jkotas commented Aug 8, 2017

benaadams Aug 2, 2017 •

edited

Loading

benaadams Aug 3, 2017 •

edited

Loading

stephentoub Aug 3, 2017 •

edited

Loading

kevingosse commented Aug 4, 2017 •

edited

Loading