Implement Memory<T> final design

[ahsonkhan]

This probably needs to be broken down into additional issues/specific work items.

[KrzysztofCwalina]

• - Measure performance impact of adding poison pill ID back
• - Measure performance impact of making abstraction to be filed-less.
• - Move Memory and friends to System.Buffers namespace
• - Move Memory and friends to System.Buffers.Primitives dll and package
• - Do we need both Pin() and Reserve() methods?
• - Do we need TryGetArray and TryGetPointer?

[benaadams]

Do we need TryGetArray and TryGetPointer?

I think? the pinnable ref on span removes the need for these?

[KrzysztofCwalina]

@benaadams, I think it covers TryGetPointer. TryGetArray might be needed for interop with existing APIs that operate on arrays only. But then, I am not sure we want to ship such leaky abstraction.

[jkotas]

TryGetArray might be needed for interop with existing APIs

It is necessary for efficient interop.

It is pretty common for efficient interop to be leaky. And if you do not provide the official way to do it, people have to resort to hacks to do it.

The purist view won for ImmutableArray<T> and you see hacks like this to grow everywhere https://github.com/dotnet/roslyn/blob/614299ff83da9959fa07131c6d0ffbc58873b6ae/src/Compilers/Core/Portable/InternalUtilities/ImmutableArrayInterop.cs#L30 . I think these hacks are much worse than having official leaky API for it.

[davidfowl]

Measure performance impact of adding poison pill ID back

Make sure that includes measuring in Kestrel as well.

Do we need TryGetArray and TryGetPointer?

Yes, lets keep both.

Do we need both Pin() and Reserve() methods?

I'm not sure we ever needed Reserve really...

[KrzysztofCwalina]

@davidfowl, I thought I added Reserve because you needed it.

[davidfowl]

@KrzysztofCwalina yea, but it's not required anymore. Pipelines manually ref counts by directly using OwnedMemory<byte>

[mjp41]

Do we need TryGetArray and TryGetPointer?

Yes, lets keep both.

So I can definitely see reasons for TryGetArray, some libraries require ArraySegments. But TryGetPointer is now only used in two places in the code base, and both of those were where @pdeligia and I couldn't work out the lifetime of the native pointer. Both are inside TcpConnections.

corefxlab/src/System.IO.Pipelines.Networking.Libuv/UvTcpConnection.cs

Line 237 in cd413fc

if (!inputBuffer.Memory.TryGetPointer(out pointer))

corefxlab/src/System.IO.Pipelines.Networking.Windows.RIO/RioTcpConnection.cs

Line 225 in 565a10a

if (!memory.TryGetPointer(out pointer))

They both only work with OwnedMemory that has been pinned. So again this kind of breaks abstraction of Memory<T>.

[KrzysztofCwalina]

Any voluntairs to fix the files linked above to not use TryGetPointer?

[KodrAus]

@KrzysztofCwalina If it's not urgent and you don't mind a bit of possible mentoring in the PR I can work on this today.

EDIT: Actually that looks quite subtle, so someone with more context would probably handle it better. I see what you mean @mjp41 about not being able to work out the lifetime of the pointer since it's passed off to libuv.

[KrzysztofCwalina]

@KodrAus, thanks for the offer though! We would be more than happy to help you get started if you wanted to start contributing in this area. But if you feel this issue is "too subtle", maybe a different issue would be a better fit.

[KodrAus]

@KrzysztofCwalina Good to know! Well I'm still interested in this issue, so might take it over to gitter to work out specifics. Code-wise I suspect it's fairly simple, but since I've just started actually using the new memory bits I need to get up to speed.

[davidfowl]

So I can definitely see reasons for TryGetArray, some libraries require ArraySegments. But TryGetPointer is now only used in two places in the code base, and both of those were where @pdeligia and I couldn't work out the lifetime of the native pointer. Both are inside TcpConnections.

I was worried about extra overhead of the struct when you just need the pointer for already pinned cases but maybe that's fine...

[davidfowl]

The other problem is that sometimes things have to be pinned long term (like for RIO) and the pointer for the backing memory just needs to be available but not constantly pinned and freed.

[KrzysztofCwalina]

I am not sure how to express this (which memory is which; pinned or not) in the type system

[davidfowl]

I am not sure how to express this (which memory is which; pinned or not) in the type system

Agreed, but this is why I like TryGetPointer. Pin today does 2 things, it ups the ref count of the backing OwnedMemory<byte> and it calls TryGetPointer or TryGetArray and pins. Free frees the GCHandle if there is any. In pipelines, the Pipe manages the reference count so there's no need to increment/decrement the ref count per read call. The pin/unpin operation is still relevant if the underlying pool exposes only arrays but that isn't the case by default.

[KrzysztofCwalina]

Seems like all the items are done.