Change OwnedMemory to MemoryManager and add an IMemoryOwner.

[ahsonkhan]

• IRetainable is IPinnable (Release is Unpin)
• OwnedMemory is split into MemoryManager and IMemoryOwner
• Reorder the arguments in the ctor of MemoryHandle
• Add a static method CreateFromPinnedArray and use the high bit on length for pre-pinned arrays
• Make the internal ctor that took OwnedMemory public (now takes MemoryManager) and add argument validation
• Reset index, length if TryGetMemoryManager returns false (cc @benaadams)

Upcoming PR on the corefx side: dotnet/corefx#28640

cc @joshfree, @GrabYourPitchforks, @KrzysztofCwalina, @pakrym, @davidfowl, @jkotas, @stephentoub

cc @Anipik, @safern - many of these changes will need to be mirrored

[GrabYourPitchforks]

Guidance & documentation should be coming online over the next few hours. I'll send links once I've finished the docs. These docs will also justify the API changes and should answer questions anybody might have regarding this.

[davidfowl]

Is there any way this could be done in stages or nah?

[ahsonkhan]

Is there any way this could be done in stages or nah?

We would have to only add APIs (leaving all the old APIs as is), wait for it to propagate, update all the calls, and then remove the old APIs. The propagation time across all repos is too large for us to wait for the round-trip. Also, this change is a bit more complex, so doing it in stages is a lot more difficult in general.

[joshfree]

Is there any way this could be done in stages or nah?

Given the goal of having this done Friday, I think the best way is to get this merged tonight and get it properly consumed in corefx tomorrow AM.

[benaadams]

With this change; any reason for this to be virtual?

[benaadams]

Probably to avoid the GetSpan() call?

[benaadams]

Could have done

public int Length { get; }

protected MemoryManager(int length)
{
    Length = length;
}

public Memory<T> Memory => new Memory<T>(this, 0, Length);

Unless its valid that derived classes of MemoryManager can change in length on the go?

[GrabYourPitchforks]

@benaadams There are scenarios where this can change; e.g., if the factory pools and reuses these instances. We can provide a default implementation of this logic via reading the value from GetSpan(), but as a perf optimization implementations may want to provide the value in a more direct manner.

[pakrym]

Why would it change in case of a pool? We usually don't return exact size requested.

[davidfowl]

@benaadams @GrabYourPitchforks just made me aware of the fact that this type will rarely be used directly because the MemoryPool returns IMemoryOwner<T>, so really none of this matters. Though as an implementer of MemoryManager<T>, I think it's confusing to expose length. So lets try to come up with something concrete.

[benaadams]

K so Memory won't inline due to the interface.

However as the implementer you will have to get a Length somewhere and possibly an Offset, so can stick it on the type and have everything else inline and have a good performance implementation with no additional virtuals or the need to implement the two methods?

public abstract class MemoryManager<T> : IMemoryOwner<T>, IPinnable
{
    protected int Offset { get; set; }
    protected int Length { get; set; }

    protected MemoryManager()
    {
    }
    
    protected MemoryManager(int length)
    {
        Length = length;
    }

    protected MemoryManager(int offset, int length)
    {
        Offset = offset;
        Length = length;
    }

    public Memory<T> Memory => new Memory<T>(this, Offset, Length);
    
    // Rest as now

[benaadams]

Or kill Length and force the implementation

public abstract class MemoryManager<T> : IMemoryOwner<T>, IPinnable
{
    public abstract Memory<T> Memory { get; }
    
    // Rest as now

Passing off the validation of offset and length to derived class

public class MyMemory : MemoryManager<byte>
{
    private int Offset { get; set; }
    private int Length { get; set; }
    
    public MyMemory(...)
    {
        // ...
    }
    
    public override Memory<T> Memory => new Memory<T>(this, Offset, Length);
} 

[davidfowl]

@GrabYourPitchforks making it abstract?

[ahsonkhan]

Thanks for the feedback. In the interest of time, I am going to merge the change as is and we can consider changing this separately.

[joshfree]

@MattGal @mmitche is there a known issue with the macos ci queue? this job has been sitting at queue position 93 the past 3 hours - and now the queue depth is up to 124 and growing.

[GrabYourPitchforks]

FYI, https://gist.github.com/GrabYourPitchforks/4c3e1935fd4d9fa2831dbfcab35dffc6 is the latest draft of the Memory<T> usage guidelines (detailing ownership, lifetime management, and expectations when you're consuming Memory<T> in your public APIs).

The document detailing Memory<T> vs. IMemoryOwner<T> vs. MemoryManager<T> is still being written.

[ahsonkhan]

is there a known issue with the macos ci queue? this job has been sitting at queue position 93 the past 3 hours - and now the queue depth is up to 124 and growing.

OSX10.12 x64 Checked Innerloop Build and Test — Triggered. (93/134 on osx-10.12||OSX.1012.Amd6.Open)

[joshfree]

@ahsonkhan don't block on the mac leg; https://github.com/dotnet/core-eng/issues/3064

[GrabYourPitchforks]

And here's the doc detailing the relationship between Memory<T>, IMemoryOwner<T>, and MemoryManager<T>, complete with implementation guidance and samples.
https://gist.github.com/GrabYourPitchforks/8efb15abbd90bc5b128f64981766e834

[mmitche]

https://github.com/dotnet/core-eng/issues/3072

[joshfree]

@ahsonkhan you're free to merge this given the infra issue blocking the last 2 legs from actually executing and the minimal risk of a macos or arm specific issue in this PR.