dotnet / dotnet/runtime

StreamPipeReader.ReadAtLeastAsyncCore should not allocate such large buffers

Open
#120,618 0 comments 0 reactions 0 assignees View on GitHub
area-System.IO.Pipelines tenet-performance
Dominant language
C#
Stars
18.3k
Forks
5.6k
PR merge metrics
PR metrics pending

Description

### Description

In Nerdbank.MessagePack we want to deserialize asynchronously only when the data is very large. The default threshold is 1MB, which means we'll asynchronously read from a `PipeReader` up to 1MB before starting deserialization, and if the pipe is empty with <=1MB, we'll deserialize synchronously instead of asynchronously for better perf.
We use `PipeReader.ReadAtLeastAsyncCore(1MB)` to accomplish this, which is nice and easy.

The problem is that [that method is implemented](https://source.dot.net/#System.IO.Pipelines/System/IO/Pipelines/[StreamPipeReader.cs](https://source.dot.net/#System.IO.Pipelines/System/IO/Pipelines/StreamPipeReader.cs,a4f52a44979f0ddb),a4f52a44979f0ddb) to allocate a single, contiguous 1MB buffer when called, regardless of the configured pool max buffer size or whether 1MB will actually be read in.

When calling `ReadAtLeastAsyncCore`, I don't need or want a contiguous buffer. I just want to buffer that much in advance, but I expect it to be broken up into smaller buffers (consistent with reasonable defaults) that keep them out of the Large Object Heap.

### Configuration

.NET Framework x64

### Regression?

Not that I know of. My client is new code.

### Data

```
Name Inc % Inc
LargeObject 100.0 1,444,496
+ Type System.Byte[] 77.4 1,118,464
|+ CLR <> 77.4 1,118,464
| + module microsoft.visualstudio.shell.ui.internal.ni <> 71.5 1,032,256
| |+ LIB <> 72.6 1,048,624
| ||+ module nerdbank.messagepack.ni <> 72.6 1,048,624
```

### Analysis

The [AllocateSegment](https://source.dot.net/#System.IO.Pipelines/System/IO/Pipelines/StreamPipeReader.cs,198591ac8dd38cb5,references) method has code to specifically bypass the configured memory pool if its max buffer size would not allow a single buffer large enough for the `ReadAtLeastAsync` method argument. That seems unnecessary and undesirable.
That method also relies on this (truly) [max buffer size](https://source.dot.net/#System.IO.Pipelines/System/IO/Pipelines/[StreamPipeReaderOptions.cs](https://source.dot.net/#System.IO.Pipelines/System/IO/Pipelines/StreamPipeReaderOptions.cs,a20461fa45807cb2),a20461fa45807cb2) limit, but that's an internal property based on an internal constant, so I can't reduce that.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.