Add MemoryExtensions to CoreLib along with necessary SpanHelpers #16521

ahsonkhan · 2018-02-23T02:58:26Z

Part of https://github.com/dotnet/corefx/issues/25182

Renamed the Span class (which contained static methods that were previously called from MemoryExtensions.Fast.cs) to SpanHelpers. Renamed the file from Span.NonGeneric.cs to SpanHelpers.cs. Changed the class to internal (and made the methods public).
The other SpanHelpers.*.cs files are identical to what was in corefx.
As part of this change, also renamed AsROSpan to AsSpan (related to Rename string-slicing extension methods corefx#27328).

Related PR: dotnet/corefx#27489

TODO before merging:

~~Figure out the mirroring for the relevant, duplicate source files between corefx/coreclr.~~

TODO outside this PR:

Cleanup StringSpanHelpers and uses in subsequent PR.

cc @jkotas, @KrzysztofCwalina, @stephentoub, @atsushikan, @dotnet/corefxlab-contrib

jkotas · 2018-02-23T03:07:20Z

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

@@ -635,7 +635,7 @@ internal static bool TryParseExactMultiple(ReadOnlySpan<char> input, string[] fo
        /// <summary>Common private Parse method called by both Parse and TryParse.</summary>
        private static bool TryParseTimeSpan(ReadOnlySpan<char> input, TimeSpanStandardStyles style, IFormatProvider formatProvider, ref TimeSpanResult result)
        {
-            input = input.Trim();


What was wrong with input.Trim() ?

We have added MemoryExtension methods (Trim/etc) and the call was ambiguous. I will clean up/remove StringSpanHelpers APIs in favor of MemoryExtension after this change goes through.

IMHO, it would be better to just delete StringSpanHelpers.Trim in this change instead of going back and forth.

There are other APIs from StringSpanHelpers we need to delete (and update uses). Most of the calls specify the class name and hence weren't ambiguous. I wanted to keep that change together (and isolated from this one).

Cleanup StringSpanHelpers and uses in subsequent PR.

I can do the cleanup change as part of this PR, but it would have to wait till tomorrow. Hope that's OK.

Bulk of the cleanup can wait for other PR. I just meant that we can do a little bit of the cleanup here to avoid useless changes.

jkotas · 2018-02-23T03:18:29Z

src/mscorlib/shared/System.Private.CoreLib.Shared.projitems

@@ -487,7 +489,10 @@
    <Compile Include="$(MSBuildThisFileDirectory)System\Span.cs" />
    <Compile Include="$(MSBuildThisFileDirectory)System\Span.Fast.cs" />
    <Compile Include="$(MSBuildThisFileDirectory)System\SpanDebugView.cs" />
-    <Compile Include="$(MSBuildThisFileDirectory)System\Span.NonGeneric.cs" />


I think you will need to keep the non-generic Span class temporary (with forwarders to the new implementations), so that things are not on the floor.

Even after this change #16520 propagates completely (which has these temporary APIs https://github.com/dotnet/coreclr/pull/16520/files#diff-34f82797d649dc710c6ff9268fa79c9bR691)?

I think so.

@jkotas, I have added back Span.NonGeneric but only kept a small subset of the APIs (specifically the ones that were recently renamed). Is that sufficient or do I need to add back all old APIs with forwarders to the impl in MemoryExtensions?

You just need to add enough to make CoreCLR CI green. If the CoreCLR CI is green with what you go, then it is fine.

It looks like there are several tests that end up calling Span.AsBytes and are failing.

I can't keep both Span.AsBytes and MemoryExtensions.AsBytes since they are extension methods, and would conflict, so should I rename AsBytes in MemoryExtensions for now? Or should I try to temporarily disable the subset of tests that end up calling AsBytes?

Rename AsBytes, to something like AsBytesExtension, in MemoryExtensions

Update use of AsBytes in corefx to AsBytesExtension

Remove Span.AsBytes and add MemoryExtensions.AsBytes

Update use of AsBytesExtension in corefx to AsBytes

Remove AsBytesExtension.

Is there a simpler approach?

Uses of AsBytes in corefx:
https://github.com/dotnet/corefx/blob/master/src/System.Private.Xml/src/System/Xml/NameTable.cs - ComputeHash32
https://github.com/dotnet/corefx/blob/master/src/System.Net.Primitives/src/System/Net/IPAddress.cs - GetHashCode

You should keep Span.AsBytes forwarder but make it non-extension method:

public static class Span { public static Span<byte> AsBytes<T>(Span<T> source) => source.AsBytes(); }

stephentoub · 2018-02-23T14:19:09Z

src/mscorlib/shared/System/Boolean.cs

@@ -181,7 +181,7 @@ public int CompareTo(Boolean value)
        public static Boolean Parse(String value)
        {
            if (value == null) throw new ArgumentNullException(nameof(value));
-            return Parse(value.AsReadOnlySpan());
+            return Parse(value.AsSpan());


@KrzysztofCwalina, FWIW, I prefer the old naming. It's possible I'm just used to it, but when I see the code in this PR, with AsSpan being passed to methods that accept a ReadOnlySpan, I immediately think "this is going through a cast and it's going to cost me something" (I've not looked to see if it does cost anything or not once the JIT is done with it). It also makes me think I'm getting back a mutable span even if I'm not, which makes reviewing code harder. And it'll lead to inconsistencies if we ever decide that we do want both AsSpan and As ReadOnlySpan exposed somewhere. I realize I may be in the minority, but I prefer the unabbreviated AsTargetType naming.

I concur. When I saw this I started digging back to find where the api-approved issue was so I could point out that we had said that it should be AsReadOnlySpan and we had explictly had issue with AsSpan. Then I saw that this was based off of a review meeting that I must not have been present for, and was marked as approved... and I sighed.

I expect AsFoo to give me a Foo, not a type sort of like Foo but with fewer members. And especially with variable names like input and people who really like var, it's misleading to a reviewer, since input.AsSpan() may or may not be "more access" than the code needs.

Yeah, there are pros and cons here. Naming is hard :-)

stephentoub · 2018-02-23T14:30:06Z

src/mscorlib/shared/System/MemoryExtensions.cs

+        public static ReadOnlyMemory<T> AsReadOnlyMemory<T>(this Memory<T> memory) => memory;
+
+        /// <summary>
+        /// 


Missing comment?

I was going to wait for Ati to add the API in corefx and copy over the xml comment.

stephentoub · 2018-02-23T15:00:18Z

src/mscorlib/shared/System/MemoryExtensions.Fast.cs

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


The approach seems to differ in some of these methods. Here the check is done at the very beginning. In https://github.com/dotnet/coreclr/pull/16521/files#diff-a42ac5dcb8522689822880cf2e02161dR304, the check is done as part of the switch in a default cast, and then it's also done in the fast-path return branches up front, presumably to avoid doing a double-validation for the common case. Why the different approaches?

I followed the approach from here (to be consistent): #16434

stephentoub · 2018-02-23T15:02:13Z

src/mscorlib/shared/System/MemoryExtensions.cs

+        /// <summary>
+        /// Determines the relative order of the sequences being compared by comparing the elements using IComparable{T}.CompareTo(T). 
+        /// </summary>
+        public static int SequenceCompareTo<T>(this ReadOnlySpan<T> first, ReadOnlySpan<T> second)


Pretty much every other method on this type has AggressiveInlining set. Is there a reason this one is different?

I don't see a reason why. This was copy/pasted. I will fix the inconsistency.

stephentoub · 2018-02-23T15:06:06Z

src/mscorlib/shared/System/SpanHelpers.cs

+
+                // PERF: This depends on the fact that the String objects are always zero terminated 
+                // and that the terminating zero is not included in the length. For even string sizes
+                // this compare can include the zero terminator. Bitwise OR avoids a branch.


This comment is incorrect for spans. Is this check actually relying on that? If so, it's buggy.

stephentoub · 2018-02-23T15:06:41Z

src/mscorlib/shared/System/SpanHelpers.cs

+            if (pointerSizeLength == 0)
+                return;
+
+            // TODO: Perhaps do switch casing to improve small size perf


Is there an issue for this TODO?

This file was essentially a rename of the existing Span.NonGenerics.

There is an issue here: https://github.com/dotnet/corefx/issues/26604.

I will add additional context to the issue.

stephentoub · 2018-02-23T15:07:11Z

src/mscorlib/shared/System/SpanHelpers.cs

+            Unsafe.InitBlockUnaligned(ref b, 0, (uint)byteLength);
+            return;
+#else
+            // TODO: Optimize other platforms to be on par with AMD64 CoreCLR


Is there an issue for this TODO?

Filed - https://github.com/dotnet/coreclr/issues/16614

…moryExtensions

ahsonkhan · 2018-02-27T04:12:57Z

Any other feedback before this gets merged and mirrored?

jkotas · 2018-02-27T13:46:02Z

tests/src/JIT/Performance/CodeQuality/Span/SpanBench.cs

@@ -928,6 +928,7 @@ static void TestArrayClear<T>(T[] array, int length, int iterationCount)
        #endregion

        #region TestSpanAsBytes<T>
+#if false


You should not need this change now.

AndyAyersMS · 2018-02-27T18:04:28Z

@ahsonkhan I was about to re-enable those parts of span bench. I'll let you take over.

ahsonkhan · 2018-02-27T18:22:04Z

I was about to re-enable those parts of span bench. I'll let you take over.

This PR isn't touching the SpanBench. Please continue your PR related to SpanBench. I can take over separately, if you prefer.

AndyAyersMS · 2018-02-27T18:25:50Z

No, I'll take care of it.

Add MemoryExtensions to CoreLib along with necessary SpanHelpers

1b7ee40

ahsonkhan added * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) area-System.Memory labels Feb 23, 2018

ahsonkhan self-assigned this Feb 23, 2018

ahsonkhan requested review from jkotas, stephentoub and a user and removed request for jkotas February 23, 2018 02:58

Make the newly added AsSpan/AsMemory into array extension methods.

fe2c6e2

jkotas reviewed Feb 23, 2018

View reviewed changes

ahsonkhan added 2 commits February 22, 2018 19:44

Remove StringSpanHelpers.Trim

80f5396

Leftover AsReadOnlySpan -> AsSpan for *Unix.cs files

abe6aba

stephentoub reviewed Feb 23, 2018

View reviewed changes

jkotas mentioned this pull request Feb 24, 2018

Remove reflection in ValueType.Equals/GetHashCode dotnet/corert#5436

Merged

ahsonkhan added 3 commits February 26, 2018 15:21

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

b112c83

…moryExtensions

Address PR feedback.

9654100

Remove duplicate methods in the Span class.

bd6a801

ahsonkhan removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 27, 2018

ahsonkhan mentioned this pull request Feb 27, 2018

Removing files since MemoryExtensions is in CoreLib now. dotnet/corefx#27489

Closed

Temporarily disable AsBytes SpanBench test.

03b1545

Add back AsBytes

6830cfb

jkotas reviewed Feb 27, 2018

View reviewed changes

Re-enable AsBytes SpanBench test

2f63e02

ahsonkhan merged commit a3ecc73 into dotnet:master Feb 27, 2018

ahsonkhan deleted the AddMemoryExtensions branch February 27, 2018 20:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MemoryExtensions to CoreLib along with necessary SpanHelpers #16521

Add MemoryExtensions to CoreLib along with necessary SpanHelpers #16521

ahsonkhan commented Feb 23, 2018 •

edited

Loading

jkotas Feb 23, 2018

ahsonkhan Feb 23, 2018

jkotas Feb 23, 2018

ahsonkhan Feb 23, 2018

jkotas Feb 23, 2018

jkotas Feb 23, 2018

ahsonkhan Feb 23, 2018

jkotas Feb 23, 2018

ahsonkhan Feb 27, 2018

jkotas Feb 27, 2018

ahsonkhan Feb 27, 2018

jkotas Feb 27, 2018 •

edited

Loading

stephentoub Feb 23, 2018

bartonjs Feb 23, 2018

KrzysztofCwalina Feb 23, 2018

stephentoub Feb 23, 2018

ahsonkhan Feb 23, 2018

stephentoub Feb 23, 2018

ahsonkhan Feb 23, 2018

stephentoub Feb 23, 2018

ahsonkhan Feb 23, 2018

stephentoub Feb 23, 2018

stephentoub Feb 23, 2018

ahsonkhan Feb 23, 2018

stephentoub Feb 23, 2018

ahsonkhan Feb 27, 2018

ahsonkhan commented Feb 27, 2018

jkotas Feb 27, 2018

AndyAyersMS commented Feb 27, 2018

ahsonkhan commented Feb 27, 2018

AndyAyersMS commented Feb 27, 2018

Add MemoryExtensions to CoreLib along with necessary SpanHelpers #16521

Add MemoryExtensions to CoreLib along with necessary SpanHelpers #16521

Conversation

ahsonkhan commented Feb 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

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

Choose a reason for hiding this comment

jkotas Feb 27, 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

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

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

ahsonkhan commented Feb 27, 2018

Choose a reason for hiding this comment

AndyAyersMS commented Feb 27, 2018

ahsonkhan commented Feb 27, 2018

AndyAyersMS commented Feb 27, 2018

ahsonkhan commented Feb 23, 2018 •

edited

Loading

jkotas Feb 27, 2018 •

edited

Loading