Move Span.DangerousCreate to MemoryMarshal.Create

[KrzysztofCwalina]

current API:

    public struct Span<T> {
        public static Span<T> DangerousCreate(object obj, ref T objectData, int length);
    }
    public struct ReadOnlySpan<T> {
        public static ReadOnlySpan<T> DangerousCreate(object obj, ref T objectData, int length);
    }

proposed API:

    public static class MemoryMarshal {
        public static Span<T> CreateSpan(object obj, ref T objectData, int length);
        public static ReadOnlySpan<T> CreateReadOnlySpan(object obj, ref T objectData, int length);
    }

[tarekgh]

Just raising the question, do we need to have similar API for {ReadOnly}Memory? I mean

    public static class MemoryMarshal {
        public static Memory<T> CreateMemory(object obj, ref T objectData, int length);
        public static ReadOnlyMemory<T> CreateReadOnlyMemory(object obj, ref T objectData, int length);
    }

per the comment https://github.com/dotnet/corefx/issues/24296#issuecomment-341586055 on #23670 looks not preferred though.

[KrzysztofCwalina]

No, Memory is not capable of storing interior pointers/refs

[tarekgh]

Sorry I meant something like

public static class MemoryMarshal {
    public static Memory<T> CreateMemory(T[] array, int start, int length);
    public static ReadOnlyMemory<T> CreateReadOnlyMemory(T[] array, int start, int length);
}

This is just will not validate parameter (similar to DangerousCreate).

I don't think we need it, I am just want to be an explicit decision we don't need it.

[KrzysztofCwalina]

I think parameter validation might be an issue for Spans which are super optimized and the validation can add up. Memory APIs have enough meat in them that I think the validation overhead is not significant. But, if people complain and show such overhead, we could always add such APIs in the next release.

[jkotas]

I think we should kill the Create overload that takes first argument as object. People always use it incorrectly (e.g. by passing the first argument as null). Just run into another case: dotnet/corefx#26289 (comment) . I have not seen a correct use it of it.

Instead, we should have Span<T> MemoryMarshal.Create<T>(ref T data, int lenght) in the netcoreapp version (ie fast Span) of System.Memory only.

[jkotas]

Also, we may want to rename NonPortableCast to MemoryMarshal.Cast to get rid of another ugly name.

[ahsonkhan]

Also, we may want to rename NonPortableCast to MemoryMarshal.Cast to get rid of another ugly name.

Updating the proposal

Edit: Creating new issue instead

[ahsonkhan]

I think we should kill the Create overload that takes first argument as object.
Instead, we should have Span<T> MemoryMarshal.Create<T>(ref T data, int lenght) in the netcoreapp version (ie fast Span) of System.Memory only.

@KrzysztofCwalina, if you agree, please update the original proposal to remove the first argument (object obj).

[KrzysztofCwalina]

I agree that the name is ugly and that it is an eyesore on MemoryExtensions/Span. So I like the rename to "Cast". I am not totally convinced the method needs to go to MemoryMarshal. The method is super useful and people will be using it no netter where it is and where it lives, especially that it is very much portable and safe in 99% of the cases. The gymnastics we go through to deswade people from using it will hurt 99% of the users and I doubt will help the 1%; they will have to discover the problems the hard way and change thir code.

@terrajobst, @weshaggard

[jkotas]

@ahsonkhan Opened dotnet/corefx#26368 on the Cast, so we may rather discuss it there.

I agree that the Cast is useful, but you need to know what you are doing. I think there needs to be a small speed-bump to get to it to make it clear.

[jkotas]

For example, we had to take a pretty no-trivial servicing fix in .NET Core 2.0 for Windows ARM because the code played it loose with alignment: dotnet/corefx#25763 . Span.Cast encourages issues like that if you are not careful.

[ahsonkhan]

I agree that the name is ugly and that it is an eyesore on MemoryExtensions/Span.

I was referring to removing the object argument from the DangerousCreate methods in the proposed API and only have it be a netcoreapp specific API.

[ianhays]

Span MemoryMarshal.Create(ref T data, int lenght)

I prefer the removal of the object argument if it isn't required, because it's not at all clear to me within a minute of searching what the purpose of the "object" parameter actually is.

[ianhays]

Ahson and I discussed this in person a bit more. Here's an updated proposal:

Current API with usage

current API:

    public struct Span<T> {
        public static Span<T> DangerousCreate(object obj, ref T objectData, int length);
    }
    public struct ReadOnlySpan<T> {
        public static ReadOnlySpan<T> DangerousCreate(object obj, ref T objectData, int length);
    }

Usage:

Person[] arr = new Person[] { new Person("Stan"), new Person("Alex"), new Person("Krzysztof") };
Person alex = arr[1];
Span<Person> spanAtAlex = Span<Person>.DangerousCreate(arr, alex, 2); // Span[Alex, Krzysztof]

Issues

The main issue right now with DangerousCreate is that the fast span and slow span have different implementations. Fast span doesn't need the first obj parameter, so it will accept that parameter as null and not throw. The Slow span, however, does need the first obj parameter and will NullRef on null. So we have a behavior difference between the two that is causing fairly widespread confusion.

API Option 1 - leave the argument validation behavior as-is

    public static class MemoryMarshal {
        // Slow versions of the below throw a nullref on a null obj parameter. 
        // We have the option of making the fast version throw something also, or just leaving it with no checks.
        public static Span<T> CreateSpan(object obj, ref T objectData, int length);
        public static ReadOnlySpan<T> CreateReadOnlySpan(object obj, ref T objectData, int length);
    }

Usage:

Person[] arr = new Person[] { new Person("Stan"), new Person("Alex"), new Person("Krzysztof") };
Person alex = arr[1];
Span<Person> spanAtAlex = MemoryMarshal.CreateSpan<Person>(arr, alex, 2); // Span[Alex, Krzysztof]

API Option 2 - give fast span its own API without the obj parameter

    // fast span
    public static class MemoryMarshal {
        public static Span<T> CreateSpan(ref T objectData, int length);
        public static ReadOnlySpan<T> CreateReadOnlySpan(ref T objectData, int length);
    }

   // slow span - T as offset
    public static class MemoryMarshal {
        public static Span<T> CreateSpan(object obj, ref T objectData, int length);
        public static ReadOnlySpan<T> CreateReadOnlySpan(object obj, ref T objectData, int length);
    }

   // slow (or both) spans - int as offset
    public static class MemoryMarshal {
        // use int as offset
        public static Span<T> CreateSpan(object obj, int offset, int length);
        public static ReadOnlySpan<T> CreateReadOnlySpan(object obj, int offset, int length);
    }

Usage:

// slow span - T as offset
Person[] arr = new Person[] { new Person("Stan"), new Person("Alex"), new Person("Krzysztof") };
Person alex = arr[1];
Span<Person> spanAtAlex = MemoryMarshal.CreateSpan<Person>(arr, alex, 2); // Span[Alex, Krzysztof]

// slow span (or fast span) - int as offset
Person[] arr = new Person[] { new Person("Stan"), new Person("Alex"), new Person("Krzysztof") };
Span<Person> spanAtAlex = MemoryMarshal.CreateSpan<Person>(arr, 1, 2); // Span[Alex, Krzysztof]

// fast span
Person[] arr = new Person[] { new Person("Stan"), new Person("Alex"), new Person("Krzysztof") };
Person alex = arr[1];
Span<Person> spanAtAlex = MemoryMarshal.CreateSpan<Person>(alex, 2); // Span[Alex, Krzysztof]

[jkotas]

You example is slicing arrays. If you need to slice array, you can just use the current Span constructors that take arrays.

DangerousCreate constructor was meant to create Span over fields of objects. The main problem with that is that it is never needed in practice. I have not seen a real world code that would actually do that. It is a very corner case scenarios. We should not be adding convoluted APIs that nobody will use.

[jkotas]

I think we should either delete this completely without replacement; or add the first set APIs from your option 2 for fast span only:

    // fast span
    public static class MemoryMarshal {
        public static Span<T> CreateSpan(ref T objectData, int length);
        public static ReadOnlySpan<T> CreateReadOnlySpan(ref T objectData, int length);
    }

This API is constructor equivalent of MemoryMarshal.GetReference. MemoryMarshal.GetReference lets you read the Span reference. MemoryMarshal.CreateSpan lets you initialize the Span reference.

I think one of the common uses of this API would be to let you create fixed size Spans of non-primitive types over stack allocated memory. For example:

object o = new object();
Span<object> span = MemoryMarshal.CreateSpan(ref o, 1);

[ahsonkhan]

The main problem with that is that it is never needed in practice. I have not seen a real world code that would actually do that. It is a very corner case scenarios.

I could only find two places where we use the DangerousCreate method, written as is. And in both cases, null is passed as the first argument:
https://github.com/dotnet/corefx/blob/103639b6ff5aa6ab6097f70732530e411817f09b/src/Common/src/CoreLib/System/Globalization/DateTimeFormat.cs#L706

StringBuilder origStringBuilder = FormatCustomized(dateTime, ReadOnlySpan<char>.DangerousCreate(null, ref nextCharChar, 1), dtfi, offset, result);

https://github.com/dotnet/corefx/blob/103639b6ff5aa6ab6097f70732530e411817f09b/src/Common/src/CoreLib/System/Globalization/TimeSpanFormat.cs#L318

StringBuilder origStringBuilder = FormatCustomized(value, ReadOnlySpan<char>.DangerousCreate(null, ref nextCharChar, 1), dtfi, result);

[jkotas]

BTW: The existing Span constructor that takes void* would work just fine for these.

[KrzysztofCwalina]

@tarekgh, did we just recommend people use this API to customize span creation exceptions? If not, then I think we can remove this API (DangerousCreate(obj, T, int)

[tarekgh]

did we just recommend people use this API to customize span creation exceptions?

I am not aware of any case we recommended that. would be good to check with aspnet if they already using it and what is their scenario. as @jkotas suggested there is a way to achieve the same result so I think we should be ok to remove this API.

CC @davidfowl

[ianhays]

Here's an updated proposal:

Current API

    public struct Span<T> {
        public static Span<T> DangerousCreate(object obj, ref T objectData, int length);
    }
    public struct ReadOnlySpan<T> {
        public static ReadOnlySpan<T> DangerousCreate(object obj, ref T objectData, int length);
    }

Proposed API

Remove the obj parameter and make it fast span specific

    public static class MemoryMarshal {
        public static Span<T> CreateSpan(ref T reference , int length);
        public static ReadOnlySpan<T> CreateReadOnlySpan(ref T reference , int length);
    }

[jkotas]

Nit: Should the argument name be reference instead of objectData, so that it matches MemoryMarshal.GetReference ?

[ianhays]

Yeah, that's better. objectData is pretty clunky.

Updated.

[terrajobst]

Video

We should consider changing the second overload to be in. Otherwise, it looks good as proposed.

[hexawyz]

@terrajobst When trying to use the API today, I was slightly suprised to discover that MemoryMarshal.CreateReadOnlySpan did not take an in parameter like you suggested.
Was there a specific reason for not implementing it that way in the end ?

[ianhays]

Yes @GoldenCrystal, we couldn't do in because we need the reference to not be readonly so that we could pass it down to the ReadOnlySpan constructor that gets a pointer to the reference.

[Sergio0694]

@jkotas

I think we should kill the Create overload that takes first argument as object. People always use it incorrectly (e.g. by passing the first argument as null). Just run into another case: dotnet/corefx#26289 (comment) . I have not seen a correct use it of it.

Hi, I was actually using that particular API (see referenced issue above this post, or here for an example), as it was the only available API (as far as I know) that can be used to create Span<T> out of T[,] arrays on .NET Standard 2.0 (where the new MemoryMarshal.CreateSpan(ref T, int) API is not available).

Right now I can't upgrade from System.Memory 4.4.0-preview2 to System.Memory 4.5.1 or the whole project falls apart due to that removed Span<T>.DangerousCreate(obj, ref T, int) API.

As pointed out in the other issue, since this specific API could technically still work on .NET Standard 2.0 (as it does work perfectly fine using version 4.4.0-preview2), is there a workaround to still create a Span<T> out of a T[,] array?

I've tried experimenting with Unsafe.As<T[,], T[]>(ref T[,]) to trick the runtime into seeing the T[,] as a T[] to get a Span<T> and then adjusting the initial offset to account for the increased header size of the T[,] with respect to the T[] type, but since the Span<T> type does automatic offset validation on read/writes, it detects that as an out of range error and just throws, so that didn't work (that was a long shot anyways).

Is there some other way around this using the available APIs? Thanks! 😄

[jkotas]

Is there some other way around this using the available APIs?

I cannot think about nice way around this. You may be able to hack around it if you manually set the object and offset in the portable Span.

[Sergio0694]

@jkotas Yeah that's what I feared. I just wish I had seen this issue earlier though, I might have pointed out this particular use case back then to try to convince you guys to keep that (obj, ref T, int API available.

I thought about manually setting the object and offset, but IIRC all those needed APIs/ctors are internal so I'd probably need to use reflections to do that, and that'd make the whole point of saving memory copies to gain speed pointless anyways.

Thanks for your help though, I'll go ahead and refactor those calls out then.
Have a nice day! 😊

[nietras]

@Sergio0694 see my https://github.com/dotnet/corefx/issues/32943#issuecomment-475972385 on the MemoryMarshal.CreateSpan/CreateReadOnlySpan I also think it is very unfortunate the Create(object, ref T, int) is not available.

If you know a way of circumventing this via reflection that works for both slow and fast Span I am interested. 😄

[Sergio0694]

@nietras I've written a class that can solve the issue for you. Note that it will have a slight overhead due to the explicit pinning, but I've tried to make it as easy as possible to use.
Here's the code:

/// <summary>
/// A <see langword="class"/> that exposes <see cref="Span{T}"/> instances from a 2D <typeparamref name="T"/> array, on .NET Standard 2.0
/// </summary>
public sealed class Memory2D<T> : IDisposable where T : unmanaged
{
    private readonly GCHandle Handle;
    private readonly T[,] Array;

    /// <summary>
    /// Crates a new <see cref="Memory2D{T}"/> instance that wraps a given 2D <typeparamref name="T"/> array
    /// </summary>
    /// <param name="array">The 2D <typeparamref name="T"/> array to wrap</param>
    public Memory2D(T[,] array)
    {
        Array = array;
        Handle = GCHandle.Alloc(array, GCHandleType.Pinned);
    }

    /// <summary>
    /// Gets a <see cref="Span{T}"/> that targets a specified row from the wrapped array
    /// </summary>
    /// <param name="row">The target row to retrieve</param>
    public unsafe Span<T> this[int row] => new Span<T>(Unsafe.AsPointer(ref Array[row, 0]), Array.GetLength(1));

    /// <summary>
    /// Gets a <see cref="Span{T}"/> that targets a specified row interval in the wrapped array
    /// </summary>
    /// <param name="start">The starting row</param>
    /// <param name="end">The ending row</param>
    public unsafe Span<T> GetRows(int start, int end) => new Span<T>(Unsafe.AsPointer(ref Array[start, 0]), (end - start) * Array.GetLength(1));

    /// <summary>
    /// Implicitly creates a <see cref="Memory2D{T}"/> instance from an input 2D <typeparamref name="T"/> array
    /// </summary>
    /// <param name="array">The 2D <typeparamref name="T"/> array to wrap</param>
    public static implicit operator Memory2D<T>(T[,] array) => new Memory2D<T>(array);

    /// <inheritdoc/>
    public void Dispose() => Handle.Free();
}

And you can use it like this:

public static void Test<T>(T[,] array) where T : unmanaged
{
    using (Memory2D<T> memory = array)
    {
        Span<T> row = memory[0];
        // Do things with the row span...

        Span<T> rows = memory.GetRows(0, 2);
        // Do things with the rows span...
    }
}

Hope this works for you! 😄

NOTE: since this Memory2D<T> class implements IDisposable, of course you're not forced to use it just inside a using block, you might as well also store it in a class and use it just like a classic Memory<T> instance, and just manually dispose it when you're done.

@jkotas I'll leave this here for future reference in case it might help others in the same situation too. I guess a solution like this would be preferable to trying to hack into the Span<T> implementation with reflections to use the constructor with the object parametrs. Besides, a single GCHandle should be way faster than using reflections in the first place 😊

[nietras]

@Sergio0694 that is not really what I was looking for or asking for 😄 What you are showing is basically just native memory (by pinning), which Span of course can be created over. That is not the problem here.

So no we cannot create Spans over managed memory that is not pinned, which is the problem. So the fact we can't do that forces devs to apply pinning which is bad in my view. A sympton of working around an API instead of an API working for you.

[Sergio0694]

@nietras Well yeah, I thought we had already established that there was no way of just using a reference-based Span<T> in .NET Standard 2.0 (other than reflections, which would be a slow and difficult to maintain hack though), so my intention here was just to provide a simple to use API to at least being able to get a Span<T> from a 2D array, without the need to perform copy operations.

I mean, the purpose of that code was to show a possible way (albeit pin-based, so not ideal of course) to get a Memory<T> out of a 2D array that can persist across stack frames (unlike just using fixed to pin in the curreent stack frame, and downwards).

Unless the CoreFX guys decide to make the CreateSpan(object, ref, int) API public again in the future, I'm afraid there's no other way around this. Or, a proper CreateMemory<T>(object, ref, int) API as well, that would be nice too.

[nietras]

other than reflections, which would be a slow and difficult to maintain hack though

@Sergio0694 no problem. 😄 It is in fact your hack I would be interested to see? 👍 How can you do this via reflections? There are quite a few restrictions around ref structs and some are even enforced by the clr as far as I can tell...

[OnurGumus]

I am just curious that how come this method is allowed outside of the unsafe context? So now one can crash the runtime easily without using unsafe. I am sure people here have good reasons. I just wanted to learn.