Fix BufferExtensions.Write to specify sizeHint to GetSpan #110047
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-memory |
| @@ -115,7 +115,7 @@ public static T[] ToArray<T>(in this ReadOnlySequence<T> sequence) | |||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | |||
| public static void Write<T>(this IBufferWriter<T> writer, ReadOnlySpan<T> value) | |||
| { | |||
| Span<T> destination = writer.GetSpan(); | |||
| Span<T> destination = writer.GetSpan(value.Length); | |||
There was a problem hiding this comment.
The "slow path" below is never going to be hit now, and the LOH will start getting hit
#110028 (comment)
There was a problem hiding this comment.
Why not? The size is a hint. The IBufferWriter implementation is neither required nor guaranteed to return that as a minimum.
There was a problem hiding this comment.
Unless my coffee hasn't kicked in and I'm misreading the doc comments, the span returned must be at least sizeHint in size.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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
(Also, the majority of IBufferWriter implementations pool.)
True, but I think we generally would like to avoid pooling large arrays?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We can add EnsureCapacity but wouldn't it be the same? Would it return anything?
There was a problem hiding this comment.
Would a Write(ReadOnlySpan<T>) DIM on IBufferWriter<T> work?
No description provided.