Fix BufferExtensions.Write to specify sizeHint to GetSpan

[stephentoub]

No description provided.

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-memory
See info in area-owners.md if you want to be subscribed.

[BrennanConroy]

The "slow path" below is never going to be hit now, and the LOH will start getting hit
#110028 (comment)

[stephentoub]

Why not? The size is a hint. The IBufferWriter implementation is neither required nor guaranteed to return that as a minimum.

[BrennanConroy]

Unless my coffee hasn't kicked in and I'm misreading the doc comments, the span returned must be at least sizeHint in size.

[stephentoub]

The GetSpan method on PipeWriter says it is the minimum length to be returned:
IBufferWriter says the same thing

Grr. Apparently we retcon'd it (but obviously left the parameter name):
dotnet/corefx#35554

[stephentoub]

and the LOH will start getting hit

Why are we not concerned about this with every other use of GetSpan/GetMemory? Literally every other use I've found in product src is passing in a value.

(Also, the majority of IBufferWriter implementations pool.)

[BrennanConroy]

Why are we not concerned about this with every other use of GetSpan/GetMemory?

Laziness or not perf sensitive code? It's definitely easier in most cases to just grab a large buffer and write instead of writing a loop.

But there are lots of cases where we are careful about what we pass in:
https://github.com/dotnet/aspnetcore/blob/8d0f798cc4de54a2851748be635a58eadbf79593/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketConnection.cs#L152

https://github.com/dotnet/aspnetcore/blob/8d0f798cc4de54a2851748be635a58eadbf79593/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs#L1865

https://github.com/dotnet/aspnetcore/blob/8d0f798cc4de54a2851748be635a58eadbf79593/src/Middleware/OutputCaching/src/FormatterBinaryWriter.cs#L110

https://github.com/dotnet/aspnetcore/blob/8d0f798cc4de54a2851748be635a58eadbf79593/src/Caching/SqlServer/src/DatabaseOperations.cs#L324

(Also, the majority of IBufferWriter implementations pool.)

True, but I think we generally would like to avoid pooling large arrays?

[bartonjs]

Ah, the joys of abstractions.

It seems like what this really wants to say is "I have value.Length bytes of material; but I don't care how many iterations it takes us to copy". So sort of writer.EnsureAvailableCapacity(value.Length), then writer.GetNextWritableChunk() (in case it did it with pages instead of linear allocation).

But we've decided that for IBufferWriter you say the minimum size you need so you can then just splat data in without doing any local bounds checking (dest[0] = 0x04; params.Q.X.CopyTo(dest.Slice(1)); params.Q.Y.CopyTo(dest.Slice(1 + params.Q.X.Length));); making it suitable for bit-banging at the expense of bulk copying.

So we could say here writer.GetSpan(int.Min(value.Length, 4096)) to avoid forcing a paging implementation to make one large page, but that 4096 is this function still having opinions as to a page size.

If it was an class instead of an interface I'd wonder if we wanted to just add an EnsureCapacity-type method; but since it's an interface that's hard.

[davidfowl]

We can add EnsureCapacity but wouldn't it be the same? Would it return anything?

[teo-tsirpanis]

Would a Write(ReadOnlySpan<T>) DIM on IBufferWriter<T> work?