Adding {ReadOnly}Memory, OwnedMemory, MemoryHandle, and IRetainable

[ahsonkhan]

Part of dotnet/corefxlab#1711

Adding the following types:

• Memory<T>
• ReadOnlyMemory<T>
• OwnedMemory<T>
• MemoryHandle
• IRetainable

Adding the following Unsafe APIs that are needed by Memory<T> (these already exist in corefx):

public unsafe static void* Add<T>(void* source, int elementOffset) { throw null; }
public static T As<T>(object o) where T : class { throw null; }

Order of changes:

1. Add to mscorlib here in coreclr (this PR).
2. After the type has been added to mscorlib, add impl (for portable) and type forwarders to System.Memory and System.Runtime in corefx.
3. Remove impl from corefxlab and update uses/references in Pipelines/Buffers.Experimental.
4. Other changes/additions like review/add APIs from MemoryExtensions

cc @shiftylogic, @KrzysztofCwalina, @davidfowl, @jkotas, @stephentoub

[justinvp]

Nit: private should be specified for the above fields.

[justinvp]

Nit: private should be specified for the above fields.

[justinvp]

Should this ctor be removed to match Span<T>?

[ahsonkhan]

I asked the same question. However, we don't have other two argument Memory ctors that Span has such as the one that takes void* or the one that takes ref T. We removed it from Span<T> because of usability concerns (https://github.com/dotnet/corefx/issues/23471). I am not sure if we have such concerns for Memory<T> (yet).

I will still remove it to be consistent though.

[justinvp]

Nit: private should be specified for the above fields.

[jkotas]

Is this use of Unsafe.As really required - can we use regular cast instead? It makes the class even less safe than it needs to be.

[stephentoub]

Nit: it's a bit odd to me to see "memory" here in lower case when talking about disposing of it. It'd make more sense to me if it were either The Memory<T> has been disposed. or The memory has been freed., but the combination just strikes me weirdly.

[stephentoub]

Nit: This sentence reads strangely to me. Let's change it to something like "Release all references before disposing this instance." or something like that. I read this sentence the first time and thought that "outstanding references" were detecting something.

[stephentoub]

Nit: how about just "The pointer is null." or "Cannot get the pointer value when it is null."

[stephentoub]

Nit: please reorder these lines to match both the order of the fields being declared and the order of the parameters to the ctor

[stephentoub]

Why throw here? If someone passes null to the ctor, shouldn't they be able to access null from the property? What are we preventing by adding this?

[stephentoub]

Is this needed?

[stephentoub]

Why does this need to be virtual when the AsSpan(int, int) overload is already abstract?

[ahsonkhan]

I am not sure why we need it. I vaguely recall Krzysztof mentioning a performance penalty.
@KrzysztofCwalina, can you help answer this?

dotnet/corefxlab#1527 (comment)

[ahsonkhan]

Regarding why it is virtual. I am not really sure given both IsDisposed and AsSpan(int, int) are both abstract.

[KrzysztofCwalina]

The reason it was made virtual is that you can imagine a faster implementation than delegating to Span(int, int). But I just discussed this with @ahsonkhan: we would like to remove the AsSpan(int, int) method, make the AsSpan() abstract, and ask callers who want a section of the ownedmemory to call AsSpan().Slice(index, len)

[stephentoub]

Why do we need ReadOnlyMemory? We expose Memory, which is efficiently convertible to ReadOnlyMemory, right? It just seems inconsistent when, for example, we don't expose both AsSpan and AsReadOnlySpan, just AsSpan.

Also, from a naming perspective, why is it AsSpan but just Memory rather than having Span/Memory or AsSpan()/AsMemory()?

[stephentoub]

Generally with the Dispose pattern, the base class' Dispose() calls GC.SuppressFinalize() in case a derived class has a finalizer.

[stephentoub]

Why is this needed rather than callers using Array.Empty<T>()?

[ahsonkhan]

From: dotnet/corefxlab#1569 (comment)

We don't have it in NS 1.1, at least I don't see it.

I will use Array.Empty<T>() here but the impl in System.Memory will need to have this EmptyArray property.

[stephentoub]

Nit: BitMask

[stephentoub]

Actually, in seeing the usage of it in the code, it'd be nice if the name specified what it was a bit mask for, e.g. RemoveOwnedFlagBitMask or something like that.

[stephentoub]

The only public constructors here take a T[]. What if I have a T* and an int length and I want to call a method that takes a Memory<T>? I need to create a custom OwnedMemory<T>-derived type for that? Should we provide it?

[ahsonkhan]

We have a type in corefxlab but there is still design/API work left for that. https://github.com/dotnet/corefxlab/blob/master/src/System.Buffers.Experimental/System/Buffers/OwnedNativeBuffer.cs

Shall I file an issue to track this?

[ahsonkhan]

What if I have a T* and an int length and I want to call a method that takes a Memory<T>? I need to create a custom OwnedMemory<T>-derived type for that? Should we provide it?

I discussed with Krzysztof, and we should provide an implementation of OwnedMemory<T> but not a Memory<T> ctor that takes a pointer and length.

cc @KrzysztofCwalina, anything else to add?

[stephentoub]

There are tradeoffs, so this is just a question, but did you consider instead making this:

public static Memory<T> Empty => Array.Empty<T>();

?

[stephentoub]

I'd previously seen other methods of getting the address be a bit faster:
dotnet/corefx#22170 (comment)
Just wondering if you looked at other approaches here (and maybe it doesn't matter).

[ahsonkhan]

I had not looked at the other approaches. This is interesting. I will try what Jan suggested.

[ahsonkhan]

The two approaches below do not give the same memory address:

var array = new byte[1000];
var handle = GCHandle.Alloc(array, GCHandleType.Pinned);
void* pointer1 = (void*)handle.AddrOfPinnedObject();
void* pointer2 = Unsafe.AsPointer(ref handle);
Console.WriteLine(new IntPtr(pointer1) + ":" + new IntPtr(pointer2)); // Output: 2201489332176:670203108448

[stephentoub]

The two approaches below do not give the same memory address

The address of the handle is different from the address wrapped by the handle.

[ahsonkhan]

Hmm. That makes sense. But then how can we get the address without using AddrOfPinnedObject()?

[stephentoub]

AddrOfPinnedObject might be perfectly fine. But other approaches you could try would be:

void* pointer;
fixed (void* tmp = array) pointer = tmp;

or

void* pointer = array.Length > 0 ?
    Unsafe.AsPointer(ref array[0]) :
    null;

or:

void* pointer = Marshal.UnsafeAddrOfPinnedArrayElement(array, 0);

etc.

[ahsonkhan]

Question: Do approaches 2 and 3 work because the array has been pinned by our GCHandle.Alloc call and will not get moved by the GC? If we don't have the GCHandle.Alloc(array, GCHandleType.Pinned); call and get the pointer without using fixed, the array could still move, correct?

[stephentoub]

Correct. And even in the first case, once we leave the fixed block, the array could still be moved if it weren't for the previous Alloc.

[stephentoub]

If Retain is meant to keep the object alive, then it seems like we still need to store the array into the handle, so as to keep a reference to it. Imagine, for example, the array contained finalizable objects; if I did:

T[] array = ...;
MemoryHandle h = new Memory<T>(array).Retain();
array = null;
...

I would expect as long as I'm still holding onto h that the objects in the array wouldn't get finalized, but with this code as it stands, they could be finalized the moment I set array = null.

[ahsonkhan]

Will this work?

var array = Unsafe.As<T[]>(_arrayOrOwnedMemory);
void* pointer = Unsafe.Add<T>((void*)Unsafe.AsPointer(ref array[0]), _index);
memoryHandle = new MemoryHandle(null, pointer);

[stephentoub]

Will this work?

No. Nothing is keeping the array alive here (and the pointer isn't valid unless something else is pinning the array).

[KrzysztofCwalina]

As implemented today (and envisioned), Retain is not a guarantee that the buffer will be kept alive. It just guarantees that the reference the caller has to the buffer will work (as opposed to being disposed/deleted/etc).

I am not saying that this is the right behavior we should ship, but rather that this is what Retain was originally designed to do.

[KrzysztofCwalina]

If we wanted to change the behavior/design, we could change the MemoryHandle._owner field to be type of Object and store either IRetainable or T[] in the filed. It would keep the array alive. But it could keep the array alive for longer than needed in some cases. It's a tradeoff. I am ok with either behavior.

[stephentoub]

I don't have a strong opinion, either. Just seemed strange that Retain() doesn't actually "retain" in that case.

[KrzysztofCwalina]

As to pinning, when the pin parameter is true, we create GC handle for the array, and so I am not sure I understand the comments about the pointer not being pinned. It's pinned when pin == true and it's not pinned when it's false (but then the pointer property returns null)

[stephentoub]

so I am not sure I understand the comments about the pointer not being pinned

The comments are in response to Ahson's "will this work" example.

[stephentoub]

Nit: "of this span"... this isn't a span

[stephentoub]

The pattern matching is syntax on structs doesn't produce ideal results. This code:

if (obj is ReadOnlyMemory<T> readOnlyMemory) return readOnlyMemory.Equals(this);

results in the IL:

        IL_0000: ldarg.1
        IL_0001: stloc.1
        IL_0002: ldloc.1
        IL_0003: isinst valuetype ReadOnlyMemory`1<!T>
        IL_0008: ldnull
        IL_0009: cgt.un
        IL_000b: dup
        IL_000c: brtrue.s IL_0019
        IL_000e: ldloca.s 2
        IL_0010: initobj valuetype ReadOnlyMemory`1<!T>
        IL_0016: ldloc.2
        IL_0017: br.s IL_001f
        IL_0019: ldloc.1
        IL_001a: unbox.any valuetype ReadOnlyMemory`1<!T>
        IL_001f: stloc.0
        IL_0020: brfalse.s IL_0031
        IL_0022: ldloca.s 0
        IL_0024: ldarg.0
        IL_0025: constrained. valuetype ReadOnlyMemory`1<!T>
        IL_002b: callvirt instance bool [mscorlib]System.Object::Equals(object)
        IL_0030: ret

whereas this code:

if (obj is ReadOnlyMemory<T>) return ((ReadOnlyMemory<T>)obj).Equals(this);

results in the IL:

        IL_0000: ldarg.1
        IL_0001: isinst valuetype ReadOnlyMemory`1<!T>
        IL_0006: brfalse.s IL_001e
        IL_0008: ldarg.1
        IL_0009: unbox.any valuetype ReadOnlyMemory`1<!T>
        IL_000e: stloc.0
        IL_000f: ldloca.s 0
        IL_0011: ldarg.0
        IL_0012: constrained. valuetype ReadOnlyMemory`1<!T>
        IL_0018: callvirt instance bool [mscorlib]System.Object::Equals(object)
        IL_001d: ret

[ahsonkhan]

doesn't produce ideal results

To clarify, it doesn't produce an incorrect result though, correct?
Are you suggesting I change the impl to this since it produces smaller IL?

if (obj is ReadOnlyMemory<T>) return ((ReadOnlyMemory<T>)obj).Equals(this);

[stephentoub]

it doesn't produce an incorrect result though, correct?

Correct

Are you suggesting I change the impl to this since it produces smaller IL?

Yes

[stephentoub]

Why is the bit masking necessary?

[ahsonkhan]

I was thinking of something like this:

    var array = new byte[100];
    var memory = new Memory<byte>(array);
    var ownedMemory = (OwnedMemory<byte>)array;
    var memoryOwned = ownedMemory.Memory;
    Assert.False(memory .Equals(memoryOwned)); // This will succeed even without the bit masking 

The bit masking isn't necessary. The first equality catches if the object is an array or an OwnedMemory.

[stephentoub]

Don't we still need to validate arguments? OwnedMemory's Length is abstract, so a derived OwnedMemory could, for example, return a negative value out of Length, which is then passed into this Memory ctor and stored here.

[ahsonkhan]

This is an internal constructor. I don't follow why the argument validation is necessary (which will add overhead to Slice calls).

I could add a null check for owner (and index/length >= 0), but what type of validation are you suggesting? We aren't accessing OwnedMemory.Length property here.

We have the following checks in Slice already:

public Memory<T> Slice(int start, int length)
{
    if ((uint)start > (uint)_length || (uint)length > (uint)(_length - start))
        ThrowHelper.ThrowArgumentOutOfRangeException();

[stephentoub]

This is an internal constructor.

It's called by OwnedMemory here:
https://github.com/dotnet/coreclr/pull/13583/files#diff-0184445324edc32335f6249267a0bf70R29
and there's no validation done on the Length, which provides an arbitrary result based on whatever the derived type chose to return:
https://github.com/dotnet/coreclr/pull/13583/files#diff-0184445324edc32335f6249267a0bf70R14
which means I can write an OwnedMemory-derived type that returns -42 for Length, and then get a Memory that has a negative length.

Now, maybe we don't care about that? After all, lots of other things could go wrong. But since we're doing some argument validation elsewhere, makes me question why not here.

[stephentoub]

We call it Dangerous here because it gives access that's not denoted by the type name (you can get the array to write to even though the type is called "read only"). But a similar issue exists on Memory<T>: the Memory<T> could have been constructed for a specific range of an array, but calling TryGetArray gives you back an ArraySegment<T>, which exposes the Array, and thus potentially gives you access to portions of data that you weren't supposed to. Do we care? I'm wondering if that one should be called Dangerous, too, or maybe we don't care about that distinction?

[ahsonkhan]

Do you suggest we mark TryGetArray with the [EditorBrowsable(EditorBrowsableState.Never)] attribute as well (along with calling it Dangerous)?

[stephentoub]

No, I'd leave it visible. And to be clear, I also wasn't pushing for Memory.TryGetArray to be renamed, rather making sure we though through whether it should be or not.

[KrzysztofCwalina]

I think ArraySegment already communicates that array items outside of the segment's range should not be accessed.

[ahsonkhan]

@jkotas, instead of using Unsafe.Add, we could (and should) do the following, correct? Should we still keep the newly added Unsafe.Add<T>(void*...) API?

void* pointer = array.Length > 0 ? Unsafe.AsPointer(ref array[_index]) : null;

Edit: Nevermind, we still need Unsafe.Add for:

void* pointer = Unsafe.Add<T>((void*)handle.AddrOfPinnedObject(), _index);

[ahsonkhan]

@dotnet-bot test Tizen armel Cross Debug Build

[ahsonkhan]

@dotnet-bot test Tizen armel Cross Debug Build

[ahsonkhan]

Any other feedback or is this PR good to go?

[sharwell]

💡 These one-line statements are particularly difficult to read.

[sharwell]

❗️ I'm not a fan of the interaction between IRetainable and IDisposable for this type. I would expect the Dispose semantics to be equivalent to a case where the constructor called Retain and then Dispose calls release, except with additional handling to ensure Dispose is idempotent.

[ahsonkhan]

We want to enable types with reference counting which means that a concrete OwnedMemory can call retain/release multiple times before disposing.

@KrzysztofCwalina, anything you would like to add?

[sharwell]

📝 This comment is related to my suggestion to review the semantics of ReferenceCountedDisposable<T> just to see if it's possible to unify the concepts.

[sharwell]

❓ Why is this restriction needed? I don't like that it means the implicit conversion operator can throw an exception that is difficult for a programmer to avoid in code.

[ahsonkhan]

I am using the same restrictions as what exist for Span:
https://github.com/dotnet/coreclr/blob/master/src/mscorlib/shared/System/Span.cs#L4

[sharwell]

@jkotas Can you shed some light on this (blame shows you as introducing the original validation for Span<T>)?

[jkotas]

This check is required because Span does not support covariance. Look for covariance in Span design doc. https://github.com/dotnet/corefxlab/blob/master/docs/specs/span.md

This check has the same effect as ref T dummy = ref array[0] except when array has zero length.

Array covariance is rarely used, but it comes with performance penalty paid by everybody and complex implementation. We did not want to carry either of these to Span/Memory.

[sharwell]

❓ Since this is an implicit conversion, why are we not handling null by returning default?

=> array != null ? new Memory<T>(array) : default;

[ahsonkhan]

I am following the same semantics as Span:
https://github.com/dotnet/coreclr/blob/master/src/mscorlib/shared/System/Span.cs#L325

[sharwell]

❓ Is Memory<T> supposed to be a ref struct in the end?

[jkotas]

No. It is what Span is for. Memory is non-ref counter-part of Span.

Memory design doc: https://github.com/dotnet/corefxlab/blob/master/docs/specs/memory.md

[sharwell]

❓ Since this is an implicit conversion, why are we not handling default(ArraySegment<T>) by returning default?

=> arraySegment.Array != null ? new Memory<T>(...) : default;

[ahsonkhan]

Same as Span. If arraySegment.Array is null, we throw.

[sharwell]

💡 This is somewhat inefficient for each caller due to the use of a conversion operator and constructor code. Consider using { get; } = instead of =>.

[ahsonkhan]

Can you please expand on why it is inefficient?

[sharwell]

The conversion of T[] to Memory<T> invokes a constructor with two argument checks. You can either perform those argument checks once, or every time the Empty property is evaluated. When you use the => form, it's evaluated every time. The { get; } = form will create a static backing field for the property which is initialized once, and simply return the cached value each time the Empty property is evaluated.

[stephentoub]

Or you use a private ctor that doesn't do any checks.

[sharwell]

💡 My strongest recommendation here would be to carefully review the semantics of ReferenceCountedDisposable<T> in Roslyn and consider how they might be usable in these scenarios as an alternative to IRetainable. This type provides strong guarantees in concurrent code and eliminates many concerns related to ownership when both IDisposable and IRetainable are at play. It also provides a weak reference type with strong semantic guarantees.

[sharwell]

❓ Is it possible to wrap this cast (which appears several times) in a property that uses the Unsafe class to avoid type checks? If so, would this have a performance win that justifies the use of unsafe? /cc @stephentoub

[jkotas]

Races in user code can make index bit to be out of sync with _arrayOrOwnedMemory. Casts are useful to diagnose this situation before it corrupts the system. If the casts are replaced by Unsafe casts, this situation will tend lead to very hard to diagnose crash during GC.

[jkotas]

Races in user code can make index bit to be out of sync with _arrayOrOwnedMemory. Casts are useful to diagnose this situation before it corrupts the system. If the casts are replaced by Unsafe casts, this situation will tend lead to very hard to diagnose crash during GC.

[jkotas]

This path looks pretty dangerous. It returns unpinned pointer to array. What is going to guarantee that the array is pinned?

[jkotas]

Ignore this comment. I see that the issue is being discussed in #13583 (comment) already.

[jkotas]

Slightly better way to do this that would be more consistent with the pinned case is: void * pointer = Unsafe.Add<T>(Unsafe.AsPointer(ref array.GetRawSzArrayData()), _index);.

And for the pinned case, you can use:

    var array = (T[])_arrayOrOwnedMemory;
    var handle = GCHandle.Alloc(array, GCHandleType.Pinned);
    void * pointer = Unsafe.Add<T>(Unsafe.AsPointer(ref array.GetRawSzArrayData()), _index);
    memoryHandle = new MemoryHandle(null, pointer, handle);

[stephentoub]

Slightly better way to do this that would be more consistent with the pinned case is

That's still not going to keep the array alive, though, right? (Nor keep the pointer valid if nothing else is pinning it, though the pointer value isn't actually needed here.)

[jkotas]

Right.

[mjp41]

Did you experiment with doing

var arrayOrOwnedMemory = _arrayOrOwnedMemory;
if (arrayOrOwnedMemory.GetType() != typeof(T[])) 
    return new ReadOnlyMemory<T>(Unsafe.As<OwnedMemory<T>>(arrayOrOwnedMemory), memory._index & RemoveOwnedFlagBitMask, memory._length); 
return new ReadOnlyMemory<T>(Unsafe.As<T[]>(arrayOrOwnedMemory), memory._index, memory._length); 

rather than the sign of index trick? This would also give type safety, but may perform better?

[jkotas]

This does not work when the array type is not exact. E.g.:

enum X {
}

    X[] a = new X[100];
    uint[] b = (uint[])(object)a;
    Console.WriteLine(b.GetType());

[mjp41]

I thought you were ruling out Covariant typing on Memory and Span? Or do I not understand your example.

[mjp41]

Sorry I hadn't spotted the

default(T) == null 

in the constructor. I guess this means you could create a Span of type "uint" where the underlying array is of type "X". Why do you need/want this condition?

[jkotas]

Covariant typing

For references, but not for primitive valuetypes - in the current implementation at least.

I am not sure whether covariance is the right name for this behavior of primitive valuetypes. Covariance is asymmetric, but the relationship between uint[] and X[] in the above example is symmetric.

[jkotas]

Why do you need/want this condition?

This condition is there to eliminate the type check for valuetypes. If we remove it, creating Span from byte[] will be slower. We do not want creating Span from byte[] to be slower.

[mjp41]

That applies to Span, but it doesn't apply to Memory, e.g. the Span property has a cast inside:

return new Span<T>((T[])_arrayOrOwnedMemory, _index, _length); 

However, I can see you want them to be consistent.

Are there cases where byte[] can be viewed at a different type? If not, the JIT could optimize GetType on anything that is forced to be a single type.

[jkotas]

Are there cases where byte[] can be viewed at a different type

Yes.

enum X : byte {
}


    X[] a = new X[100];
    byte[] b = (byte[])(object)a;
    Console.WriteLine(b.GetType());

[ahsonkhan]

I have addressed the remaining open comments. Please let me know if I missed anything.

[KrzysztofCwalina]

I think we should look into ReferenceCountedDisposable, but as a separate workitem, and a potential DRC. For now, the PR looks good to me.

[ahsonkhan]

@dotnet-bot test Tizen armel Cross Debug Build
@dotnet-bot test Tizen armel Cross Release Build

[ahsonkhan]

@dotnet-bot test Windows_NT x86 full_opt ryujit CoreCLR Perf Tests Correctness
@dotnet-bot test Windows_NT x86 full_opt legacy_backend CoreCLR Perf Tests Correctness
@dotnet-bot test Windows_NT x64 full_opt ryujit CoreCLR Perf Tests Correctness

[ahsonkhan]

@dotnet-bot test Ubuntu x64 Checked Build and Test