Expose StringBuilder.GetChunks() #29877

vancem · 2018-05-23T18:58:56Z

This allows efficient 'foreach' of the chunks in a StringBuilder.
Also added tests for the foreach.

This is the CoreFX part of of the CoreCLR PR: dotnet/coreclr#17530 which actually implemented the GetChunks() method. This change exposes it publicly and provides a test.

This addresses issue https://github.com/dotnet/corefx/issues/29770 where the API design was approved.

@danmosemsft @stephentoub

stephentoub · 2018-05-23T19:03:33Z

src/System.Runtime/ref/System.Runtime.cs

@@ -7621,6 +7621,16 @@ public sealed partial class StringBuilder : System.Runtime.Serialization.ISerial
        void System.Runtime.Serialization.ISerializable.GetObjectData(System.Runtime.Serialization.SerializationInfo info, System.Runtime.Serialization.StreamingContext context) { }
        public override string ToString() { throw null; }
        public string ToString(int startIndex, int length) { throw null; }
+
+        // foreach over ReadOnlyMemory<char> support (IEnumerable-like interface)


This comment can/should be deleted from the ref, as we're trying to get to a state where these files are easily machine re-generatable.

stephentoub · 2018-05-23T19:05:39Z

src/System.Runtime/tests/System/Text/StringBuilderTests.netcoreapp.cs

+                // Copy the string out (not using StringBuilder).  
+                string outStr = "";
+                foreach (ReadOnlyMemory<char> chunk in inBuilder.EnumerateChunks())
+                    outStr += new string(chunk.Span);


Do we want any validation for the behavior when the StringBuilder is modified while its chunks are being enumerated?

The behavior is undefined when a StringBuilder is modified when enumeration is in progress. I will put a warning about this in EnumerateChunks().

I had considered actively throwing if this is detected, but this DOES slow the main Append() path down (not pay for play), and the value here is very low because this foreach API is a pretty rare use case for use by sophisticated users.

weshaggard · 2018-05-24T00:02:05Z

src/System.Runtime/ref/System.Runtime.cs

@@ -7621,6 +7621,13 @@ public sealed partial class StringBuilder : System.Runtime.Serialization.ISerial
        void System.Runtime.Serialization.ISerializable.GetObjectData(System.Runtime.Serialization.SerializationInfo info, System.Runtime.Serialization.StreamingContext context) { }
        public override string ToString() { throw null; }
        public string ToString(int startIndex, int length) { throw null; }
+        public ChunkEnumerator EnumerateChunks() { throw null; }


From the API review I thought this was supposed to be called GetChunks?

You're right.

davidfowl · 2018-05-24T03:03:05Z

I missed the review here but is there any benefit to exposing this as a ReadOnlySequence<char>?

vancem · 2018-05-24T18:31:07Z

I missed the review here but is there any benefit to exposing this as a ReadOnlySequece?

There is a logistic layering problem because StringBuilder is in system.private.corelib and ReadOnlySequence is not there.

It also seems reasonable that if anyone wants ReadOnlySequence, that they build it using GetChunks().

ahsonkhan · 2018-05-24T22:49:35Z

src/System.Runtime/tests/System/Text/StringBuilderTests.netcoreapp.cs

+                foreach (ReadOnlyMemory<char> chunk in inBuilder.EnumerateChunks())
+                    outStr += new string(chunk.Span);
+
+                // The strings formed by concatinating the chunks should be the same as the value in the StringBuilder. 


nit: concatenating

ahsonkhan · 2018-05-24T22:52:26Z

src/System.Runtime/ref/System.Runtime.cs

+        {
+            public ChunkEnumerator GetEnumerator() { throw null; }
+            public bool MoveNext() { throw null;  }
+            public ReadOnlyMemory<char> Current { get { throw null; } }


nit: reorder this to be in alphabetical order.

@weshaggard, we should also regenerate this ref to clean up some stuff.

For instance, the type isn't fully qualified here: https://github.com/dotnet/corefx/blob/master/src/System.Runtime/ref/System.Runtime.cs#L3743
And we are using abbreviated default syntax:
https://github.com/dotnet/corefx/blob/master/src/System.Runtime/ref/System.Runtime.cs#L5088
Lastly, overridden methods don't tend to have method bodies:
https://github.com/dotnet/corefx/blob/master/src/System.Runtime/ref/System.Runtime.cs#L5086

Vance you normalize this by running our GenerateReferenceSource target on the ref (https://github.com/dotnet/corefx/blob/master/dir.targets#L120). Just run msbuild /t:GenerateReferenceSource in the ref folder (note you will need to undo some changes to the file for the changes that are in the System.Runtime.manual.cs).

That will also remove the enumerator APIs that are getting added in this PR. I think we need to account for System.Private.CoreLib.dll as well (maybe we need to build corefx using the local build of coreclr by passing the path to msbuild /p:CoreCLROverridePath)?
https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/developer-guide.md#testing-with-private-coreclr-bits

Can you please share any additional steps that might be needed to normalize the ref?

If the new APIs are added manually and you just round-trip the system.runtime reference assembly they will not be lost and it will normalize the ref.

Add API baseline.

Can be removed in a week after UWP is updated.

vancem · 2018-05-25T18:07:21Z

@dotnet-bot test Windows x86 Release Build

vancem · 2018-05-25T19:25:05Z

The Arm builds don't have machines to run them. This change has no architecture specific change and previous runs which are VERY similar have passed, so I think it is OK to check in. .

* Rename EumerateChunks->GetChunks Add API baseline. * Fix whitespace * Fix spelling * undo basline * Put back lost rename * Add baseline for new StringBuilder API GetChunks() Can be removed in a week after UWP is updated. Commit migrated from dotnet/corefx@a9ddb9b

stephentoub reviewed May 23, 2018

View reviewed changes

weshaggard reviewed May 24, 2018

View reviewed changes

vancem changed the title ~~Expose StringBuilder.EnumerateChunks()~~ Expose StringBuilder.GetChunks() May 24, 2018

ahsonkhan reviewed May 24, 2018

View reviewed changes

vancem force-pushed the StringBuilderEnumerateChunks branch 3 times, most recently from 98b5ad5 to 35f289c Compare May 25, 2018 16:30

vancem added 5 commits May 25, 2018 09:30

Rename EumerateChunks->GetChunks

5985f55

Add API baseline.

Fix whitespace

bcd7a96

Fix spelling

9acaa71

undo basline

760f630

Put back lost rename

3f75697

vancem force-pushed the StringBuilderEnumerateChunks branch from 35f289c to 760f630 Compare May 25, 2018 16:36

weshaggard approved these changes May 25, 2018

View reviewed changes

Add baseline for new StringBuilder API GetChunks()

8eb5d55

Can be removed in a week after UWP is updated.

vancem merged commit a9ddb9b into dotnet:master May 25, 2018

karelz added this to the 3.0 milestone Jun 2, 2018

ahsonkhan mentioned this pull request Nov 15, 2018

Merge relevant changes from the auto-generated System.Runtime ref #33512

Merged

vancem mentioned this pull request Jan 31, 2020

Make it easy for large StringBuilders to be written to TextWriters without making a large string dotnet/runtime#26347

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose StringBuilder.GetChunks() #29877

Expose StringBuilder.GetChunks() #29877

vancem commented May 23, 2018 •

edited

Loading

stephentoub May 23, 2018

stephentoub May 23, 2018

vancem May 23, 2018

weshaggard May 24, 2018

danmoseley May 24, 2018

davidfowl commented May 24, 2018 •

edited

Loading

vancem commented May 24, 2018

ahsonkhan May 24, 2018

ahsonkhan May 24, 2018

weshaggard May 24, 2018

ahsonkhan May 24, 2018 •

edited

Loading

weshaggard May 25, 2018

vancem commented May 25, 2018

vancem commented May 25, 2018

Expose StringBuilder.GetChunks() #29877

Expose StringBuilder.GetChunks() #29877

Conversation

vancem commented May 23, 2018 • 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

davidfowl commented May 24, 2018 • edited Loading

vancem commented May 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan May 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vancem commented May 25, 2018

vancem commented May 25, 2018

vancem commented May 23, 2018 •

edited

Loading

davidfowl commented May 24, 2018 •

edited

Loading

ahsonkhan May 24, 2018 •

edited

Loading