Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix BufferExtensions.Write to specify sizeHint to GetSpan #110047

Closed
wants to merge 1 commit into from
Closed

Fix BufferExtensions.Write to specify sizeHint to GetSpan #110047

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/libraries/System.Memory/src/System/Buffers/BuffersExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@stephentoub stephentoub Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@stephentoub stephentoub Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@stephentoub stephentoub Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@bartonjs bartonjs Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// Fast path, try copying to the available memory directly
if (value.Length <= destination.Length)
Expand All @@ -140,7 +140,7 @@ private static void WriteMultiSegment<T>(IBufferWriter<T> writer, in ReadOnlySpa
input = input.Slice(writeSize);
if (input.Length > 0)
{
destination = writer.GetSpan();
destination = writer.GetSpan(input.Length);

if (destination.IsEmpty)
{
Expand Down
Loading