Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Use ArrayPool for oversized allocations - #39643

Merged
davidfowl merged 5 commits into
dotnet:masterfrom
benaadams:Pipelines-arraypool
Jul 30, 2019
Merged

Use ArrayPool for oversized allocations#39643
davidfowl merged 5 commits into
dotnet:masterfrom
benaadams:Pipelines-arraypool

Conversation

@benaadams

@benaadams benaadams commented Jul 20, 2019

Copy link
Copy Markdown
Member

When Pipelines allocation is larger than the max pool size; use ArrayPool, rather than new byte[]

Oversized allocations are useful in situations where you want to minimize syscalls; for example IHttpSendFileFeature in Kestrel its faster to read from a file directly into the Pipe (zero copy(ish)) with a larger buffer size (16kB+) than it is to read the smaller chunks the Pool allows, however if its allocating an array each time than this more than offsets that advantage.

Currently with the smaller block sizes it is more performant to read a file chunk first into a larger array pool buffer (lower syscalls), then copy that file into many smaller Pipe buffers.

It makes more sense to just read into the Pipe directly and avoid the additional copy.

It is unfortunate that this approach is currently slower; as you either have to increase file read calls using the smaller buffers, or allocate a new array for each read with a larger buffer.

This PR makes direct file reads into the Pipe a non-problematic option.

/cc @davidfowl @Tratcher @stephentoub

Resolves https://github.com/dotnet/corefx/issues/39640

@benaadams
benaadams force-pushed the Pipelines-arraypool branch from 214596c to 4c3ac97 Compare July 20, 2019 10:47
@benaadams
benaadams force-pushed the Pipelines-arraypool branch from 4c3ac97 to 923e9a9 Compare July 20, 2019 11:52
Comment thread src/System.IO.Pipelines/src/System/IO/Pipelines/BufferSegment.cs Outdated
@benaadams
benaadams force-pushed the Pipelines-arraypool branch from 07534b8 to df9abf5 Compare July 20, 2019 22:02
Comment thread src/System.IO.Pipelines/src/System/IO/Pipelines/Pipe.cs Outdated
Comment thread src/System.IO.Pipelines/src/System/IO/Pipelines/Pipe.cs Outdated
else
{
ArrayPool<byte>.Shared.Return(array);
byte[] poolArray = (byte[])_memoryOwner;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Could _memoryOwner ever be anything else other than IMemoryOwner<byte> or byte[]?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not anymore.

@Tratcher

Copy link
Copy Markdown
Member

This will make dotnet/aspnetcore#12632 a lot harder.

@davidfowl

Copy link
Copy Markdown
Member

Not really, we need need to expose an option to clear on return.

@davidfowl
davidfowl merged commit e7134de into dotnet:master Jul 30, 2019
@Tratcher

Tratcher commented Jul 30, 2019

Copy link
Copy Markdown
Member

ArrayPool<byte>.Shared isn't usable if you always want to Rent cleared memory, you can't rely on everybody else to clear on return. You need an option to clear on Rent or you need a separate pool that's always cleared.

@davidfowl

Copy link
Copy Markdown
Member

Sounds like a reason to do this https://github.com/dotnet/corefx/issues/25843

@benaadams
benaadams deleted the Pipelines-arraypool branch July 30, 2019 10:07
@karelz karelz added this to the 5.0 milestone Aug 3, 2019
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
* Use ArrayPool for oversized allocations



Commit migrated from dotnet/corefx@e7134de
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use ArrayPool.Shared instead of a heap array as fallback in PipeWriter implementations

6 participants