Misc Span/Memory changes #15941

ianhays · 2018-01-20T00:09:52Z

progress towards https://github.com/dotnet/corefx/issues/26368, https://github.com/dotnet/corefx/issues/24854, https://github.com/dotnet/corefx/issues/26295, https://github.com/dotnet/corefx/issues/26139

Note that this includes breaking changes to any upstream projects using these functions. This means it will break CoreFX builds dependent on any new coreclr produced until an accompanying PR in dotnet/corefx/ is merged.

ahsonkhan · 2018-01-20T01:59:07Z

src/mscorlib/shared/System/ReadOnlySpan.cs

+        /// <summary>
+        /// 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>


nit: Is the returns xml tag necessary? It seems redundant with whatever has been stated in the summary anyway.

It's definitely redundant. I added it because I thought that intellisense may do something useful with the returns tag, but I couldn't actually find a case where it's used without the summary. I'll remove it.

ahsonkhan · 2018-01-20T02:01:13Z

src/mscorlib/shared/System/Runtime/InteropServices/MemoryMarshal.cs

+        /// Thrown if the Length property of the new Span would exceed Int32.MaxValue.
+        /// </exception>
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        public static ReadOnlySpan<TTo> Cast<TFrom, TTo>(ReadOnlySpan<TFrom> source)


This looks like a duplicate.

Thanks, this should be casting Span

ahsonkhan · 2018-01-20T02:01:21Z

src/mscorlib/shared/System/Runtime/InteropServices/MemoryMarshal.cs

+        /// Thrown if the Length property of the new Span would exceed Int32.MaxValue.
+        /// </exception>
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        public static ReadOnlySpan<TTo> Cast<TFrom, TTo>(ReadOnlySpan<TFrom> source)


Same here. This looks like a duplicate.

ahsonkhan

Otherwise, LGTM.

jkotas · 2018-01-20T04:11:37Z

src/mscorlib/shared/System/Runtime/InteropServices/MemoryMarshal.cs

+                ThrowHelper.ThrowInvalidTypeWithPointersNotSupported(typeof(TTo));
+
+            return new ReadOnlySpan<TTo>(
+                ref Unsafe.As<TFrom, TTo>(ref MemoryMarshal.GetReference(source)),


This can use _pointer.Value directly. No need to go through GetReference wrapper.

jkotas · 2018-01-20T04:12:56Z

It was not clear to me from the discussions that we are adding MemoryMarshal.ToEnumerable (I thought we have abandoned it as perf trap) and Memory.Cast (I thought that we will add TryCast instead). @KrzysztofCwalina ?

Otherwise LGTM - once you make the CI green.

ahsonkhan · 2018-01-20T04:19:26Z

and Memory.Cast (I thought that we will add TryCast instead).

From our API review today: [starts at 13:30] https://youtu.be/Qcqn-yv1X0o?t=13m30s
We decided to add MemoryMarshal.Cast to enable cases where the user explicitly aligns first (for example, Span.IndexOf), and then wants to avoid the unnecessary checks that TryCast would do. Hence, we decided to keep Cast (on MemoryMarshal) and add a TryCast extension method, for the common case (with higher visibility).

stephentoub · 2018-01-22T12:38:50Z

src/mscorlib/shared/System/ReadOnlySpan.cs

-        /// <param name="length">The number of <typeparamref name="T"/> elements the memory contains.</param>
-        [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        [EditorBrowsable(EditorBrowsableState.Never)]
-        public static ReadOnlySpan<T> DangerousCreate(object obj, ref T objectData, int length) => new ReadOnlySpan<T>(ref objectData, length);


At least from my experience, it's easier to stage changes like this across coreclr/corefx if when changing a method's name/location, you first add the new method in coreclr, let that propagate to corefx and fix everything up, and then go back and delete the old method. Otherwise, there's a period of time where coreclr and corefx don't work together, and if there's any other issue happening at the same time, you may not be able to fix things quickly.

Great idea! I wasn't too pleased with the broken in-between phase. I'll do that.

stephentoub · 2018-01-22T12:40:48Z

src/mscorlib/shared/System/ReadOnlySpan.cs

+        /// 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}]";


Performance here isn't important, but FYI this is more expensive than just:

"System.Span[" + Length.ToString() + "]";

The string interpolation version will box Length, will create an array to store that boxed length, and then will end up calling string.Format, whereas the above allocates the string for length and then calls String.Concat(string, string, string).

Good to know, thanks :)

stephentoub · 2018-01-22T12:41:55Z

src/mscorlib/shared/System/Runtime/InteropServices/MemoryMarshal.cs

+            int newLength = checked((int)((long)source.Length * Unsafe.SizeOf<TFrom>() / Unsafe.SizeOf<TTo>()));
+            return new ReadOnlySpan<TTo>(Unsafe.As<Pinnable<TTo>>(source.Pinnable), source.ByteOffset, newLength);
+        }
+
 #else


Why do we have both the portable and fast versions in the same file? Can we split this into separate files like we do elsewhere?

Sure. I'll have to temporarily have four files to prevent corefx from breaking before the accompanying PR there is merged:

MemoryMarshal.Fast.cs

MemoryMarshal.Portable.cs

MemoryMarshal.Shared.cs (only the few functions that are the same for both fast/portable)

MemoryMarshal.cs (old file without changes)

Why do you need to have MemoryMarshal.Shared.cs file? I think these can be just be added to the existing MemoryMarshal.cs file.

The purpose of the Shared file is to make it so that merging the coreclr PR won't cause CoreFX to break. To do that, we need to mostly leave the existing MemoryMarshal file intact, which means we can't include it in coreclr if we also have a MemoryMarshal.Fast that contains some of the signatures that are currently living in MemoryMarshal.

There are two cases:

You are adding new APIs (including adding overloads of existing APIs with new signatures). In this case, you can just add it to MemoryMarshal.cs

You add modifying existing APIs. In this case, you will need to augment the mirror PR in CoreFX to get it through. MemoryMarshal.Shared.cs file is not going to help you with that.

What I am missing?

You add modifying existing APIs. In this case, you will need to augment the mirror PR in CoreFX to get it through. MemoryMarshal.Shared.cs file is not going to help you with that.

The way I have it currently set up, I'm not actually modifying any existing APIs, I'm only adding new API, so the mirror PR won't be broken.

You are adding new APIs (including adding overloads of existing APIs with new signatures). In this case, you can just add it to MemoryMarshal.cs

If I add the new API to MemoryMarshal, then coreclr needs to include MemoryMarshal, which means I'll need to remove the Fast-Specific section from MemoryMarshal to avoid conflicts with MemoryMarshal.Fast.cs, which means that the mirror PR to corefx will be broken for the non-portable builds unless I modify the mirror PR to include the MemoryMarshal.Fast file.

I was trying to avoid having to make fixes to the mirror PR, but if you're strongly against the Shared suffix, then I can. I'll just fix the mirror PR to include the new MM.Fast and MM.Portable files to get the build green then keep my feature CoreFX PR the same minus the new file inclusions.

you're strongly against the Shared suffix

Yes, I am strongly against adding temporary files with .Shared suffix. It just means more round trips between repos to clean things up.

mirror PR to corefx will be broken for the non-portable builds

We should expect that you need to patch up the mirror PR for changes like these.

👍 Shared removed.

stephentoub · 2018-01-22T12:43:57Z

src/mscorlib/shared/System/Runtime/InteropServices/MemoryMarshal.cs

+        /// <typeparam name="T">The element type of the <paramref name="memory" />.</typeparam>
+        /// <param name="memory">The Memory to view as an <see cref="IEnumerable{T}"/></param>
+        /// <returns>An <see cref="IEnumerable{T}"/> view of the given <paramref name="memory" /></returns>
+        public static IEnumerable<T> ToEnumerable<T>(Memory<T> memory)


Does this end up having the desired optimization? I would hope so, as Environment.CurrentManagedThreadId is available to it, but the Environment it has access to is also internal, so we should verify that's not throwing off the compiler.

@stephentoub Environment.CurrentManagedThreadId is not used here. Was this meant for something else?

It's used by the compiler's implementation of iterators, and the compiler keys off its presence to determine whether to take the optimization that reuses the enumerable as the enumerator.

(I expect it's fine though, as corelib already has other iterators in it, and they look to compile "correctly".)

I checked the IL and confirmed that it is getting compiled correctly with the use of System.Environment::get_CurrentManagedThreadId()

stephentoub · 2018-01-22T12:44:58Z

src/mscorlib/shared/System/Runtime/InteropServices/MemoryMarshal.cs

+        /// <typeparam name="T">The element type of the <paramref name="memory" />.</typeparam>
+        /// <param name="memory">The Memory to view as an <see cref="IEnumerable{T}"/></param>
+        /// <returns>An <see cref="IEnumerable{T}"/> view of the given <paramref name="memory" /></returns>
+        public static IEnumerable<T> ToEnumerable<T>(Memory<T> memory)


Should this be for ReadOnlyMemory<T> instead of for Memory<T>?

Seems like it might as well be, yeah.

- Add MemoryMarshal.CreateSpan to replace DangerousCreate - Add MemoryMarshal.CreateReadOnlySpan to replace DangerousCreate - Add MemoryMarshal.Cast to replace NonPortableCast - Add ToString override to Span and ReadOnlySpan - Add ToEnumerable function to MemoryMarshal that takes a ReadOnlyMemory

ianhays · 2018-01-22T18:22:23Z

Thanks everyone for the feedback. I force pushed a new commit that addresses of all it and also leaves the existing functions in place to ease the intermediate stage per @stephentoub 's suggestion.

jkotas · 2018-01-22T18:23:42Z

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

@@ -435,7 +435,8 @@
    <Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\LayoutKind.cs" />
    <Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\MarshalAsAttribute.cs" />
    <Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\MarshalDirectiveException.cs" />
-    <Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\MemoryMarshal.cs" />
+    <Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\MemoryMarshal.Fast.cs" />
+    <Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\MemoryMarshal.Shared.cs" />


This should stay MemoryMarshal.cs to follow our naming conventions.

jkotas · 2018-01-22T18:25:42Z

src/mscorlib/shared/System/Runtime/InteropServices/MemoryMarshal.Portable.cs

+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+


MemoryMarshal.Portable.cs does not need to be in the shared CoreLib subtree. It can stay in the CoreFX-specific subtree.

https://github.com/dotnet/coreclr/tree/master/src/mscorlib/shared#conventions

Good point, removed. I'll add it back to CoreFX in the System.Memory project.

jkotas

LGTM. Thanks!

* Misc Span/Memory additions - Add MemoryMarshal.CreateSpan to replace DangerousCreate - Add MemoryMarshal.CreateReadOnlySpan to replace DangerousCreate - Add MemoryMarshal.Cast to replace NonPortableCast - Add ToString override to Span and ReadOnlySpan - Add ToEnumerable function to MemoryMarshal that takes a ReadOnlyMemory Signed-off-by: dotnet-bot <[email protected]>

* Misc Span/Memory additions - Add MemoryMarshal.CreateSpan to replace DangerousCreate - Add MemoryMarshal.CreateReadOnlySpan to replace DangerousCreate - Add MemoryMarshal.Cast to replace NonPortableCast - Add ToString override to Span and ReadOnlySpan - Add ToEnumerable function to MemoryMarshal that takes a ReadOnlyMemory Signed-off-by: dotnet-bot-corefx-mirror <[email protected]>

* Misc Span/Memory additions - Add MemoryMarshal.CreateSpan to replace DangerousCreate - Add MemoryMarshal.CreateReadOnlySpan to replace DangerousCreate - Add MemoryMarshal.Cast to replace NonPortableCast - Add ToString override to Span and ReadOnlySpan - Add ToEnumerable function to MemoryMarshal that takes a ReadOnlyMemory Signed-off-by: dotnet-bot <[email protected]>

* Misc Span/Memory additions - Add MemoryMarshal.CreateSpan to replace DangerousCreate - Add MemoryMarshal.CreateReadOnlySpan to replace DangerousCreate - Add MemoryMarshal.Cast to replace NonPortableCast - Add ToString override to Span and ReadOnlySpan - Add ToEnumerable function to MemoryMarshal that takes a ReadOnlyMemory Signed-off-by: dotnet-bot-corefx-mirror <[email protected]>

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 review from ahsonkhan and jkotas January 20, 2018 00:09

ianhays mentioned this pull request Jan 20, 2018

Consume Span moves dotnet/corefx#26467

Merged

ahsonkhan reviewed Jan 20, 2018

View reviewed changes

ahsonkhan approved these changes Jan 20, 2018

View reviewed changes

jkotas reviewed Jan 20, 2018

View reviewed changes

stephentoub reviewed Jan 22, 2018

View reviewed changes

jkotas suggested changes Jan 22, 2018

View reviewed changes

jkotas reviewed Jan 22, 2018

View reviewed changes

Remove MemoryMarshal.Shared

ec3a246

jkotas approved these changes Jan 22, 2018

View reviewed changes

jkotas merged commit c69e814 into dotnet:master Jan 23, 2018

ianhays mentioned this pull request Jan 29, 2018

Remove replaced Span/ReadOnlySpan methods. #16086

Closed

AndyAyersMS mentioned this pull request Feb 2, 2023

JIT: optimize away overflow check in some span non portable casts dotnet/runtime#9426

Open

Misc Span/Memory changes #15941

Misc Span/Memory changes #15941

Conversation

ianhays commented Jan 20, 2018

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas commented Jan 20, 2018

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

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

ianhays commented Jan 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas left a comment

Choose a reason for hiding this comment

ahsonkhan commented Jan 20, 2018 •

edited

Loading

jkotas Jan 22, 2018 •

edited

Loading