Consume Span moves by ianhays · Pull Request #26467 · dotnet/corefx

ianhays · 2018-01-20T00:13:19Z

Move Span.DangerousCreate to MemoryMarshal.CreateSpan
Move ReadOnlySpan.DangerousCreate to MemoryMarshal.CreateReadOnlySpan
Move Span.NonPortableCast to MemoryMarshal.Cast
Move ReadOnlySpan.NonPortableCast to MemoryMarshal.Cast
Add ToString override to Span and ReadOnlySpan
Add ToEnumerable function to MemoryMarshal that takes a Memory
Since the above require adding a fast-span specific API (the new Create functions), I had to add netcoreapp and uap configurations to the test build so we only run the Create tests against the fast-span.

resolves #26368
resolves #24854
resolves #26295
resolves #26139

ahsonkhan · 2018-01-20T02:05:32Z

    <!-- disable warnings about obsolete APIs -->
    <NoWarn>$(NoWarn);0809</NoWarn>
    <IsPartialFacadeAssembly Condition="'$(TargetGroup)' == 'netcoreapp' Or '$(TargetGroup)' == 'uap'">true</IsPartialFacadeAssembly>
+    <DefineConstants Condition="'$(IsPartialFacadeAssembly)' != 'true'">$(DefineConstants);netstandard;FEATURE_PORTABLE_SPAN</DefineConstants>


Can we remove the netstandard constant?

ahsonkhan · 2018-01-20T02:08:03Z

    {
        [Fact]
-        public static void DangerousCreate1()
+        public static void CreateReadOnlySpan1()


nit: remove the 1 suffix from the test name.

ahsonkhan · 2018-01-20T02:08:24Z

    {
        [Fact]
-        public static void DangerousCreate1()
+        public static void CreateSpan1()


Same here. Not sure why it was DangerousCreate1 to begin with.

Also elsewhere.

ahsonkhan · 2018-01-20T02:10:57Z

    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
    <ProjectGuid>{15DB0DCC-68B4-4CFB-8BD2-F26BCCAF5A3F}</ProjectGuid>
+    <IsPartialFacadeAssembly Condition="'$(TargetGroup)' == 'netcoreapp' OR '$(TargetGroup)' == 'uap'">true</IsPartialFacadeAssembly>
+    <DefineConstants Condition="'$(IsPartialFacadeAssembly)' != 'true'">$(DefineConstants);netstandard;FEATURE_PORTABLE_SPAN</DefineConstants>


These constants seem unnecessary (not used).

Good point. I added them to keep the new tests in a single file and used IFDEFS but decided that was cumbersome so broke them up into group-specific files included based on IsPartialFacadeAssembly.

I'll remove the DefineConstants.

ahsonkhan

We need to add the ToString() methods to the System.Memory ref as well (for Span/ReadOnlySpan).

ahsonkhan · 2018-01-20T02:14:00Z

        public int Length { get { throw null; } }
        public void CopyTo(System.Span<T> destination) { }
        [System.ComponentModel.EditorBrowsableAttribute((System.ComponentModel.EditorBrowsableState)(1))]
-        public static System.ReadOnlySpan<T> DangerousCreate(object obj, ref T objectData, int length) { throw null; }


All these changes need to be made to the System.Runtime ref as well.

ianhays · 2018-01-24T17:46:29Z

@ahsonkhan thanks for the feedback! I addressed all of it.

ianhays · 2018-01-24T21:52:11Z

cc: @GrabYourPitchforks @KrzysztofCwalina

ianhays · 2018-01-26T20:13:53Z

Pinging @KrzysztofCwalina @GrabYourPitchforks @ahsonkhan @stephentoub for final feedback.

KrzysztofCwalina · 2018-01-26T22:47:43Z

+            if (SpanHelpers.IsReferenceOrContainsReferences<TTo>())
+                ThrowHelper.ThrowArgumentException_InvalidTypeWithPointersNotSupported(typeof(TTo));
+
+            int newLength = checked((int)((long)source.Length * Unsafe.SizeOf<TFrom>() / Unsafe.SizeOf<TTo>()));


Should we check that TTo's size is more than zero? i.e. what happens if I cast from span of bytes to a span of empty structs?

Unsafe.SizeOf<TTo> returns a nonzero value for all types.

Added this test:

struct EmptyStruct { } [Fact] public static void CastSpanToEmptyStruct() { Span<uint> span = new Span<uint>(new uint[] { 1 }); Span<EmptyStruct> emptyspan = MemoryMarshal.Cast<uint, EmptyStruct>(span); Assert.Equal(1, Unsafe.SizeOf<EmptyStruct>()); Assert.Equal(4, emptyspan.Length); }

ahsonkhan · 2018-01-27T03:10:56Z

    <Compile Include="MemoryMarshal\AsMemory.cs" />
    <Compile Include="MemoryMarshal\GetReference.cs" />
    <Compile Include="MemoryMarshal\TryGetArray.cs" />
+    <Compile Include="MemoryMarshal\CastReadOnlySpan.cs" />


nit: sort order as alphabetical

ahsonkhan

Otherwise, lgtm.

AndyAyersMS · 2018-01-29T07:38:44Z

The checked multiply pattern in Cast generates some ugly code which can be avoided in many cases by doing some case analysis.

See notes over in dotnet/coreclr#15492.

If we're going to leave it as is, then I'd get rid of the aggressive inlining attribute.

- [Move Span.DangerousCreate to MemoryMarshal.CreateSpan](#26139) - [Move ReadOnlySpan.DangerousCreate to MemoryMarshal.CreateReadOnlySpan](#26139) - [Move Span.NonPortableCast to MemoryMarshal.Cast](#26368) - [Move ReadOnlySpan.NonPortableCast to MemoryMarshal.Cast](#26368) - [Add ToString override to Span and ReadOnlySpan](#26295) - [Add ToEnumerable function to MemoryMarshal that takes a Memory](#24854)

ahsonkhan · 2018-01-29T19:10:57Z

@ianhays, can you please maintain the commit history during PR review? It makes it easier to view only the diffs between commits.

ahsonkhan · 2018-01-29T19:16:54Z

+        /// Returns a <see cref="String"/> with the name of the type and the number of elements
+        /// </summary>
+        /// <returns>A <see cref="String"/> with the name of the type and the number of elements</returns>
+        public override string ToString() => $"System.Span[{Length}]";


Should this be identical to the DebuggerDisplay string (i.e. should we include typeof(T).Name to ToString) - SpanDebuggerDisplay / ReadOnlySpanDebuggerDisplay?

Similarly, should we add the prefix "System.Span" to the DebuggerDisplay, with angle brackets around the type T?

For instance, I propose the following:

Span<int> span = new int[10]; string str = span.ToString(); //System.Span<int>[10]

Sure, that would be useful.

Updated with the new ToString/DebuggerDisplay string.

I'll post a PR to coreclr with the new strings to FastSpan along with the removal of the old NonPortableCast functions after this PR in corefx is merged.

ianhays · 2018-01-29T20:39:43Z

can you please maintain the commit history during PR review? It makes it easier to view only the diffs between commits.

Apologies, I wasn't expecting any more feedback. That rebase was to remove the feedback commits pre-merge.

ahsonkhan

Looks good!

ianhays · 2018-01-29T23:01:40Z

Thanks for the feedback! merged.

Consume Span moves - [Move Span.DangerousCreate to MemoryMarshal.CreateSpan](dotnet/corefx#26139) - [Move ReadOnlySpan.DangerousCreate to MemoryMarshal.CreateReadOnlySpan](dotnet/corefx#26139) - [Move Span.NonPortableCast to MemoryMarshal.Cast](dotnet/corefx#26368) - [Move ReadOnlySpan.NonPortableCast to MemoryMarshal.Cast](dotnet/corefx#26368) - [Add ToString override to Span and ReadOnlySpan](dotnet/corefx#26295) - [Add ToEnumerable function to MemoryMarshal that takes a Memory](dotnet/corefx#24854) Commit migrated from dotnet/corefx@540a99c

ianhays added the area-System.Memory label Jan 20, 2018

ianhays added this to the 2.1.0 milestone Jan 20, 2018

ianhays self-assigned this Jan 20, 2018

ianhays requested a review from ahsonkhan January 20, 2018 00:13

ahsonkhan reviewed Jan 20, 2018

View reviewed changes

ahsonkhan suggested changes Jan 20, 2018

View reviewed changes

ahsonkhan reviewed Jan 20, 2018

View reviewed changes

KrzysztofCwalina reviewed Jan 26, 2018

View reviewed changes

ahsonkhan reviewed Jan 27, 2018

View reviewed changes

ahsonkhan approved these changes Jan 27, 2018

View reviewed changes

ahsonkhan reviewed Jan 29, 2018

View reviewed changes

Update

327d67d

ahsonkhan approved these changes Jan 29, 2018

View reviewed changes

ianhays merged commit 540a99c into dotnet:master Jan 29, 2018

This was referenced Jan 29, 2018

Remove replaced Span/ReadOnlySpan methods. dotnet/coreclr#16086

Closed

Update Span/ReadOnlySpan strings to match Portable Span dotnet/coreclr#16087

Merged

Enable Span.ToString tests #26663

Closed

ahsonkhan mentioned this pull request Jan 31, 2018

Update use of NonPortableCast to MemoryMarshal.Cast and cleanup dotnet/corefxlab#2092

Merged

ianhays deleted the memory_moves branch June 11, 2018 17:55

Uh oh!

Conversation

ianhays commented Jan 20, 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!

ahsonkhan Jan 20, 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!

ahsonkhan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ianhays commented Jan 24, 2018

Uh oh!

ianhays commented Jan 24, 2018

Uh oh!

ianhays commented Jan 26, 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!

ahsonkhan left a comment

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS commented Jan 29, 2018

Uh oh!

ahsonkhan commented Jan 29, 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!

ianhays commented Jan 29, 2018

Uh oh!

ahsonkhan left a comment

Choose a reason for hiding this comment

Uh oh!

ianhays commented Jan 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ahsonkhan Jan 20, 2018 •

edited

Loading