Support casting ReadOnlyMemory<T> to Memory<T>

[stephentoub]

Latest Proposal

From https://github.com/dotnet/corefx/issues/23879#issuecomment-340861229

namespace System.Runtime.InteropServices
{
    public static class MemoryMarshal
    {
        public static bool TryGetArray<T>(ReadOnlyMemory<T> readOnlyMemory, out ArraySegment<T> arraySegment);
        public static Memory<T> AsMemory<T>(ReadOnlyMemory<T> readOnlyMemory);
    }
}

Original Proposal

We already have DangerousTryGetArray(...) on ReadOnlyMemory<T>, as well as rom.Span.DangerousGetPinnableReference(), so it's possible to get writable access to the underlying data. But there's currently no efficient way to get a Memory<T> from a ReadOnlyMemory<T>.

This is relevant for cases where a shared data structure or API needs to be usable from APIs that work with either Memory<T> or ReadOnlyMemory<T>. For example, Socket.SendAsync will take a ReadOnlyMemory<T>, but the underlying SocketAsyncEventArgs type only works with Memory<T>, because it needs to support both read and write APIs. If there's no way to efficiently convert a ReadOnlyMemory<T> to a Memory<T>, we'll need to increase the size of such objects to store both, and potentially add branching in a variety of places to make sure the right underlying field is being used.

I propose we add the following method to ReadOnlyMemory<T>:

public Memory<T> DangerousAsMemory();

cc: @KrzysztofCwalina, @ahsonkhan

[stephentoub]

Actually, I'm happy with a solution using Unsafe.As:

ReadOnlyMemory<byte> rom = ...;
Memory<byte> m = Unsafe.As<ReadOnlyMemory<byte>, Memory<byte>>(ref rom);

We would just need to acknowledge that code is depending on the layout of the two types matching and guarantee never to diverge them from each other. Any problems with that, @jkotas /@KrzysztofCwalina?

[jkotas]

We had a very similar discussion a while back about unsafe access to array backing ImmutableArray. The purist approach won in that discussion, we did not expose the method for it and recommended to use hack in places that really need this. The outcome is that the hack took its own life, and it is spreading and mutating: here, here or even F# version here.

Unsafe.As cast between ReadOnlyMemory and Memory is a more compact version of the same hack. In fact, one can start using Unsafe.As for dangerous conversions between array and immutable array as well. Unsafe.As was not available when ImmutableArray discussion happened.

I am not very fond of the officially recommended hacks like this. I think having methods to codify blessed dangerous conversions is better. It clearly states what is supported, does not imply that it is ok to depend on private runtime and framework implementation details, and prevents problems like when you bulk change a type by using automatic refactoring and the conversion that was dangerous before is plain wrong now.

I see this primarily as interop problem. Efficient interop between different subsystem is important. After all, efficient interop was one of the key ingredients for the early .NET success. However, efficient interop is often unsafe and requires violating abstractions that are otherwise respected within the subsystem.

[stephentoub]

I think having methods to codify blessed dangerous conversions is better.

I agree.

I will unblock myself with Unsafe.As. But I do think we should add this, or something equivalent (doesn't matter to me whether it lives on ReadOnlyMemory<T> or on Unsafe).

[karelz]

API review: We should find out how to hide the API. Let's try to use same thing as we used on ImmutableArray.

[jkotas]

@karelz There is no API to do this on ImmutableArray. The solution for ImmutableArray was tell people to hack around the lack of proper API by using unsafe code that depends on internal implementation details (see links above).

Is the recommendation here to hack around it in similar way? If yes, this issue should be closed because of there is no API to add.

[karelz]

Apparently we worked based off wrong information in the API review meeting, then we ran out of time.
Either way, it is back to @stephentoub to propose how to hide such API and/or if to still propose it.

[stephentoub]

Here's my suggestion. We remove the DangerousGetPinnableReference and DangerousTryGetArray methods from {ReadOnly}Span/Memory<T>, and instead add:

namespace System.Runtime.InteropServices
{
    public static class SpanMarshal
    {
        public static ref T GetPinnableReference<T>(Span<T> span);
        public static ref readonly T GetPinnableReference<T>(ReadOnlySpan<T> readOnlySpan);
    }

    public static class MemoryMarshal
    {
        public static bool TryGetArray<T>(ReadOnlyMemory<T> readOnlyMemory, out ArraySegment<T> arraySegment);
        public static Memory<T> AsMemory<T>(ReadOnlyMemory<T> readOnlyMemory);
    }
}

cc: @KrzysztofCwalina, @ahsonkhan, @jkotas, @jaredpar, @VSadov

[KrzysztofCwalina]

I like the idea of a separate class in InteropServices. But, could we have just once class for all span/memory related APIs? e.g. BuffersMarshal or MemoryMarshal?

[benaadams]

Are static functions, could just add to the regular Marshal class? I think that class puts people off more than unsafe does

[terrajobst]

Video

OK, it seems we should separate the discussion on the pinnable references. But the readonly casting looks reasonable. We should, however, choose a generic type name so that we can add the pinnable stuff later on. Buffers seems like a good candidate:

namespace System.Runtime.InteropServices
{
    public static class BuffersMarshal
    {
        public static bool TryGetArray<T>(ReadOnlyMemory<T> readOnlyMemory, out ArraySegment<T> arraySegment);
        public static Memory<T> AsMemory<T>(ReadOnlyMemory<T> readOnlyMemory);
    }
}

[karelz]

@marek-safar FYI: this will move existing API (not yet published) - so technically breaking change from build-to-build, which you may care about ...

[karelz]

FYI: The API review discussion was recorded - see https://youtu.be/m4BUM3nJZRw?t=4805 (15 min duration)

[terrajobst]

We just decided to rename this type. Reason being that memory is the over arching name for this feature.

namespace System.Runtime.InteropServices
{
    public static class MemoryMarshal
    {
        public static bool TryGetArray<T>(ReadOnlyMemory<T> readOnlyMemory, out ArraySegment<T> arraySegment);
        public static Memory<T> AsMemory<T>(ReadOnlyMemory<T> readOnlyMemory);
    }
}

FYI: Another round of the API review discussion was recorded - see https://youtu.be/bHwCPVNQLwo?t=5165 (8 min duration)

[Drawaes]

@stephentoub are you adding this class? I am happy to do it for dotnet/corefx#24296 now if you want, unless you have started?

If you haven't and want me to add it, shall I put it in the System.Memory solution along side the rest of the Span/Memory code ?

[stephentoub]

are you adding this class?

Not currently on my TODO list; feel free to grab it. But note that when we add it we should also remove the existing DangerousTryGetArray method. Also, @terrajobst/@KrzysztofCwalina, what did we decide about the other existing dangerous methods like DangerousGetPinnableReference().... are they moving here, too?

shall I put it in the System.Memory solution along side the rest of the Span/Memory code ?

It should be exposed from System.Memory, but implementation-wise we'll actually need two implementations, just as for the rest of this stuff; one in System.Private.Corelib and one in System.Memory.

[KrzysztofCwalina]

Yeah, we should move DangerousGetPinnableReference. DangerousGetPinnableReference won't be used for language supported fixed syntax. We will need another method for it. The reason is that language syntax will need null ref when fixing empty spans. DangerousGetPinnableReference does not return null ref for empty spans. We cannot change DangerousGetPinnableReference to return a null ref as it has perfromance cost.

cc: @jkotas, @VSadov, @ahsonkhan

[jkotas]

We should also rename it as part of the move. Pinnable does not make sense if it is not going to be used for pinning. And Dangerous can be omitted too because of methods under System.Runtime.InteropServices are dangerous by convention.

So what about:

namespace System.Runtime.InteropServices
{
    public static class MemoryMarshal
    {
        public static ref T GetReference<T>(in ReadOnlySpan<T> span);
        public static ref T GetReference<T>(in Span<T> span);
    }
}

[ektrah]

There was already some confusion (can't find the issue# right now) if the reference was pinned or not (some people assumed it was). Maybe MemoryMarshal.GetUnpinnedReference()?

Btw, I'm currently using DangerousGetPinnableReference a lot with p/invoke (which, as far as I know, pins the reference):

[DllImport("lib")]
static extern void Foo(ref byte source, int length);

static void Foo(ReadOnlySpan<byte> source) =>
    Foo(ref source.DangerousGetPinnableReference(), source.Length);

[stephentoub]

So what about

I like GetReference. I don't think we need to say anything about pinned or unpinned. Unpinned is also potentially misleading, in that it could actually be pinned due to something elsewhere, that pinning just has nothing to do with this action.

[Drawaes]

Started a WIP PR to add this, just a shell to ensure that approach is correct and everyone is 😎 dotnet/coreclr#14833

[aalmada]

@ektrah I agree that the name is confusing. I'm now using Span on PInvokes and I'm not sure if I should use fixed or not.
The name DangerousGetPinnableReference indicates that I should but what if the allocation was done using stackalloc or Marshal.AllocHGlobal? It's unnecessary.
In this repository I find code that uses fixed and also code that doesn't...

[karelz]

We should review the final form proposed by @jkotas (ideally today).

[karelz]

Video

API review: Let's separate @jkotas's rename change into separate issue -- dotnet/corefx#25412.
Marking as api-approved again.

[stephentoub]

AsMemory was already implemented/added.

[ahsonkhan]

What about TryGetArray?

public static bool TryGetArray<T>(ReadOnlyMemory<T> readOnlyMemory, out ArraySegment<T> arraySegment);

[stephentoub]

What about TryGetArray?

I thought someone created a separate issue for that. If not, can you open one? Thanks! This issue was meant to be about the cast only and morphed.