Change OwnedMemory Pin to take an optional integer offset

[ahsonkhan]

Related to https://github.com/dotnet/corefx/issues/24317 and dotnet/corefx#24323 (comment)

the scenario is I use OwnedMemory and MemoryHandle directly, without using Memory, e.g. I implement my own Memory-like factory for spans. If we don't like AddOffset, we should add OwnedMemory.Pin overload that takes offset to the added to the pointer inside the handle.

API Proposal

Change:

namespace System.Buffers
{
    public abstract class OwnedMemory<T> : IDisposable, IRetainable 
    {
        public abstract MemoryHandle Pin();
    }
}

To:

namespace System.Buffers
{
    public abstract class OwnedMemory<T> : IDisposable, IRetainable 
    {
        public abstract MemoryHandle Pin(int offset = 0);
    }
}

Usage

Example from NativeOwnedMemory:

public override unsafe MemoryHandle Pin() => new MemoryHandle(this, (void*)_ptr);

Would become:

public override unsafe MemoryHandle Pin(int offset = 0) 
{
   void* pointer = (void*)((byte*)_ptr + offset);
   return new MemoryHandle(this, pointer);
}

The following can then be re-written from Memory.Retain:

if (_index < 0)
{
    memoryHandle = ((OwnedMemory<T>)_arrayOrOwnedMemory).Pin();
    memoryHandle.AddOffset((_index & RemoveOwnedFlagBitMask) * Unsafe.SizeOf<T>());
}

To:

if (_index < 0)
{
    memoryHandle = ((OwnedMemory<T>)_arrayOrOwnedMemory).Pin((_index & RemoveOwnedFlagBitMask) * Unsafe.SizeOf<T>());
}

We can then remove the internal AddOffset method here which was added in dotnet/corefx#24323.

cc @KrzysztofCwalina, @stephentoub, @karelz, @terrajobst, @pakrym

[pakrym]

AddOffset can be removed without adding anything, see dotnet/corefx#24323 (comment) second point.

[ahsonkhan]

How would that work? Wouldn't we need to add getters to the private fields of MemoryHandle (for example, for GCHandle _handle?

memoryHandle = ((OwnedMemory<T>)_arrayOrOwnedMemory).Pin();
void* pointer = (void*)((byte*)memoryHandle.Pointer + ((_index & RemoveOwnedFlagBitMask) * Unsafe.SizeOf<T>()));
memoryHandle = new MemoryHandle((OwnedMemory<T>)_arrayOrOwnedMemory, pointer); // what about GCHandle?

[stephentoub]

The following can then be re-written from Memory.Retain:

Is that the only benefit?

How would that work? Wouldn't we need to add getters to the private fields of MemoryHandle (for example, for GCHandle _handle?

Your example is in the same assembly. We could add internal getters, we could make the fields internal, etc., or simply keep the internal AddOffset we have today.

What's the key scenario for exposing this functionality?

[KrzysztofCwalina]

The key scenario is using OwnedMemory directly without using Memory/RreadOnlyMemory. Memory and friends are factories of spans. They are very general purpose, e.g. they handle arrays, native pointers, and now strings. Quite often (when using OwnedMemory internally), it's more efficient to create a custom span factory to avoid the overhead associated with the general purpose Memory. But without the Pin(int) APIs, it's not possible for the factory to be slicable (as explained in @ahsonkhan's comment).

[karelz]

FYI: The API review discussion was recorded - see https://youtu.be/BI3iXFT8H7E?t=3747 (2 min duration)

[ahsonkhan]

Reverted here - dotnet/coreclr#15516

Re-opening.

[ahsonkhan]

The change from the previous PR (dotnet/coreclr#15410) was reverted here (dotnet/coreclr#15516) to unblock CoreCLR ingestion into CoreFX - this change was causing CoreFX socket tests to fail.

See trail of build update failures:
dotnet/corefx#25787
dotnet/corefx#25821
dotnet/corefx#25872

It requires further investigation to why that is happening to resolve it. TBD

Related to dotnet/corefx#25770

FYI @KrzysztofCwalina, @jkotas, @stephentoub, @davidfowl, @pakrym

[ghost]

Since this appears to a byte offset, despite OwnedMemory<> having a type parameter for the element, shouldn't the argument be named "byteOffset"? Right now, I have to play a guessing game to figure out what's intended...

[ahsonkhan]

Since this appears to a byte offset, despite OwnedMemory<> having a type parameter for the element, shouldn't the argument be named "byteOffset"?

I am fine with this change.