Corefxlab: Buffer<T> wrapping OwnedBuffer<T> is problematic for the BCL

[stephentoub]

I started looking more at the Buffer / OwnedBuffer in corefxlab, and I’m concerned about the design.

Specifically, for the BCL we want to add APIs in various places that work with buffers, in order to:

• Support passing in arbitrary pointers, e.g. Stream.Write
• Support passing in array subsets, e.g. int.Parse

etc. We could add individual overloads to these methods for native pointers, for offsets+counts, but part of the goal of this whole effort has been to unify all of that.

My hope had been that we'd be able to add an individual new overload in many of these situations, taking a single type that unified all of this and that didn't pull in other dependencies. I thought that was Buffer<T>. But in looking at it today, creating a Buffer<T> from a pointer or an array allocates an OwnedBuffer<T>, which is bad. For example, I'd want someone to be able to do a stackalloc and then call fileStream.Read(new Buffer<byte>(ptr, len)) to read into that memory, but with the current design, that allocates. And even if we pooled OwnedBuffers under the covers, it's still allocating and/or potentially a source of contention on a pool. And folks shouldn't need to be maintain their own pool of OwnedBuffes to avoid such things.

In my mind, there's the fast Span<T>, and then Buffer<T> simply be the heap-able version of Span<T>, essentially an ArraySegment<T> that can also work with pointers in addition to arrays. The BCL could operate in terms of those two simple exchange types and not need to be concerned in these APIs with types like OwnedBuffer<T>; it would be up to callers to use them as they needed in their system:
C# public struct Span<T> { private ByReference<T> _ptr; private int _length; ... } public struct Buffer<T> { private T[] _array; private IntPtr _ptr; private int _offset; private int _count; ... }

I understand that one of the impacts on the existing design is control over the buffers to help with pooling, to help avoiding tearing, etc. I think we're paying too much for that and such issues should be left to types that sit entirely above Span<T> and Buffer<T> rather than being inherently intertwined. Developers using Span<T> and Buffer<T> are responsible for doing so correctly; if you put a Buffer<T> on the heap and have a race condition that ends up tearing it, that's a bug in your program that you need to fix, not something that every developer who doesn't have such bugs should be forced to pay for in usability and runtime cost. A higher-level set of types can be classes, be concerned with pooling, etc.

I realize we've probably been around and around on many of these designs, that various issues are called out in https://github.com/dotnet/corefxlab/blob/master/docs/specs/memory.md, etc., but I think we need to revisit this from the perspective of the minimal set of core types that we'd use throughout the BCL, with no unnecessary integration with an entire buffer pooling system, and then have such a system that's built on top, that's replaceable if someone wants to use a different one, etc.

[omariom]

@stephentoub
Do I understand it correctly that your plan to use Buffer and Span is like this: take Buffers on the API edges, use Spans in the synchronous parts of the implementations, and when switching to async make copies for the Buffers wrapping pointers?

[ahsonkhan]

cc @KrzysztofCwalina, @joshfree, @shiftylogic

[stephentoub]

Do I understand it correctly

Based on my current understanding, I think it would make sense for guaranteed always synchronous APIs to take Span<T> and for possibly asynchronous APIs to take Buffer<T>. But the APIs wouldn't make any copies: it would be the responsibility of the caller to ensure that the supplied Buffer<T> is valid for the lifetime that's necessary. Higher-level types could help with that, but the API should not and would not make a copy, nor would Buffer<T> itself do any kind of reference counting, etc.

[KodrAus]

Perhaps we should settle on a mental model for Span and Buffer that aligns with the BCL's desire for an abstract slice, and with Pipeline's heavy use of pooling.

The way I see it, both Span and Buffer are types that don't own their contents, where Span has a minimal lifetime and Buffer has a maximal lifetime. The way we're managing the lifetime for Span is through its by-ref constraints. The way we're managing the lifetime for Buffer is through reference counting against an explicitly typed owner.

I feel like Buffer as it stands makes a good effort to encourage memory safety, and I would be interested to see what falls back on the user if it becomes their responsibility to manage the lifetime of buffers taken from arbitrary memory, without the OwnedBuffer to do it for them.

What do you think @stephentoub? Please correct me if I'm wrong.

[davidfowl]

This actually wouldn't change anything with pipeline's usage much as life time the OwnedBuffer<byte> is managed by the pipe itself. For other scenarios though, the caller has to manage the lifetime of the buffer which IMO isn't that big a deal. The bigger issue is that there is no first class way to pass around a life time aware slice of OwnedBuffer<T>. That's basically what Buffer<T> is today. It would make it possible to do something like this:

```C#
OwnedBuffer ownedBuffer = pool.Lease(1024);
ownedBuffer.AddReference();
await Stream.WriteAsync(ownedBuffer.Buffer);
ownedBuffer.Release();

```C#
public class CustomStream : Stream
{
    private List<Buffer<byte>> _owned = new List<Buffer<byte>>();
    public override Task WriteAsync(Buffer<byte> data)
    {
         // Take ownership of the buffer
         data.AddReference();
         // Add it to a list
         _owned.Add(data);
    }

    // Imagine some background thread flushing the bytes on some interval
   public void OnTimerCallback()
   {
        foreach(var buffer in _owned)
        {
             WriteToSomethingElse(buffer);
             // Now I'm done with it
             buffer.Release();
        }
   }
}

The goal here to have an exchange type that enables building systems that can be buffer lifetime aware. Without the slice type, it would require passing both the OwnedBuffer<T> and the Buffer<T> Maybe the answer is the have something like:

Life time aware types

• class OwnedBuffer<T> - A reference counted aware buffer
• struct OwnedBufferSpan<T> - A struct that represents a slice of OwnedBuffer<T> (libraries can help manage the lifetime e.g. upping the reference count when there's a pending async operation so that native memory can't be cleaned up via OwnedBuffer during that call)

Non lifetime aware

• struct Buffer<T> - A heapable span of memory (up to the caller to manage lifetime, libraries cannot handle lifetime on behalf of the caller)

Stack only

• struct Span<T> - Stack only pointer like type

Conversions

It would be possible to go from:

• OwnedBuffer<T> -> Span<T>
• OwnedBuffer<T> -> Buffer<T>
• OwnedBufferSpan<T> -> Span<T>
• OwnedBufferSpan<T> -> Buffer<T>
• Buffer<T> -> Span<T>

Thoughts?

[stephentoub]

This is along the lines of what I had in mind, at least in terms of the separation of concerns:

• The workhorse here is Buffer<T>. It's the primary type that represents the memory, it's the most generic thing that can be passed around anywhere stack or heap, etc. Most APIs in a general purpose framework would work in terms of Buffer<T>.
• We then have Span<T> as a performance optimization, which we pay for with its stack-only-ness. Some APIs in a general purpose framework may take Span<T> rather than Buffer<T>, if the developer of it is sure that won't limit any potential future implementation that would require it to be on the heap. Since Span<T> can't be converted to anything else (other than a raw pointer) but everything can be converted to it, it's the universal receiver and thus if an API can be written in terms of it, it should be.
• Lifetime management can then be layered on top in separate types, e.g. your suggested OwnedBuffer<T> and OwnedBufferSpan<T>, even in a separate library.

That said, is there a reason the "owned"ness needs to be tied to Buffer<T>? Rather than OwnedBuffer<T>, why not a more general-purpose type like:
```C#
public class RefCount
{
private T _value;
private int _refCount;
...
public RefCount(T value) { ... }
public void AddReference() { ... }
public void Release() { ... }
public ref T Value => ref _value;
...
}

such that it could be used as a `RefCount<Buffer<T>>` but could also be used to reference count any other type (we already have custom versions of this floating around, e.g. SafeHandle)?  And if it's critical to enable the slicing scenario on this with a struct that could share in the reference counting of its parent, you could also have:
```C#
public struct ChildRefCount<T>
{
    private readonly RefCount<T>_owner;
    private T _value;
    ...
    public ChildRefCount(RefCount<T> owner, T childValue) { ... }
    public void AddReference() => _owner.AddReference();
    public void Release() => _owner.Release();
    public ref T Value => ref _value;
}

which could be used as a ChildRefCount<Buffer<T>>. Assuming Buffer<T> is sliceable, then you can still do things like var slice = new ChildRefCount<T>(refCount, refCount.Value.Slice(...));, and if that's too onerous, helpers could be provided as extension methods to operate on RefCount<Buffer<T>> and ChildRefCount<Buffer<T>>, as they are today in BufferExtensions.

There may be some holes here we could patch, but I like this direction in general more because:

• it keeps Buffer<T> and Span<T> as the two core pieces that general libraries can be written in terms of, without any interference from additional functionality like reference counting
• it provides general-purpose reference counting in the form of RefCount<T> and ChildRefCount<T>, that can be used with Buffer<T> but can be used with other types as well
• it enables a higher-level pooling library to pool RefCount<Buffer<T>>s, hand out RefCount<Buffer<T>>s and ChildRefCount<Buffer<T>>s, without requiring that such instantations be used in implementations and signatures everywhere.

etc.

[mjp41]

@stephentoub, @davidfowl I like this proposal. I think it tidies up the API nicely. I guess the proof will be in how the rewritten code looks like.

[KodrAus]

I like the direction this is moving too, and the idea of a general purpose RefCount type. It would make the intent for APIs and fields that require a lifetime longer than transient clearer. So the idea would be APIs that need longer lifetimes can require RefCount<Buffer<byte>> at their edges. The danger would be people thinking they can just wrap a Buffer they don't 'own' in RefCount which isn't necessarily safe since the real owner of the memory that contains that buffer might not be participating in ref counting. For managed data though it should be fine, since a live reference back to the original object would keep it from being collected, right?

So if we've got APIs at the point where memory comes in to .NET that encourage safe abstractions then being management-scheme agnostic down the line could be ok.

[KrzysztofCwalina]

Sorry for getting late to the thread. I think this is a good issue that we need to resolve. One way of resolving it is being discussed above (dependency free Buffer).

Another that we talked about in the past is to change OwnedBuffer into an interface that could be implemented by T[] and/or System.String. It seems like it would solve the problem @stephentoub outlined at the beginning of this issue, but it would also prevent tearing and result in less types/concepts in the system.

Thoughts?

cc @jkotas, @vancem

[stephentoub]

It seems like it would solve the problem @stephentoub outlined at the beginning of this issue

Only partially. You would still need to allocate to pass data into an API if you didn't have one of these T[] or Strings, e.g. if you had a pointer (such as some stackalloc'd memory, or data handed to you from a native lib like libcurl), etc.

[KrzysztofCwalina]

If you have a pointer to a stack allocated memory, you should probably not wrap it in Buffer. It would be a bug farm, as Buffer has different lifetime than stack pointer. For stack pointers, Span should be used.

You would not need to allocate anything for sliced arrays. Buffer stores index and count.

[stephentoub]

If you have a pointer to a stack allocated memory, you should probably not wrap it in Buffer. It would be a bug farm, as Buffer has different lifetime than stack pointer.

a) A stack-based pointer was just one example. A pointer provided by some other native component would apply equally.
b) Even in the stackalloc case, that's up to the developer... if they choose to pass it around, they need to understand what they're doing.
c) We need to define BCL APIs that take the appropriate type. We can't, for example, define a Stream.Read(Span) method, because many streams that are implicitly async-only in implementation will implement Read by delegating to ReadAsync and blocking, which means such a Stream.Read method would need to take a Buffer, not a Span.

[KrzysztofCwalina]

Re a) I agree. It would be a downside. But I think one object allocation per native buffer might be acceptable.

Re b) I don't think it's "up to the developer". If they want to pass the buffer into their own API, they can just make the API take Span\

Re c) I think we decided Span\

[stephentoub]

But I think one object allocation per native buffer might be acceptable.

This seems to be assuming that the managed developer is creating and caching native buffers. What about the case where native code is creating/storing its own buffers and calling back to managed code with some pointer of data for the managed code to handle? For example, libcurl calls back to a registered handler every time it has header or body data to be processed. It just calls the registered function with a pointer and a length. I'd like to use Buffer, but I don't want to allocate an object on every callback.

it clearly communicates that it might need to store the buffer for later

I disagree. See my Stream.Read example. It's a synchronous method. It doesn't store the argument for later. Stackalloc'd space would be fine. But it can't take a Span.

[KodrAus]

I think the way Span interacts with async is unfortunate but necessary. So if I think of Buffer as just a generalised Span that I would reach for first, then building safe abstractions over that for managing a Buffers lifetime if it needs to be practically longer than Spans sounds like a clear way forward.

If you pass a raw Buffer to a method then I think a reasonable expectation is that it must live at least until that method returns (so possibly after an await). To be precise, the Buffer must be guaranteed to live for the lexical scope of the method using it. If a method needs something longer then it should require an owned reference.

That's basically just an implicit version of what @davidfowl proposed. EDIT: Actually I think it's more like the opposite where the _caller needs_ to guarantee the lifetime, rather than the _callee being able_ to guarantee the lifetime.

So with that approach, @stephentoub's Stream.Read example would require Buffer, with the expectation that implementors won't hold that reference after the method completes. Which is fine, because they block on the async calls, and anyone else calling ReadAsync with borrowed memory would need to do the same before returning or require owned memory in their signature.

[KrzysztofCwalina]

I think it will be very difficult to ensure that async methods "don't hold to Buffer after they complete". It might be doable for simple async methods, but the moment your async method calls some other async methods, which in turn call yet other, and so on, the code will get very complicated and opaque to the point where providing such guarantee will be impractical.

This is why I like the simpler rule better: span for sync, Buffer for async. When you pass buffer, you need to ensure it will not AV, by either using GCed memory or OwnedMemory that can be "poisoned".

cc: @jkotas, @mjp41

[ahsonkhan]

When you pass buffer, you need to ensure it will not AV, by either using GCed memory or OwnedMemory that can be "poisoned".

Side question: What is AV?

[mjp41]

@ahsonkhan Access Violation, normally caused by accessing freed memory.

@KrzysztofCwalina , @stephentoub is there a coding pattern that could work for when to use Buffer versus RefCount. For example, if you pass a Buffer when the method returns it should not have kept a reference. If you pass a Buffer to an Async method, it can't keep it after awaiting it? If it lasts longer, then you should take a refcount?

[KodrAus]

I think it will be very difficult to ensure that async methods "don't hold to Buffer after they complete".

@KrzysztofCwalina You're totally right, it's just a recommended pattern for the callee to know if they require a buffer with a minimal or maximal lifetime, like @mjp41 suggests. Applied recursively I think it would be reasonably safe, but a bit crummy because it means you have to trust this opaque stack to play by the rules when there isn't a strong incentive for individual APIs to do so besides an inability to give a Buffer to a method that requires an OwnedBuffer or RefCount<Buffer>. I think other proposals here are much better, but I'm assuming the always-ref-countable API is untenable in the BCL.

[KrzysztofCwalina]

We enabled Memory\

[stephentoub]

@KrzysztofCwalina, is there an issue somewhere tracking adding an OwnedMemory<T>-derived type that let's me easily create one from a pointer and length? e.g.
C# namespace System.Buffers { public sealed class NativeOwnedMemory<T> : OwnedMemory<T> { public NativeOwnedMemory(void* pointer, int length); ... } }
so that I can still use Memory<T> with a pointer and length?

[KrzysztofCwalina]

opend issue #1924