Add ROSpan StartsWith and EndsWith string-like APIs with StringComparison by ahsonkhan · Pull Request #26880 · dotnet/corefx

ahsonkhan · 2018-02-06T02:32:01Z

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

StartsWith
EndsWith

Depends on: dotnet/coreclr#16223 and dotnet/coreclr#16418

TODO: ~~Add tests.~~

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

…ison

justinvp · 2018-02-06T06:26:04Z

+        /// <param name="span">The source span.</param>
+        /// <param name="value">The sequence to compare to the end of the source span.</param>
+        /// <param name="comparison">One of the enumeration values that determines how the <paramref name="span"/> and <paramref name="value"/> are compared.</param>
+        public static bool EndsWith(this ReadOnlySpan<char> span, ReadOnlySpan<char> value, StringComparison comparison)


comparison -> comparisonType to match the naming used everywhere else.

stephentoub · 2018-02-06T10:35:50Z

+        public static bool EndsWith(this ReadOnlySpan<char> span, ReadOnlySpan<char> value, StringComparison comparison)
+        {
+            string sourceString = span.ToString();
+            string valueString = value.ToString();


Ouch; this is really unfortunate. We should consider only exposing these for core.

If we keep these, we should at least do a reasonable implementation for Ordinal and OrdinalIgnoreCase, and presumably we could do so for Invariant as well? I agree the CurrentCulture ones will be problematic.

Invariant would be as the current culture (if the string is not fully ASCII).

I think there is opportunity to optimize for the ordinal and ordinal ignore casing but I think we already saying the portable implementation would be slower than the core one and whatever we do will not be able to match the core performance (i guess).

not exposing these would be an issue for anyone want to write a library that work on net core and down level too. I prefer keeping it and document it as it is slower than the core one and we can try optimize here as much we can.

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

I can file an issue to optimize this method for the ordinal cases. We do have the generic StartsWith method that take a T, which does not take SringComparison that can be used for one of the two scenarios.

FYI (from API review):

// Those need access to globalization APIs. We'll also expose them from // the .NET Framework OOB (slow span). They will try to extract the string // from the underlying span (because slow span stores it) -- or -- allocate // a new string. This avoids bifurcating the API surface.

They will try to extract the string from the underlying span (because slow span stores it)

Can this change do this then?

I haven't been able to figure out how to do that. The underlying data is stored as a Pinnable<char> with byte offset, and the globalization APIs being utilized work on strings (not char*), at least publically.

It looks like we're using Unsafe.As to cast the object to a Pinnable. Can you just do a type check on it and cast it back? e.g. object o = _pinnable; if (o is string s) { ... }? @jkotas or @atsushikan could comment on the safety of all this.

If that is doable, I can definitely update our ToString implementation and take advantage of that as well.

Is there a way to do something similar for the FastSpan ToString which has a ByReference field?

No, there is not a reasonable way to do that for fast span.

…ible.

jkotas · 2018-02-07T00:25:35Z

        {
            if (typeof(T) == typeof(char))
            {
+                if (_pinnable is string str)


This is not enough. You have to also check that the offset and length are for the whole string (ie that the Span is not pointing at substring).

Would adding offset and length checks be sufficient here?

if (obj is string str && _byteOffset == 0 && _length == str.Length)

You need to use StringAdjustment for the offset.

jkotas · 2018-02-07T00:29:01Z

        {
            if (typeof(T) == typeof(char))
            {
+                if (_pinnable is string str)


Note that the optimizing compilers can figure out here that Pinnable<T> can never be a string, and turn this condition into false.

I think you may want to pass it through Unsafe.As here to strip the type information here. It is not guaranteed to work, but it is increasing the changes that it will.

We may want to have a slow-span specific test for this so that we will know when it does not work.

Note that the optimizing compilers can figure out here that Pinnable can never be a string

Even the C# compiler will... that's why in my example I stored it into an object first, otherwise the C# compiler just emit the cast as a hard-coded false. Though as you say an even better compiler could see through the object cast as well.

ahsonkhan · 2018-02-07T01:03:53Z

        {
            if (typeof(T) == typeof(char))
            {
+                if (_pinnable is string str)


Can this ever be true for Span or should I remove this check?

ahsonkhan · 2018-02-07T02:09:10Z

For slow span, if we get a read/write Span from Memory, we have Pinnable<T> != null and hence ToString() can return the underlying string.

But, if we construct a span from void* and length, Pinnable<T> == null and then ToString() returns a new string.

Are we fine with this discrepancy?

string original = TestHelpers.BuildString(10, 42);
ReadOnlyMemory<char> readOnlyMemory = original.AsReadOnlyMemory();
Memory<char> memory = MemoryMarshal.AsMemory(readOnlyMemory);

fixed (char* pOriginal = original)
fixed (char* pString1 = memory.Span.ToString())
{
    Assert.Equal((int)pOriginal, (int)pString1); // passes the test
}

fixed (char* pOriginal = original)
{
    Span<char> span = new Span<char>(pOriginal, original.Length);
    fixed (char* pString1 = span.ToString())
    {
        Assert.Equal((int)pOriginal, (int)pString1); // fails the test
    }
}

…rtsWith

joshfree · 2018-02-08T22:50:51Z

+            string subString2 = span.Slice(0, 2).ToString();
+            string subString3 = span.Slice(1, 2).ToString();
+
+            Assert.Equal("RDDNEGSNET", returnedString);


nit: shouldn't these use original instead of the hardcoded literal

…does ordinal comparison

…rtsWith

ahsonkhan · 2018-02-15T18:40:06Z

@dotnet-bot test this please

ahsonkhan · 2018-02-15T19:34:26Z

@dotnet-bot test this please

stephentoub · 2018-02-15T20:29:57Z

            if (typeof(T) == typeof(char))
            {
+                object obj = Unsafe.As<Pinnable<T>, object>(ref Unsafe.AsRef(_pinnable));
+                if (obj is string str && _byteOffset == MemoryExtensions.StringAdjustment && _length == str.Length)


As in #27170, how about doing the _byteOffset check before doing any of the casting / type checking?

stephentoub · 2018-02-15T20:30:44Z

+            {
+                Assert.Equal((int)pOriginal, (int)pString1);
+                Assert.Equal((int)pOriginal, (int)pString2);
+            }


Why not just use Assert.Same?

stephentoub · 2018-02-15T20:30:54Z

+                Assert.NotEqual((int)pSubString1, (int)pSubString2);
+                Assert.NotEqual((int)pSubString1, (int)pSubString3);
+                Assert.NotEqual((int)pSubString2, (int)pSubString3);
+            }


And Assert.NotSame?

stephentoub · 2018-02-15T20:31:26Z

+        // This test is only relevant for portable span
+#if FEATURE_PORTABLE_SPAN
+        [Fact]
+#endif


Is this the convention used in this project? Elsewhere in corefx we've used SkipOnTargetFramework, e.g. https://github.com/dotnet/corefx/pull/27170/files#diff-f8d6f156088e92893813570d9608526eR63

I am not sure if NetFramework is the only TargetFrameworkMoniker where we would run this test (uapaot in particular).

Essentially, we want to run on !netcoreapp and !uap, based on when IsPartialFacadeAssembly is false:

corefx/src/System.Memory/ref/System.Memory.csproj

Line 10 in 540a99c

<DefineConstants Condition="'$(IsPartialFacadeAssembly)' != 'true'">$(DefineConstants);FEATURE_PORTABLE_SPAN</DefineConstants>

But given this, I will change it.

For reference, the framework monikers that are supported: https://github.com/dotnet/buildtools/blob/master/src/xunit.netcore.extensions/TargetFrameworkMonikers.cs#L23-L26

stephentoub · 2018-02-15T20:32:02Z

        {
            if (typeof(T) == typeof(char))
            {
+                object obj = Unsafe.As<Pinnable<T>, object>(ref Unsafe.AsRef(_pinnable));


Can this just be:

object obj = Unsafe.As<object>(_pinnable);

? Or does doing this via the refs help further?

Nit: (Also, a comment indicating why this is used rather than just if (_pinnable is string str ...) would be helpful.)

I will merge the changes from your PR into here which will include the comments, test updates, etc.

Thanks!

Cool, thanks. Didn't realize you'd already done the initial work (I remember we'd talked about it, but then couldn't find it in the repo 😄)

stephentoub · 2018-02-15T20:34:13Z

+                if (obj is string str && _byteOffset == MemoryExtensions.StringAdjustment && _length == str.Length)
+                {
+                    return str;
+                }


Is this change needed? When will a Span<char> (rather than a ReadOnlySpan<char>) wrap a string?

See code sample here: #26880 (comment)

I asked the same question but since we support ReadOnlyMemory -> Memory conversion, it is possible (based on the comments here - https://github.com/dotnet/corefx/blob/master/src/Common/src/CoreLib/System/Memory.cs#L195).

I personally don't think it's worthwhile to add this check to Span<char>. It's such a corner case to optimize for, and at the expense of adding a type check to other Span<char>.ToString() calls. And in general folks can't depend on ToString producing the same string from multiple calls, so I don't consider the discrepancy a real issue.

I don't have a strong opinion on this. I am fine with removing this check for Span. @KrzysztofCwalina, @jkotas - any objections?

ahsonkhan · 2018-02-16T01:46:34Z

@dotnet-bot test Linux x64 Release Build

ahsonkhan · 2018-02-21T21:13:18Z

@dotnet-bot test this please

…rtsWith

…ison (dotnet/corefx#26880) * Add ROSpan StartsWith and EndsWith string-like APIs with StringComparison * We have separate implementations of slow and fast span. * Remove unused using directive. * Address PR feedback and add tests. * Add test files * Get the string from the underlying span to avoid allocation when possible. * Update ToString and add tests * Update Span ToString and add tests * Address PR feedback and disable ToString test for fast span * Borrow additional tests from the existing StringTests test bed. * Fix comment grammar * No StringComparison results in generic StartsWith being called which does ordinal comparison * Address feedback related to ToString and tests * Fix tests for culture specific cases, for Unix. * Fix typo in variable names * Respond to recent change AsReadOnlySpan -> AsSpan * Fix typo in test. Commit migrated from dotnet/corefx@42d5370

Add ROSpan StartsWith and EndsWith string-like APIs with StringCompar…

53c16cf

…ison

ahsonkhan added blocked Issue/PR is blocked on something - see comments area-System.Memory labels Feb 6, 2018

ahsonkhan self-assigned this Feb 6, 2018

ahsonkhan mentioned this pull request Feb 6, 2018

Add ReadOnlySpan string-like StartsWith and EndsWith API with globalization support dotnet/coreclr#16223

Merged

ahsonkhan added 2 commits February 5, 2018 18:36

We have separate implementations of slow and fast span.

18e7f69

Remove unused using directive.

a257e3b

justinvp reviewed Feb 6, 2018

View reviewed changes

stephentoub reviewed Feb 6, 2018

View reviewed changes

ahsonkhan added 3 commits February 6, 2018 15:50

Address PR feedback and add tests.

ddfb49d

Add test files

8df2742

Get the string from the underlying span to avoid allocation when poss…

5c8114d

…ible.

jkotas reviewed Feb 7, 2018

View reviewed changes

ahsonkhan commented Feb 7, 2018

View reviewed changes

Update ToString and add tests

273bc7e

ahsonkhan added 2 commits February 6, 2018 18:09

Update Span ToString and add tests

2d5444b

Merge branch 'master' of https://github.com/dotnet/corefx into AddSta…

041692a

…rtsWith

joshfree reviewed Feb 8, 2018

View reviewed changes

ahsonkhan added 4 commits February 8, 2018 16:34

Address PR feedback and disable ToString test for fast span

162a710

Borrow additional tests from the existing StringTests test bed.

a090776

Fix comment grammar

839bf29

No StringComparison results in generic StartsWith being called which …

8aaf7c2

…does ordinal comparison

ahsonkhan requested review from KrzysztofCwalina and tarekgh February 10, 2018 02:11

Merge branch 'master' of https://github.com/dotnet/corefx into AddSta…

410240f

…rtsWith

ahsonkhan mentioned this pull request Feb 15, 2018

Add fast-path optimization to slow ReadOnlySpan<char>'s ToString #27170

Closed

stephentoub reviewed Feb 15, 2018

View reviewed changes

ahsonkhan added 2 commits February 15, 2018 18:28

Address feedback related to ToString and tests

7d0fa71

Fix tests for culture specific cases, for Unix.

4bac971

ahsonkhan mentioned this pull request Feb 16, 2018

Fix impl of ReadOnlySpan StartsWith/EndsWith for Unix. dotnet/coreclr#16418

Merged

Fix typo in variable names

a2ada65

ahsonkhan removed the blocked Issue/PR is blocked on something - see comments label Feb 21, 2018

ahsonkhan added 3 commits February 21, 2018 13:48

Merge branch 'master' of https://github.com/dotnet/corefx into AddSta…

8f9ca8c

…rtsWith

Respond to recent change AsReadOnlySpan -> AsSpan

b9167c2

Fix typo in test.

4b9d13e

ahsonkhan merged commit 42d5370 into dotnet:master Feb 22, 2018

ahsonkhan deleted the AddStartsWith branch February 22, 2018 01:09

karelz added this to the 2.1.0 milestone Mar 10, 2018

Conversation

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

Uh oh!

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 6, 2018 • edited 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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub Feb 6, 2018 • edited 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!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan commented Feb 15, 2018

Uh oh!

ahsonkhan commented Feb 15, 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!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Feb 15, 2018 • edited 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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub Feb 15, 2018 • edited 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

ahsonkhan commented Feb 6, 2018 •

edited

Loading

ahsonkhan Feb 6, 2018 •

edited

Loading

ahsonkhan Feb 6, 2018 •

edited

Loading

stephentoub Feb 6, 2018 •

edited

Loading

ahsonkhan commented Feb 7, 2018 •

edited

Loading

ahsonkhan Feb 15, 2018 •

edited

Loading

ahsonkhan Feb 15, 2018 •

edited

Loading

stephentoub Feb 15, 2018 •

edited

Loading

ahsonkhan Feb 15, 2018 •

edited

Loading