Add Span/ReadOnlySpan TryCast #26662

ianhays · 2018-01-29T23:37:20Z

Exposes Span.TryCast and ReadOnlySpan.TryCast as extension methods in MemoryExtensions.
Implemented for both the fast and portable implementations in equivalent ways, though eventually the fast implementation should be moved to corelib with the other MemoryExtensions.Fast methods and implemented using compiler directives (e.g. #IF intelProcessorCompiled) or maybe an intrinsic for the else case.
I decided that requiring a fixed memory location for the source span would be too restrictive, so I implemented the functions in a way that works even if the memory pointed to by the span gets moved mid-operation or post-operation. The downside to this is that we can't handle the "accidentally aligned" case as well, but this is a worthy tradeoff in my opinion. More details in the doc comments for TryCast.
Added a bunch of tests for TryCast that assert the behavior for param types of various sizes and alignment.
This code is based around two large assumptions:
- The alignment rules for TFrom will be followed if sourceSpan is moved.
- The maximum alignment size of any data item is 8 bytes. For C# this is true for all primitives, including decimal.

resolves https://github.com/dotnet/corefx/issues/26465

cc: @stephentoub @ahsonkhan @KrzysztofCwalina @GrabYourPitchforks

…in MemoryExtensions. - Implemented for both the fast and portable implementations in equivalent ways, though eventually [the fast implementation should be moved to corelib with the other MemoryExtensions.Fast methods](https://github.com/dotnet/corefx/issues/25182) and implemented using compiler directives (e.g. #IF intelProcessorCompiled) or maybe an intrinsic for the else case. - I decided that requiring a fixed memory location for the source span would be too restrictive, so I implemented the functions in a way that works even if the memory pointed to by the span gets moved mid-operation or post-operation. The downside to this is that we can't handle the "accidentally aligned" case as well, but this is a worthy tradeoff in my opinion. More details in the doc comments for TryCast. - Added a bunch of tests for TryCast that assert the behavior for param types of various sizes and alignment. - This code is based around two large assumptions: - The alignment rules for TFrom will be followed if sourceSpan<TFrom> is moved. - The maximum alignment size of any data item is 8 bytes. For C# this is true for all primitives, including decimal.

KrzysztofCwalina · 2018-01-29T23:56:35Z

src/System.Memory/ref/System.Memory.cs

@@ -75,6 +75,8 @@ public static partial class MemoryExtensions
        public static System.ReadOnlySpan<char> TrimStart(this System.ReadOnlySpan<char> span, char trimChar) { throw null; }
        public static System.ReadOnlySpan<char> TrimStart(this System.ReadOnlySpan<char> span, System.ReadOnlySpan<char> trimChars) { throw null; }
        public static bool TryGetString(this System.ReadOnlyMemory<char> readOnlyMemory, out string text, out int start, out int length) { throw null; }
+        public static bool TryCast<TFrom, TTo>(this ReadOnlySpan<TFrom> source, out ReadOnlySpan<TTo> output) where TFrom : struct where TTo : struct { throw null; }


As we talked in the other thread, it would be better to make these instance methods. I am not sure if you'd like to merge this and then separately DCR, or make the API change now.

jkotas · 2018-01-30T00:26:27Z

src/System.Memory/src/System/MemoryExtensions.Fast.cs

+                    // This check also returns false if it's coincidentally aligned to TTo in the case where sizeof(TFrom)<sizeof(TTo). This is
+                    // because the source location can move such that it no longer adheres to the alignment requirement of TTo while still
+                    // following the alignment rules for it's declared/constructed type, TFrom.
+                    void* pointer = Unsafe.AsPointer(ref MemoryMarshal.GetReference(source));


The GC can move the objects around. For example - on ARM, the byref that was 8-byte aligned now can be 8-byte misaligned a moment later.

Interesting. I thought that when GC moves objects, it maintains word alignment.

Depends on the GC implementation...

This code is based around two large assumptions:
The alignment rules for TFrom will be followed if sourceSpan is moved.

This is one of the assumptions made in this implementation.

for example - on ARM, the byref that was 8-byte aligned now can be 8-byte misaligned a moment later.

So you're saying that on ARM an array of 8-byte aligned TFrom's can be moved by the GC to a location such that it is no longer 8-byte aligned? Wouldn't that cause any subsequent valid TFrom accesses to fail, since unaligned access is unsupported? How does anything work when on a unaligned-unsupported architecture if the GC is breaking the alignment rules when it moves blocks?

ARM an array of 8-byte aligned TFrom's can be moved by the GC to a location such that it is no longer 8-byte aligned

Right.

How does anything work when on a unaligned-unsupported architecture if the GC is breaking the alignment rules when it moves blocks?

The GC is not breaking any rules. The GC knows what the array type is. It will only do it when it is safe (e.g. when the array type is byte).

It will only do it when it is safe (e.g. when the array type is byte).

Are you talking about the case in which the array type that the GC is aware of has a different alignment than the TFrom that is given to TryCast?

I am operating on the assumption that the alignment of TFrom is equivalent to the alignment that the GC is aware of. If they're different then the source Span is already subject to failed unaligned accesses and so our TryCast isn't making anything worse by allowing a cast from an invalid type to another (potentially) invalid type.

So under that assumption, the GC will either:

maintain the alignment rules of TFrom

fail whether we allow the cast or not, in which case we might as well allow it.

Consider this:

// Note this type is guaranteed to be 4-byte aligned. There is nothing forcing it to be 8-byte aligned. struct Bundle { int x,y; } Bundle[] a = new Bundle[10]; // Let's assume that the GC will happen to allocate the array on 8-byte boundary Span<Bundle> s = new Span<Bundle>(a); bool success = s.TryCast<Bundle,double>(out Span<double> sd); // This will succeed //GC moves `a` to a different place that is not 8-byte aligned anymore ... double d = sd[5]; // Misaligned access fault

I see, thanks for the example. We will return the wrong result in the case where TFrom is a struct in which all of these conditions are met:

Unsafe.SizeOf<TFrom> != actualTFromAlignment

actualTFromAlignment < Unsafe.SizeOf<TTo>

addressof(source) % Unsafe.SizeOf<TTo> == 0

This is essentially the reverse of the issue I commented about above:

// This check will return false in the case where TTo is a struct that is 8 bytes long but only 1/2/4-byte aligned and the source pointer // is 1/2/4 byte-aligned but not also 8-byte aligned. Unfortunately, to catch that case we would have to walk the // struct element sizes to determine the actual struct alignment.

Except in this case we return true incorrectly while in the other case we return false incorrectly, which is of course a more severe error.

Both are the result of the difference between SizeOf and SizeOfLargestElement. I'm not aware of a way to get the latter without reflection.

How do you feel about:

int sizeOfTFrom = typeof(TFrom).IsPrimitive ? Math.Min(8, Unsafe.SizeOf<TFrom>()) : 1;

Without being able to look into the struct to determine the real alignment, we can't really assume it follows any alignment but byte alignment, which means TryCast will pretty much always return false when TFrom is a struct.

It will make the API slower for the common use case of casting span of bytes to span of chars; or span of ints.

I think it is a good thing for the portable version of the API to be more conservative. It will tech folks to write proper fallbacks for this.

jkotas · 2018-01-30T00:35:44Z

src/System.Memory/src/System/MemoryExtensions.Fast.cs

+            if (RuntimeHelpers.IsReferenceOrContainsReferences<TTo>())
+                ThrowHelper.ThrowArgumentException_InvalidTypeWithPointersNotSupported(typeof(TTo));
+
+            if (RuntimeInformation.OSArchitecture == Architecture.X64 ||


This should to check ProcessArchitecture, not OSArchitecture.

(You pretty much never want to use Runtime.OSArchitecture property. It is only useful if you want to deal with Windows WoW quirks that is very rare.)

And the implementation of either of these API is dog slow (each invocation takes a lock)...

And the implementation of either of these API is dog slow (each invocation takes a lock)...

Is there a better way to check the processor architecture in a fully portable implementation?

ahsonkhan · 2018-01-30T00:56:16Z

src/System.Memory/src/System/MemoryExtensions.Fast.cs

@@ -3,6 +3,8 @@
 // See the LICENSE file in the project root for more information.

 using System.Runtime.CompilerServices;
+using System.Runtime.InteropServices;


If we get rid of the AggressiveInlining, do we need using System.Runtime.CompilerServices; anymore now that we use Internal.Runtime.... for Fast span?

ahsonkhan · 2018-01-30T00:58:16Z

src/System.Memory/tests/TestHelpers.cs

+        {
+            public char a1; // 2 bytes
+                            // 2 bytes padding
+            public int a2;  // 4 bytes


Is this struct guaranteed to be laid out like this or do we need to set explicit offsets, for example:
https://github.com/dotnet/corefx/blob/master/src/System.Memory/tests/TestHelpers.cs#L241?

ahsonkhan · 2018-01-30T00:58:59Z

src/System.Memory/tests/Span/TryCast.cs

+        }
+
+        [Fact]
+        public static unsafe void TryCastSpan_Overflow()


Why is this unsafe?

ahsonkhan · 2018-01-30T00:59:43Z

src/System.Memory/tests/Span/TryCast.cs

+        }
+
+        [Fact]
+        public static unsafe void TryCastSpan_ToEmptyStruct()


same here, these don't need to be unsafe.

here and elsewhere

ahsonkhan · 2018-01-30T01:01:58Z

src/System.Memory/src/System/MemoryExtensions.Portable.cs

+        /// <summary>
+        /// Attempts to cast a ReadOnlySpan of one primitive type <typeparamref name="TFrom"/> to another primitive type <typeparamref name="TTo"/>.
+        /// </summary>
+        /// <param name="source">The source slice, of type <typeparamref name="TFrom"/>.</param>


nit: remove "slice," or rename to "span"

ahsonkhan · 2018-01-30T01:02:41Z

src/System.Memory/src/System/MemoryExtensions.Portable.cs

+        /// </summary>
+        /// <param name="source">The source slice, of type <typeparamref name="TFrom"/>.</param>
+        /// <param name="output">The destination of type <typeparamref name="TTo"/>.</param>
+        /// <remarks>If <typeparamref name="TTo"/> is 8-byte aligned and <paramref name="source"/> points to a valid aligned <typeparamref name="TTo"/> address,


For x86, it could be 4-byte aligned too, right? Update to word size/pointer size.

Interesting point. Is the maximum alignment of a C# type on an x86 cpu 4 bytes? Seems like the word size wouldn't affect the alignment, but I'm not certain.

For some context, the shortcut here is that when a TFrom is aligned by 8-bytes then it is also aligned to 4-bytes, 2-bytes, 1-bytes. And since TTo can be at most aligned by 8-bytes, any TTo will also be aligned.

ahsonkhan · 2018-01-30T01:05:16Z

src/System.Memory/src/System/MemoryExtensions.Portable.cs

+
+                    // First we have to make sure the given pointer follows the alignment rules of its TFrom type. Since the TFrom alignment will
+                    // be preserved even if the memory is moved, we can safely make assumptions about the alignment of TTo without the need for pinning.
+                    int sizeOfTFrom = Math.Min(8, Unsafe.SizeOf<TFrom>());


Do we need to have different implementations for IntPtr.Size == 4 and IntPtr.Size == 8?

ahsonkhan · 2018-01-30T01:06:53Z

src/System.Memory/src/System/MemoryExtensions.Portable.cs

+        /// this will always return false</remarks>
+        /// <remarks>Does not require the memory being spanned over to be fixed.</remarks>
+        /// <returns>True if successful; else False</returns>
+        public static bool TryCast<TFrom, TTo>(this Span<TFrom> source, out Span<TTo> output) where TFrom : struct where TTo : struct


Is it possible for this method to call the ReadOnlySpan one or will the out parameter cause issues?

I believe the out parameter would cause issues.

ahsonkhan · 2018-01-30T01:10:38Z

src/System.Memory/src/System/MemoryExtensions.Fast.cs

+        /// this will always return false</remarks>
+        /// <remarks>Does not require the memory being spanned over to be fixed.</remarks>
+        /// <returns>True if successful; else False</returns>
+        public static bool TryCast<TFrom, TTo>(this Span<TFrom> source, out Span<TTo> output) where TFrom : struct where TTo : struct


The Fast and Portable implementations of this API are essentially identical. Should we just have one copy in MemoryExtensions.cs? MemoryExtensions.Fast.cs only contains extension methods with different implementation and calls the Span class (within CoreLib) directly.

I expect that we will move MemoryExtensions to CoreLib. Once that happens we can have much better, faster and accurate, platform-specific implementation of this API for fast Span. I do not think we need to bother with sharing it.

They're duplicated so we can replace the Fast implementation once it gets moved to corelib.
From the commit comments:

Implemented for both the fast and portable implementations in equivalent ways, though eventually the fast implementation should be moved to corelib with the other MemoryExtensions.Fast methods and implemented using compiler directives (e.g. #IF intelProcessorCompiled) or maybe an intrinsic for the else case.

Right, this is hard to get right in portable way. I would suggest that the portable implementation just checks that sizeof(TTo) <= sizeof(IntPtr) and returns false otherwise.

Maybe even do it for x86/x64 and avoid dependency on the RuntimeInformation package.

jkotas · 2018-02-04T18:49:19Z

We should validate the usefulness of this API on real world examples. It has proven to be very useful in other cases, e.g. resulting to the API not being added (#26598 (comment)) or significant changes in design/implementation (dotnet/coreclr#15608 (comment)).

ianhays · 2018-02-05T18:18:05Z

The existence of https://github.com/dotnet/corefx/issues/26644 seems to suggest that it's former version, NonPortableCast, was being used at least enough to spark an (accepted) proposal to promote it from extension method to member.

I'm not necessarily against closing this and re-evaluating #26465, but to do so would likely require that we postpone it until the 2.2 timeframe as the 2.1 API cutoff is fast approaching. That would leave us without a portable casting solution since Cast was made fast-span only.

jkotas · 2018-02-05T18:51:56Z

portable casting solution since Cast was made fast-span only

MemoryMarshal.Cast is in both slow Span and fast Span.

I'm not necessarily against closing

I would also feel better if we do this with longer runway. I am not convinced that this is the right API to have. The design will likely require iterations to get right.

ianhays · 2018-02-06T18:02:35Z

MemoryMarshal.Cast is in both slow Span and fast Span.

So it is. I was confusing it with Create which was moved to fast-only.

I would also feel better if we do this with longer runway. I am not convinced that this is the right API to have. The design will likely require iterations to get right.

Since we don't have a good answer for portable span anyways, I'm fine with that.

ianhays self-assigned this Jan 29, 2018

ianhays added this to the 2.1.0 milestone Jan 29, 2018

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

KrzysztofCwalina reviewed Jan 29, 2018

View reviewed changes

jkotas reviewed Jan 30, 2018

View reviewed changes

ahsonkhan reviewed Jan 30, 2018

View reviewed changes

src/System.Memory/tests/Span/TryCast.cs

}

[Fact]

public static unsafe void TryCastSpan_Overflow()

Copy link

Member

ahsonkhan Jan 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this unsafe?

ahsonkhan reviewed Jan 30, 2018

View reviewed changes

ianhays closed this Feb 6, 2018

ianhays deleted the trycast branch June 11, 2018 17:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Span/ReadOnlySpan TryCast #26662

Add Span/ReadOnlySpan TryCast #26662

ianhays commented Jan 29, 2018

KrzysztofCwalina Jan 29, 2018

jkotas Jan 30, 2018 •

edited

Loading

ahsonkhan Jan 30, 2018

jkotas Jan 30, 2018

ianhays Jan 30, 2018

jkotas Jan 30, 2018 •

edited

Loading

ianhays Jan 30, 2018

jkotas Jan 30, 2018

ianhays Jan 30, 2018

ianhays Jan 30, 2018

jkotas Jan 30, 2018

jkotas Jan 30, 2018

jkotas Jan 30, 2018

ianhays Jan 30, 2018

ahsonkhan Jan 30, 2018

ahsonkhan Jan 30, 2018 •

edited

Loading

ahsonkhan Jan 30, 2018

ahsonkhan Jan 30, 2018 •

edited

Loading

ahsonkhan Jan 30, 2018

ahsonkhan Jan 30, 2018 •

edited

Loading

ianhays Jan 30, 2018

ahsonkhan Jan 30, 2018 •

edited

Loading

ahsonkhan Jan 30, 2018

ianhays Jan 30, 2018

ahsonkhan Jan 30, 2018

jkotas Jan 30, 2018 •

edited

Loading

ianhays Jan 30, 2018

jkotas Jan 30, 2018

jkotas Jan 30, 2018

jkotas commented Feb 4, 2018 •

edited

Loading

ianhays commented Feb 5, 2018

jkotas commented Feb 5, 2018 •

edited

Loading

ianhays commented Feb 6, 2018

Add Span/ReadOnlySpan TryCast #26662

Add Span/ReadOnlySpan TryCast #26662

Conversation

ianhays commented Jan 29, 2018

Choose a reason for hiding this comment

jkotas Jan 30, 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

jkotas Jan 30, 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

ahsonkhan Jan 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan Jan 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan Jan 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan Jan 30, 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

jkotas Jan 30, 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

jkotas commented Feb 4, 2018 • edited Loading

ianhays commented Feb 5, 2018

jkotas commented Feb 5, 2018 • edited Loading

ianhays commented Feb 6, 2018

jkotas Jan 30, 2018 •

edited

Loading

jkotas Jan 30, 2018 •

edited

Loading

ahsonkhan Jan 30, 2018 •

edited

Loading

ahsonkhan Jan 30, 2018 •

edited

Loading

ahsonkhan Jan 30, 2018 •

edited

Loading

ahsonkhan Jan 30, 2018 •

edited

Loading

jkotas Jan 30, 2018 •

edited

Loading

jkotas commented Feb 4, 2018 •

edited

Loading

jkotas commented Feb 5, 2018 •

edited

Loading