Added support for splitting on ReadOnlySpan<char> #295

bbartels · 2019-11-26T16:34:32Z

This implementation complies with the specifications discussed in: #934

I tried to add the tests I wrote (adapted from original StringSplitTests), but I was not sure which directory pertains to CoreLib tests.
My best guess is src/libraries/System.Runtime/tests/System, but would like someone to confirm.

This implementation uses multiple types to achieve splitting by a single- and multiple chars. This results in almost identical code in the two types, but could be mitigated by merging the two implementations. I am not sure which implementation is preferred in this scenario.

I need help with the following:

Review Implementation
Add Tests to correct directory
Decide on a merged or separate Enumerator types to handle splitting on an element/sequence

danmoseley · 2019-12-17T03:18:13Z

@ahsonkhan

Wraith2 · 2019-12-17T14:29:03Z

Is the input span to split required to be a single item of n length or n items of 1 length? string split has both single item and array of items overloads which are commonly used. So for example if i call this new method with new ROS(@"\r\n") i think it's going to just look for that literal value as a single item of 2 length, and in that case what input format would be used for splitting on multiple selected whitespace chars equivalent to new []{'\t','\n','\r',\ '}?

bbartels · 2019-12-17T15:35:14Z

@Wraith2 You are correct, it will just treat it as one single value to split on. The reason for that is because when the API was reviewed, the team decided to not include a way to split on multiple chars. So I decided not to include it here.

See 'SplitAny()' in #934 (comment)

stephentoub · 2020-01-15T14:22:28Z

@bbartels, you have this marked WIP. Are you still working on it? It hasn't seen progress in over a month. Thanks.

bbartels · 2020-01-15T14:47:50Z

There is not much I can currently do before someone reviews and/or addresses the points in the Todo above. Maybe I should have made that a little clearer (and possibly omitted WIP as it may not be the appropriate label for this PR).

stephentoub · 2020-01-15T14:49:09Z

I see, ok, thanks. Yes, the way the PR reads right now, it appears you're saying there's nothing for anyone to do until you do more work on it (at least that's how I read it).

bbartels · 2020-01-15T15:02:24Z

My mistake, could have sworn I made it clear enough, but upon rereading I am unsure myself what conclusion to make!

danmoseley · 2020-05-19T22:48:01Z

Adding area owners. Apologies for the delay.

layomia

Thanks for the PR.

We need to expose the API additions through the System.Memory ref - https://github.com/dotnet/runtime/blob/master/src/libraries/System.Memory/ref/System.Memory.cs.

Tests should be added here: https://github.com/dotnet/runtime/tree/master/src/libraries/System.Memory/tests/ReadOnlySpan.

src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.Split.cs

bbartels

@layomia Finished merging the implementations, as well as added tests to the specified directory. There are a couple points I need feedback on below.

bbartels · 2020-05-22T11:38:18Z

src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.Split.cs

+        private readonly ReadOnlySpan<T> _separators;
+        private readonly T _separator;
+        private readonly bool _isSequence;
+        private readonly int _separatorLength;


This could be replaced with a cleaner:

private int SeparatorLength => _isSequence ? _separators.Length : 1;

However, according to my benchmarks it will incur a 3% perf penalty on the tested scenario.
What would be preferred in this case?

The current way is preferred to avoid the extra calculation.

Can you share your benchmarks source/results for this PR?

src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.Split.cs

bbartels · 2020-06-04T09:09:16Z

For some reason I cannot comment on 'outdated' review comments, not sure if this is by design.
I've changed up the implementation to pass all the tests and make the the calculation easier to follow. I hope this resolves the concerns.
There are two review comments I need clarification on before I can proceed (see above).

danmoseley · 2020-06-04T16:49:32Z

Trailing whitespace 😢

bbartels · 2020-06-04T17:04:52Z

@danmosemsft Oh my, no clue how they managed to sneak in. Should be fixed now!

layomia · 2020-06-10T18:25:52Z

Overall, LGTM.

There are two review comments I need clarification on before I can proceed

@bbartels, can you highlight the comments? All my feedback seems to be addressed.

bbartels · 2020-06-10T20:15:21Z

@layomia I think one of them was #295 (comment), but honestly completely forgot what the other one was. Probably should have noted it down 😅, don't think it was too important thought!

EDIT: So it turns out I never pressed Submit Review and my comments weren't published...
Still a bit unfamiliar with the github review system.

layomia · 2020-06-10T20:28:40Z

I think one of them was #295 (comment),

Ah. That's resolved - #295 (comment).

bbartels · 2020-06-10T20:35:18Z

Okay, then there are no further objections from me :) Thank you for the guidance throughout the review, was a good learning experience!

layomia

Thanks a lot, @bbartels!

It would be great if you could add your benchmarks to the dotnet/performance repo here. This doc goes over how to write/run benchmarks for new API. Here's an example PR for new API in System.Text.Json - dotnet/performance#1130.

danmoseley · 2020-06-10T20:41:01Z

I wonder how we could find places we could potentially use this ourselves in this repo. Perhaps someone out there's interested in looking.

davidfowl · 2020-06-10T20:43:49Z

Finally!!!!

bbartels · 2020-06-10T20:46:57Z

@layomia Will do, should have some time tomorrow!
The doc link just links to the same place as your first link. Is it just this?

layomia · 2020-06-10T21:05:56Z

@bbartels

Will do, should have some time tomorrow!

Nice!

If you have some more time, an additional way to widen the impact of this new API is this task - #295 (comment). I imagine this would involve searching this repo for usages of String.Split(), figuring out if the new API can be used instead, and opening a PR with any changes.

The doc link just links to the same place as your first link. Is it just this?

I meant this - https://github.com/dotnet/performance/blob/2c15891e126a397fed9b3c34120228ae21b1ea1f/docs/benchmarking-workflow-dotnet-runtime.md#benchmarking-new-api 😅

src/libraries/System.Memory/tests/ReadOnlySpan/Split.char.cs

GrabYourPitchforks · 2020-06-11T04:11:49Z

Out of curiosity, why is the parameterless Split routing splitting only on the space character (' ')? This differs from string.Split(), which splits on all whitespace, including tabs and line separators.

This also speaks to @danmosemsft's earlier comment. We need to be careful to account for any behavioral differences between this and string.Split before we go changing all of the internal call sites to use this. Otherwise we risk introducing a regression.

It's still a worthwhile exercise though! :)

bbartels · 2020-06-11T06:40:30Z

@GrabYourPitchforks I faintly remember watching the API review on YouTube and there being arguments against introducing anything that has multiple seperators (which would be needed to match string°Split semantics). Though I also remember that there was discussion about sending this back to API Review once the implementation is done. Maybe that's not such a bad idea, to decide on Split() semantics, as it would require a constructor taking a ReadOnlySpan argument for multiple separators,which was previously postponed for UTF-8 reasons.

layomia · 2020-06-11T10:50:06Z

why is the parameterless Split routing splitting only on the space character (' ')?

What I understand from watching the API review discussion is that, for simplicity, the Split methods should be used to split on single separators, while SplitAny could be added in the future for multiple separators. So, the parameterless overload for Split would split on a single space character, while the parameterless SplitAny would be for any whitespace token. What are your thoughts on this? @GrabYourPitchforks

Another area where the current implementation differs from String.Split is the handling of zero-length target strings. I've opened this PR to reconcile them - #37742.

stephentoub · 2020-06-11T11:13:40Z

The parameterless Split should split on anything for which char.IsWhitespace returns true. There's nothing fundamentally wrong with that behavior for string, and any such subtle differences like that from string are going to lead to bugs.

bbartels · 2020-06-11T11:26:43Z

~~I feel like there is no way around another API Review, unless a constructor that does not allow for splitting on multiple separators is introduced, e.g. SpanSplitEnumerator(bool splitOnWhiteSpace).~~

The discussion about semantics of a SplitAny(ReadOnlySpan<char> input, ReadOnlySpan<char> separators) have purposefully been delayed as per this.

stephentoub · 2020-06-11T11:28:50Z

I feel like there is no way around another API Review, unless a constructor that does not allow for splitting on multiple separators is introduced

Why? If you want to split on just ' ', you can do so just as with string by passing that to split. If you want to split on all whitespace, you can do that just as with string by not passing any parameters.

bbartels · 2020-06-11T11:35:41Z

Why? If you want to split on just ' ', you can do so just as with string by passing that to split. If you want to split on all whitespace, you can do that just as with string by not passing any parameters.

Oh, my bad, I forgot the SpanSplitEnumerator constructors were never made public. I was working with that assumption. Otherwise a consumer could have instantiated a SpanSplitEnumerator with multiple separators, the behaviour of which has not been decided yet.

bbartels · 2020-06-11T11:58:00Z

Moving discussion regarding ROS.Split() here: #37746

This reverts commit 78ed8e8.

…7757) This reverts commit 78ed8e8.

Added initial implementation of SpanSplitEnumerator

2da6058

Dotnet-GitSync-Bot added the area-System.Memory label Nov 26, 2019

bbartels mentioned this pull request Dec 17, 2019

Add 'split' support for ReadOnlySpan<char> similar to string #934

Closed

stephentoub changed the title ~~[WIP] Added support for splitting on ReadOnlySpan<char>~~ Added support for splitting on ReadOnlySpan<char> Jan 15, 2020

stephentoub requested a review from ahsonkhan January 15, 2020 14:49

danmoseley requested review from GrabYourPitchforks and adamsitnik May 19, 2020 22:47

danmoseley mentioned this pull request May 19, 2020

Add logic to properly honor naming policy when serializing flag enums #36726

Merged

danmoseley requested review from layomia and removed request for GrabYourPitchforks and adamsitnik May 20, 2020 03:57

danmoseley assigned layomia May 20, 2020

layomia reviewed May 21, 2020

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.Split.cs Outdated Show resolved Hide resolved

layomia added this to the 5.0 milestone May 21, 2020

jtmueller reviewed May 22, 2020

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.Split.cs Outdated Show resolved Hide resolved

bbartels added 3 commits May 22, 2020 12:27

Merged SpanSplitEnumerator and SpanSplitSequenceEnumerator

e795f69

Reordered .projitems entry

0bc1a96

Exposed System.Memory API additions

addae39

bbartels commented May 22, 2020

View reviewed changes

Added SpanSplitEnumerator Tests

d153ebd

jaredpar mentioned this pull request May 25, 2020

Errors installing the SDK during builds #34015

Closed

danmoseley mentioned this pull request May 26, 2020

API proposal: StringSplitOptions.TrimEntries #31038

Closed

bbartels force-pushed the master branch from fda65b0 to 443dad6 Compare June 4, 2020 08:57

Removed trailing whitespace

dab7335

Fixed Unit Tests

be90021

layomia approved these changes Jun 10, 2020

View reviewed changes

layomia merged commit 78ed8e8 into dotnet:master Jun 10, 2020

ahouben reviewed Jun 10, 2020

View reviewed changes

src/libraries/System.Memory/tests/ReadOnlySpan/Split.char.cs Show resolved Hide resolved

layomia mentioned this pull request Jun 11, 2020

Align ROS.Split handling of zero-length target strings with String.Split #37742

Closed

layomia added a commit to layomia/dotnet_runtime that referenced this pull request Jun 11, 2020

Revert "Added support for splitting on ReadOnlySpan<char> (dotnet#295)"

5a4f587

This reverts commit 78ed8e8.

layomia added a commit that referenced this pull request Jun 11, 2020

Revert "Added support for splitting on ReadOnlySpan<char> (#295)" (#3…

d00c5f6

…7757) This reverts commit 78ed8e8.

bbartels mentioned this pull request Jun 17, 2020

Vectorized common String.Split() paths #38001

Merged

ghost locked as resolved and limited conversation to collaborators Dec 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for splitting on ReadOnlySpan<char> #295

Added support for splitting on ReadOnlySpan<char> #295

bbartels commented Nov 26, 2019 •

edited

Loading

danmoseley commented Dec 17, 2019

Wraith2 commented Dec 17, 2019 •

edited

Loading

bbartels commented Dec 17, 2019

stephentoub commented Jan 15, 2020

bbartels commented Jan 15, 2020

stephentoub commented Jan 15, 2020 •

edited

Loading

bbartels commented Jan 15, 2020

danmoseley commented May 19, 2020

layomia left a comment

bbartels left a comment •

edited

Loading

bbartels May 22, 2020

layomia May 28, 2020

layomia May 28, 2020

bbartels commented Jun 4, 2020 •

edited

Loading

danmoseley commented Jun 4, 2020

bbartels commented Jun 4, 2020

layomia commented Jun 10, 2020

bbartels commented Jun 10, 2020 •

edited

Loading

layomia commented Jun 10, 2020

bbartels commented Jun 10, 2020 •

edited

Loading

layomia left a comment •

edited

Loading

danmoseley commented Jun 10, 2020

davidfowl commented Jun 10, 2020

bbartels commented Jun 10, 2020 •

edited

Loading

layomia commented Jun 10, 2020

GrabYourPitchforks commented Jun 11, 2020 •

edited

Loading

bbartels commented Jun 11, 2020

layomia commented Jun 11, 2020 •

edited

Loading

stephentoub commented Jun 11, 2020

bbartels commented Jun 11, 2020 •

edited

Loading

stephentoub commented Jun 11, 2020 •

edited

Loading

bbartels commented Jun 11, 2020 •

edited

Loading

bbartels commented Jun 11, 2020

Added support for splitting on ReadOnlySpan<char> #295

Added support for splitting on ReadOnlySpan<char> #295

Conversation

bbartels commented Nov 26, 2019 • edited Loading

danmoseley commented Dec 17, 2019

Wraith2 commented Dec 17, 2019 • edited Loading

bbartels commented Dec 17, 2019

stephentoub commented Jan 15, 2020

bbartels commented Jan 15, 2020

stephentoub commented Jan 15, 2020 • edited Loading

bbartels commented Jan 15, 2020

danmoseley commented May 19, 2020

layomia left a comment

Choose a reason for hiding this comment

bbartels left a comment • edited Loading

Choose a reason for hiding this comment

bbartels May 22, 2020

Choose a reason for hiding this comment

layomia May 28, 2020

Choose a reason for hiding this comment

layomia May 28, 2020

Choose a reason for hiding this comment

bbartels commented Jun 4, 2020 • edited Loading

danmoseley commented Jun 4, 2020

bbartels commented Jun 4, 2020

layomia commented Jun 10, 2020

bbartels commented Jun 10, 2020 • edited Loading

layomia commented Jun 10, 2020

bbartels commented Jun 10, 2020 • edited Loading

layomia left a comment • edited Loading

Choose a reason for hiding this comment

danmoseley commented Jun 10, 2020

davidfowl commented Jun 10, 2020

bbartels commented Jun 10, 2020 • edited Loading

layomia commented Jun 10, 2020

GrabYourPitchforks commented Jun 11, 2020 • edited Loading

bbartels commented Jun 11, 2020

layomia commented Jun 11, 2020 • edited Loading

stephentoub commented Jun 11, 2020

bbartels commented Jun 11, 2020 • edited Loading

stephentoub commented Jun 11, 2020 • edited Loading

bbartels commented Jun 11, 2020 • edited Loading

bbartels commented Jun 11, 2020

bbartels commented Nov 26, 2019 •

edited

Loading

Wraith2 commented Dec 17, 2019 •

edited

Loading

stephentoub commented Jan 15, 2020 •

edited

Loading

bbartels left a comment •

edited

Loading

bbartels commented Jun 4, 2020 •

edited

Loading

bbartels commented Jun 10, 2020 •

edited

Loading

bbartels commented Jun 10, 2020 •

edited

Loading

layomia left a comment •

edited

Loading

bbartels commented Jun 10, 2020 •

edited

Loading

GrabYourPitchforks commented Jun 11, 2020 •

edited

Loading

layomia commented Jun 11, 2020 •

edited

Loading

bbartels commented Jun 11, 2020 •

edited

Loading

stephentoub commented Jun 11, 2020 •

edited

Loading

bbartels commented Jun 11, 2020 •

edited

Loading