Add ReadOnlySpan string-like Equals/CompareTo/IndexOf/Contains API with globalization support #16467

ahsonkhan · 2018-02-21T07:26:24Z

Part of https://github.com/dotnet/corefx/issues/21395#issuecomment-359906138

Equals
CompareTo
IndexOf ~~(fast span only)~~
Contains ~~(fast span only)~~

TODO:

Add more tests in corefx
~~Verify correctness on Unix~~
Can we expose string.IndexOf with out int matchedLength parameter? Can we expose string.Contains with StringComparison comparisonType parameter? If not, these APIs cannot be implemented for portable span.

Related corefx PR: dotnet/corefx#27319

cc @jkotas, @stephentoub, @KrzysztofCwalina, @tarekgh, @JeremyKuhne, @joshfree

…th globalization support

stephentoub · 2018-02-21T10:03:07Z

Can we expose string.IndexOf with out int matchedLength parameter? Can we expose string.Contains with StringComparison comparisonType parameter? If not, these APIs cannot be implemented for portable span.

Why?

justinvp · 2018-02-21T15:29:25Z

src/mscorlib/shared/System/Span.NonGeneric.cs

+
+        public static bool Equals(this ReadOnlySpan<char> span, ReadOnlySpan<char> value, StringComparison comparisonType)
+        {
+            StringSpanHelpers.CheckStringComparison(comparisonType);


Is this check needed?

justinvp · 2018-02-21T15:29:44Z

src/mscorlib/shared/System/Span.NonGeneric.cs

+
+        public static int CompareTo(this ReadOnlySpan<char> span, ReadOnlySpan<char> value, StringComparison comparisonType)
+        {
+            StringSpanHelpers.CheckStringComparison(comparisonType);


Is this check needed?

justinvp · 2018-02-21T15:30:43Z

src/mscorlib/shared/System/Span.NonGeneric.cs

+
+                case StringComparison.Ordinal:
+                    matchedLength = defaultMatchLength;
+                    return IndexOfOrdinalHelper(span, value, false);


Nit: It'd be nice to specify the name of the bool parameter. Same for a few other calls in this file below.

tarekgh · 2018-02-21T16:34:36Z

src/mscorlib/shared/System/Span.NonGeneric.cs

+                    if (span.Length != value.Length)
+                        return false;
+                    if (value.Length == 0)
+                        return true;


why this return true?

Here span.Length == value.Length == 0 #Closed

right. I misread the previous condition if (span.Length != value.Length)

In reply to: 169812349 [](ancestors = 169812349)

tarekgh · 2018-02-21T16:35:11Z

src/mscorlib/shared/System/Span.NonGeneric.cs

+                    if (span.Length != value.Length)
+                        return false;
+                    if (value.Length == 0)
+                        return true;


why this return true?

Same reason here.

tarekgh · 2018-02-21T16:39:32Z

src/mscorlib/shared/System/Span.NonGeneric.cs

+                    return CompareOrdinalHelper(span, value);
+
+                case StringComparison.OrdinalIgnoreCase:
+                    return CompareInfo.CompareOrdinalIgnoreCase(span, value);


return CompareInfo.CompareOrdinalIgnoreCase(span, value); [](start = 19, length = 58)

why you don't have the condition

if (span.Length == 0 || value.Length == 0)
return span.Length - value.Length;

as you have it in ordinal case?

It is already checked in here: https://github.com/dotnet/coreclr/blob/master/src/mscorlib/shared/System/Globalization/CompareInfo.cs#L596

tarekgh · 2018-02-21T16:43:59Z

src/mscorlib/shared/System/Span.NonGeneric.cs

+                matchedLength = 0;
+                return -1;
+            }
+


do we do this check with the strings APIs? this looks wrong to me.

Yes we do: https://github.com/dotnet/coreclr/blob/master/src/mscorlib/src/System/Globalization/CompareInfo.Windows.cs#L265 #Closed

ok, thanks for confirming

In reply to: 169812892 [](ancestors = 169812892)

tarekgh · 2018-02-21T16:48:58Z

src/mscorlib/shared/System/Span.NonGeneric.cs

+            }
+        }
+
+        internal static unsafe int IndexOfCultureHelper(ReadOnlySpan<char> span, ReadOnlySpan<char> value, int* matchLengthPtr, bool invariantCulture = false)


bool invariantCulture = false [](start = 128, length = 29)

you don't need this. you already checking this value, instead you can just check CompareInfo.Name.Length == 0. remember current culture can be invariant too :-)

you don't need this.

What are you referring to?

I don't understand what you mean.

you don't need the parameter invariantCulture.

just check CompareInfo.Name.Length == 0 inside the method

In reply to: 169813408 [](ancestors = 169813408)

tarekgh · 2018-02-21T16:49:59Z

src/mscorlib/shared/System/Span.NonGeneric.cs

+                CultureInfo.CurrentCulture.CompareInfo.IndexOf(span, value, CompareOptions.None, matchLengthPtr);
+        }
+
+        internal static unsafe int IndexOfCultureIgnoreCaseHelper(ReadOnlySpan<char> span, ReadOnlySpan<char> value, int* matchLengthPtr, bool invariantCulture = false)


bool invariantCulture = false [](start = 138, length = 29)

same comment as the one on IndexOfCultureHelper

tarekgh · 2018-02-21T16:56:43Z

src/mscorlib/shared/System/Span.NonGeneric.cs

+
+                    // Since we know that the first two chars are the same,
+                    // we can increment by 2 here and skip 4 bytes.
+                    // This leaves us 8-byte aligned, which results


This leaves us 8-byte [](start = 23, length = 21)

@jkotas does 4-bytes alignments considered as 8-bytes aligned too?

This comment and optimization was true for String implementation. It does not hold for Span implementation.

tarekgh · 2018-02-21T17:00:48Z

src/mscorlib/shared/System/Span.NonGeneric.cs

+                while (length > 0)
+                {
+                    if (*(int*)a != *(int*)b)
+                        goto DiffNextInt;


is this correct? what happen if we have length == 1?

Again, this is invalid copy&paste from the String implementation.

tarekgh · 2018-02-21T17:09:31Z

src/mscorlib/src/System/Globalization/CompareInfo.Unix.cs

            return -1;
        }

+        internal static unsafe int IndexOfOrdinalCore(ReadOnlySpan<char> source, ReadOnlySpan<char> value, bool ignoreCase)


IndexOfOrdinalCore [](start = 35, length = 18)

why duplicating the code here? isn't better to have such methods take char* and avoid the code duplication?

If we have a method that takes a char*, we would have to pin the span/string to get the pointer in all cases (unnecessarily) instead of only around the OS call. Given this change - #16434, I want to re-measure the cost of casting string to span and see if we can address the code duplication that way. I will address this outside this PR.

tarekgh · 2018-02-21T17:18:25Z

src/mscorlib/src/System/Globalization/CompareInfo.Unix.cs

+            Debug.Assert(!_invariantMode);
+            Debug.Assert(source.Length != 0);
+            Debug.Assert(target.Length != 0);
+            Debug.Assert((options == CompareOptions.None || options == CompareOptions.IgnoreCase));


Debug.Assert((options == CompareOptions.None || options == CompareOptions.IgnoreCase)); [](start = 11, length = 88)

Although this may look true from the calls coming from Span but we may support spans in CompareInfo which will allow passing more options than what we are currently passing. so we'll need to re-visit this code again if we did that in the future. I prefer writing this code here with the assumptions you may get more options than what we are currently passing from the span APIs.

What options should this Debug.Assert check then?

remove the assert.

In reply to: 169814071 [](ancestors = 169814071)

We have similar assert here - https://github.com/dotnet/coreclr/blob/master/src/mscorlib/src/System/Globalization/CompareInfo.Unix.cs#L180

tarekgh · 2018-02-21T17:31:52Z

@stephentoub @ahsonkhan

Can we expose string.IndexOf with out int matchedLength parameter? Can we expose string.Contains with StringComparison comparisonType parameter? If not, these APIs cannot be implemented for portable span.

Why?

I believe Contains can be implemented using IndexOf. but the problem would be in the APIs that need to return the matchedLength as there is no easy way to get this length from the currently exposed APIs.

We may have the following options:

We can implement getting the matched length with a very slow path, by repeating the sub-string comparisons until we get the right match length.
We may think in doing the interop with the OS as we are doing coreclr. this is not a big deal to do.
We decide not supporting these APIs for the portable.

I prefer the second option.

stephentoub · 2018-02-21T18:59:47Z

but the problem would be in the APIs that need to return the matchedLength as there is no easy way to get this length from the currently exposed APIs

My question was, why do we need matchedLength at all? Why is that required in order to support slow span?

ahsonkhan · 2018-02-21T19:02:52Z

My question was, why do we need matchedLength at all?

See https://github.com/dotnet/corefx/issues/21395#issuecomment-359980642

Example: imagine the source span have the characters A ̀ and we are searching for Á with culture aware operation. IndexOf will return the index of the match and will return the matched length is 2 and not just 1.

If the user uses target.Length (i.e. 1) to slice the source to keep going, that would be incorrect.

stephentoub · 2018-02-21T19:04:09Z

Maybe I misunderstood the question. @ahsonkhan wrote:

"Can we expose string.IndexOf with out int matchedLength parameter? Can we expose string.Contains with StringComparison comparisonType parameter? If not, these APIs cannot be implemented for portable span."

To me that sounds like he's saying unless we have out int matchedLength in the signature of IndexOf, we can't implement IndexOf at all. Was that not the question?

ahsonkhan · 2018-02-21T19:23:39Z

To me that sounds like he's saying unless we have out int matchedLength in the signature of IndexOf, we can't implement IndexOf at all. Was that not the question?

To clarify, we can probably implement it but it would require doing a lot of the work that the string APIs already do and it will be slow.

How would we correctly (and with reasonable perf) implement this API with the existing string public surface area (for portable span)?

public static int IndexOf(this ReadOnlySpan<char> span, ReadOnlySpan<char> value, StringComparison comparisonType, out int matchedLength)

I was hoping for something like this:

public static int IndexOf(this ReadOnlySpan<char> span, ReadOnlySpan<char> value, StringComparison comparisonType, out int matchedLength)
{
 return span.AsReadOnlySpan().IndexOf(value.AsReadOnlySpan(), comparisonType, out matchedLength);
}

But, we can't do that. And it doesn't look like we can add the String APIs to netfx to allow this.

tarekgh · 2018-02-21T19:25:35Z

@ahsonkhan I have proposed how we can do that in #16467 (comment)

stephentoub · 2018-02-21T19:43:49Z

We may think in doing the interop with the OS as we are doing coreclr. this is not a big deal to do.

@tarekgh, can you elaborate? You're proposing re-implementing that support in System.Memory for the slow span implementation? What about on Unix where we call through System.Globalization.Native, require ICU, etc... we'd be signing up to make sure that this implementation works with downlevel System.Globalization.Natives?

stephentoub · 2018-02-21T19:50:18Z

But, we can't do that. And it doesn't look like we can add the String APIs to netfx to allow this.

Ok, I was missing that you were talking about adding a new String.IndexOf to .NET Framework. Even if that were to happen, it would be for a future release, and that won't help the System.Memory implementation running on whatever netstandard version it's targeting.

tarekgh · 2018-02-21T20:18:47Z

@stephentoub

can you elaborate? You're proposing re-implementing that support in System.Memory for the slow span implementation? What about on Unix where we call through System.Globalization.Native, require ICU, etc... we'd be signing up to make sure that this implementation works with downlevel System.Globalization.Natives?

My suggestion was basically getting the matched length using the OS help. so the suggestion is scoped only when we doing IndexOf call.
I am seeing most of Linux users will just use net core and not use the portable library. I know this statement is not 100% accurate but expecting the majority of user will do that. The portable library will be more interesting for Windows users. If this right, then we'll just need to do only one call to Windows to get the matched length. for Linux users who want to use the portable library, we can provide the slow implementation and ask them to move to 2.1 to get the perf gain.

side point, I can see there is some complication here and I already talked offline with @ahsonkhan, so it may be ok for now not providing this functionality and we can consider it in the future releases.

ahsonkhan · 2018-02-22T06:12:15Z

@dotnet-bot test Windows_NT arm Cross Checked corefx_baseline Build and Test
@dotnet-bot test Windows_NT x64_arm64_altjit Checked corefx_baseline
@dotnet-bot test Ubuntu x64 Checked corefx_baseline
@dotnet-bot test Windows_NT x64 Checked corefx_baseline
@dotnet-bot test Windows_NT x86_arm_altjit Checked corefx_baseline
@dotnet-bot test Windows_NT x86 Checked corefx_baseline

ahsonkhan · 2018-02-22T07:56:01Z

/mnt/j/workspace/dotnet_coreclr/master/jitstress/x64_checked_ubuntu_corefx_baseline_prtest/_/fx/bin/tests/System.Net.Security.Tests/netcoreapp-Linux-Release-x64/RunTests.sh: line 85:  2694 Aborted                 (core dumped) $RUNTIME_PATH/dotnet xunit.console.netcore.exe System.Net.Security.Tests.dll -xml testResults.xml -notrait Benchmark=true -notrait category=nonnetcoreapptests -notrait category=nonlinuxtests -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing
23:13:44   /mnt/j/workspace/dotnet_coreclr/master/jitstress/x64_checked_ubuntu_corefx_baseline_prtest/_/fx/src/System.Net.Security/tests/FunctionalTests
23:13:44   ----- end 07:13:44 ----- exit code 134 ----------------------------------------------------------
23:13:44 /mnt/j/workspace/dotnet_coreclr/master/jitstress/x64_checked_ubuntu_corefx_baseline_prtest/_/fx/Tools/tests.targets(492,5): warning MSB3073: The command "/mnt/j/workspace/dotnet_coreclr/master/jitstress/x64_checked_ubuntu_corefx_baseline_prtest/_/fx/bin/tests/System.Net.Security.Tests/netcoreapp-Linux-Release-x64/RunTests.sh /mnt/j/workspace/dotnet_coreclr/master/jitstress/x64_checked_ubuntu_corefx_baseline_prtest/_/fx/bin/testhost/netcoreapp-Linux-Release-x64/" exited with code 134. [/mnt/j/workspace/dotnet_coreclr/master/jitstress/x64_checked_ubuntu_corefx_baseline_prtest/_/fx/src/System.Net.Security/tests/FunctionalTests/System.Net.Security.Tests.csproj]
23:13:44 
/mnt/j/workspace/dotnet_coreclr/master/jitstress/x64_checked_ubuntu_corefx_baseline_prtest/_/fx/Tools/tests.targets(500,5): error : One or more tests failed while running tests from 'System.Net.Security.Tests' please check /mnt/j/workspace/dotnet_coreclr/master/jitstress/x64_checked_ubuntu_corefx_baseline_prtest/_/fx/bin/tests/System.Net.Security.Tests/netcoreapp-Linux-Release-x64/testResults.xml for details!

@dotnet-bot test Ubuntu x64 Checked corefx_baseline

ahsonkhan · 2018-02-22T09:59:30Z

This failure is not related to the PR (see dotnet/corefx#27339 (comment)):

 System.IO.FileSystem.DriveInfoTests.DriveInfoWindowsTests.Ctor_InvalidPath_ThrowsArgumentException(driveName: \"\", paramName: \"path\") [FAIL]
23:01:15         Assert.Equal() Failure
23:01:15         Expected: path
23:01:15         Actual:   (null)
23:01:15         Stack Trace:
23:01:16            D:\j\workspace\x64_checked_w---d7295605\_\fx\src\CoreFx.Private.TestUtilities\src\System\AssertExtensions.cs(40,0): at System.AssertExtensions.Throws[T](String netCoreParamName, String netFxParamName, Action action)
23:01:16   Finished:    System.IO.FileSystem.DriveInfo.Tests

cc @JeremyKuhne

ahsonkhan · 2018-02-22T16:12:56Z

Any other feedback?

tarekgh · 2018-02-22T16:24:26Z

            // PERF: This depends on the fact that the String objects are always zero terminated

Is this true for spans? I mean the spans will be always null terminated?

Refers to: src/mscorlib/shared/System/Span.NonGeneric.cs:392 in b34cfd0. [](commit_id = b34cfd0, deletion_comment = False)

ahsonkhan · 2018-02-22T16:25:55Z

Is this true for spans? I mean the spans will be always null terminated?

No, this isn't true for spans. I will address it outside this PR.

tarekgh · 2018-02-22T16:37:04Z

I don't have any other feedback. thanks for getting this ready. I assume you have tested this as we are testing the same functionality with strings.

LGTM

Add ReadOnlySpan string-like Equals/CompareTo/IndexOf/Contains API wi…

eff78a3

…th globalization support

ahsonkhan added the area-System.Memory label Feb 21, 2018

ahsonkhan self-assigned this Feb 21, 2018

ahsonkhan mentioned this pull request Feb 21, 2018

Add ROSpan Equals/CompareTo/IndexOf/Contains string-like APIs with StringComparison dotnet/corefx#27319

Merged

justinvp reviewed Feb 21, 2018

View reviewed changes

tarekgh reviewed Feb 21, 2018

View reviewed changes

ahsonkhan closed this Feb 21, 2018

ahsonkhan reopened this Feb 21, 2018

ahsonkhan added 2 commits February 21, 2018 18:44

Address PR feedback.

7e14d49

Fix unix implementation

b34cfd0

ahsonkhan merged commit cb4dd60 into dotnet:master Feb 22, 2018

ahsonkhan deleted the StringSpanAPIs branch February 22, 2018 21:01

ViktorHofer mentioned this pull request Jan 31, 2020

Reconcile CompareInfo code duplicates introduced with Span extension methods dotnet/runtime#26621

Closed

Add ReadOnlySpan string-like Equals/CompareTo/IndexOf/Contains API with globalization support #16467

Add ReadOnlySpan string-like Equals/CompareTo/IndexOf/Contains API with globalization support #16467

Uh oh!

Conversation

ahsonkhan commented Feb 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephentoub commented Feb 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Feb 21, 2018 • edited by tarekgh Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Feb 21, 2018 • edited by tarekgh Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tarekgh commented Feb 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephentoub commented Feb 21, 2018

Uh oh!

ahsonkhan commented Feb 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephentoub commented Feb 21, 2018

Uh oh!

ahsonkhan commented Feb 21, 2018

Uh oh!

tarekgh commented Feb 21, 2018

ahsonkhan commented Feb 21, 2018 •

edited

Loading

ahsonkhan Feb 21, 2018 •

edited by tarekgh

Loading

ahsonkhan Feb 21, 2018 •

edited by tarekgh

Loading

tarekgh commented Feb 21, 2018 •

edited

Loading

ahsonkhan commented Feb 21, 2018 •

edited

Loading

stephentoub commented Feb 21, 2018 •

edited

Loading

tarekgh commented Feb 21, 2018 •

edited

Loading

ahsonkhan commented Feb 22, 2018 •

edited

Loading