Remove StringSpanHelpers and start using MemoryExtensions by ahsonkhan · Pull Request #16718 · dotnet/coreclr

ahsonkhan · 2018-03-02T20:27:04Z

cc @stephentoub, @jkotas, @tarekgh, @benaadams

Ignore the change to src/jit/hwintrinsiclistxarch.h (it's from #16715)

ahsonkhan · 2018-03-02T20:27:22Z

@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-03-02T20:55:50Z

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test

jcagme · 2018-03-02T22:01:21Z

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test please

jcagme · 2018-03-02T22:17:51Z

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test please

jcagme · 2018-03-02T23:12:02Z

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test please

stephentoub · 2018-03-02T23:15:35Z

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


-        // TODO https://github.com/dotnet/corefx/issues/21395: Expose this publicly?
-        internal virtual int Compare(ReadOnlySpan<char> string1, ReadOnlySpan<char> string2, CompareOptions options)
+        internal virtual int CompareOptionNone(ReadOnlySpan<char> string1, ReadOnlySpan<char> string2, CompareOptions options)


Why take take an options and force it to be None if it's not going to be used?

We need to pass the option to CompareString which passes it to the interop call.

Hmm... I just realized what you meant. Lol.

We can simply do the following:

internal virtual int CompareOptionNone(ReadOnlySpan<char> string1, ReadOnlySpan<char> string2) { return _invariantMode ? string.CompareOrdinal(string1, string2) : CompareString(string1, string2, CompareOptions.None); }

stephentoub · 2018-03-02T23:16:13Z

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

-            return CompareString(string1, string2, options);
+        internal virtual int CompareOptionIgnoreCase(ReadOnlySpan<char> string1, ReadOnlySpan<char> string2, CompareOptions options)
+        {
+            Debug.Assert(options == CompareOptions.IgnoreCase);


Same question for IgnoreCase... why aren't all of these just calling CompareString?

Should I make private unsafe int CompareString(ReadOnlySpan<char> string1, ReadOnlySpan<char> string2, CompareOptions options) internal and call it directly?

Nevermind. Ignore my comment.

jcagme · 2018-03-02T23:17:10Z

Trying MSIT fix
@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test please

stephentoub · 2018-03-02T23:21:37Z

src/mscorlib/shared/System/Guid.cs

            // Find the end of this hex number (since it is not fixed length)
            numStart = 3;
-            numLen = guidString.IndexOf(',', numStart) - numStart;
+            numLen = guidString.Slice(numStart).IndexOf(',') - numStart;


Have you runs our tests, and does this pass? Putting in the slice will change the resulting value as compared to passing in a start index.

Same comment for all of these.

I ran the corefx System.Runtime tests locally. I didn't run the coreclr tests.

I will fix the bug and run the coreclr tests.

I actually meant the corefx tests. If we don't have a test that catches this, we should. This is wrong, right? Consider if numStart is 3 and the ',' is found at 4. Previously IndexOf(',',numStart) would return 4, and then we'd subtract 3 to get 1. Now IndexOf(',') will return 1, and then we'll subtract 3 to get -2.

Yes, it is incorrect. We need to remove the - numStart

We have these tests that, theoretically, should call into TryParseGuidWithHexPrefix. I will re-run the tests locally to see if they fail.

Yep, the tests catch this bug and do fail. I must not have re-built coreclr successfully with my changes earlier (probably due to the issue with HW_Flag_BaseTypeFromFirstArg).

stephentoub · 2018-03-02T23:23:16Z

src/mscorlib/shared/System/Globalization/DateTimeParse.cs

+                    Span<char> result = new char[i + 1];
+                    result[i] = ch;
+                    Value.Slice(0, i).CopyTo(result);
+                    Value = result.AsReadOnlySpan();


Do we have any tests that hit this and the below removal paths? It'd be good to make sure they're covered.

I will check and add if needed.

Added some tests for this particular codepath - dotnet/corefx#27678

Filed an issue for the rest: https://github.com/dotnet/corefx/issues/27679

stephentoub · 2018-03-02T23:25:39Z

src/mscorlib/shared/System/Version.cs

            // We musn't have any separators after build.
            int buildEnd = -1;
-            int minorEnd = input.IndexOf('.', majorEnd + 1);
+            int minorEnd = input.Slice(majorEnd + 1).IndexOf('.');


As with the other cases where slicing was added, I don't think this and the below cases do the right thing.

stephentoub · 2018-03-02T23:26:03Z

src/mscorlib/src/System/String.Comparison.cs


        public bool StartsWith(char value) => Length != 0 && _firstChar == value;
+
+        public static void CheckStringComparison(StringComparison comparisonType)


Good catch. Will fix.

stephentoub · 2018-03-02T23:26:14Z

src/mscorlib/src/System/String.Comparison.cs

+            // Single comparison to check if comparisonType is within [CurrentCulture .. OrdinalIgnoreCase]
+            if ((uint)(comparisonType - StringComparison.CurrentCulture) > (StringComparison.OrdinalIgnoreCase - StringComparison.CurrentCulture))
+            {
+                throw new ArgumentException(SR.NotSupported_StringComparison, nameof(comparisonType));


Should this use ThrowHelper?

We don't use ThrowHelper anywhere in this file. In what scenario would it help?

ThrowHelper outside generics (where its codegen size); it helps with inlining and the size when inlined.

So

((uint)(comparisonType - StringComparison.CurrentCulture) > (StringComparison.OrdinalIgnoreCase - StringComparison.CurrentCulture))

will probably easily inline

but add new, throw, nameof, SR.NotSupported_StringComparison and it probably won't

Whereas ThrowHelper.ThrowArgumentException(ExceptionResource.NotSupported_StringComparison) is one int constant register load and a call with no post cleanup (as it only throws).

Gotcha. So, it will help inline CheckStringComparison, mainly for when it won't end up throwing (which is the most common scenario).

jcagme · 2018-03-02T23:35:13Z

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test please

jcagme · 2018-03-03T00:32:22Z

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test please

…tringCleanup

ahsonkhan · 2018-03-03T06:31:42Z

OSX - https://github.com/dotnet/core-eng/issues/2808
Tizen:

qemu: Unsupported syscall: 389

Any other feedback before this gets merged?

ahsonkhan · 2018-03-03T23:49:13Z

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test

Remove StringSpanHelpers and start using MemoryExtensions

0c3c9a1

ahsonkhan added the area-System.Memory label Mar 2, 2018

ahsonkhan self-assigned this Mar 2, 2018

stephentoub reviewed Mar 2, 2018

View reviewed changes

ahsonkhan added 2 commits March 2, 2018 20:58

Address PR feedback.

f6809ca

Merge branch 'master' of https://github.com/dotnet/coreclr into SpanS…

2912633

…tringCleanup

ahsonkhan mentioned this pull request Mar 3, 2018

Add DateTime ParseExact test with 'Allow White' styles dotnet/corefx#27678

Merged

ahsonkhan merged commit 7ac6d5c into dotnet:master Mar 4, 2018

ahsonkhan deleted the SpanStringCleanup branch March 4, 2018 01:48


		public bool StartsWith(char value) => Length != 0 && _firstChar == value;

		public static void CheckStringComparison(StringComparison comparisonType)

Conversation

ahsonkhan commented Mar 2, 2018

Uh oh!

ahsonkhan commented Mar 2, 2018

Uh oh!

ahsonkhan commented Mar 2, 2018

Uh oh!

jcagme commented Mar 2, 2018

Uh oh!

jcagme commented Mar 2, 2018

Uh oh!

jcagme commented Mar 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Mar 3, 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!

jcagme commented Mar 2, 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!

stephentoub Mar 2, 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!

ahsonkhan Mar 3, 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!

ahsonkhan Mar 3, 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!

Choose a reason for hiding this comment

Uh oh!

jcagme commented Mar 2, 2018

Uh oh!

jcagme commented Mar 3, 2018

Uh oh!

ahsonkhan commented Mar 3, 2018

Uh oh!

ahsonkhan commented Mar 3, 2018

Uh oh!

Reviewers

Assignees

Labels

ahsonkhan Mar 3, 2018 •

edited

Loading

stephentoub Mar 2, 2018 •

edited

Loading

ahsonkhan Mar 3, 2018 •

edited

Loading

ahsonkhan Mar 3, 2018 •

edited

Loading