Skip to content

FileBufferingReadStream CopyToAsync Performance problem. #24032

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.

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

Closed
avparuch opened this issue Jul 16, 2020 · 2 comments · Fixed by #24499 or #24609
Closed

FileBufferingReadStream CopyToAsync Performance problem. #24032

avparuch opened this issue Jul 16, 2020 · 2 comments · Fixed by #24499 or #24609
Assignees
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions bug This issue describes a behavior which is not expected - a bug.
Milestone

Comments

@avparuch
Copy link
Contributor

When buffering a response using FileBufferingReadStream: initially, the stream's Position and Length are both 0. Based on the code in https://github.com/dotnet/runtime/blob/master/src/libraries/System.Private.CoreLib/src/System/IO/Stream.cs#L160-L188, CopyToAsync chooses a buffer size of 1.

When the stream is say, 16KB, this triggers a FileStream.GetLengthInternal() call 16,384 times and causes a big CPU spike for requests that are buffered to disk.

Please see attached profiler trace.
GetLengthInternal

We are currently solving this issue by calling the overload of CopyToAsync that accepts an explicit buffer size (we chose 16KB).

@Tratcher Tratcher added area-servers bug This issue describes a behavior which is not expected - a bug. labels Jul 16, 2020
@Tratcher
Copy link
Member

For reference, CopyToAsync is being called as a simple way to fully buffer the data.

We have a DrainAsync extension that can also be used for this (but we should still fix CopyToAsync).

public static Task DrainAsync(this Stream stream, CancellationToken cancellationToken)

Fixing this is tricky, the Stream.CopyToAsync code that does this check isn't virtual. We may have to talk to the runtime team about how FileBufferingReadStream implements CanSeek, Length, etc..

@BrennanConroy BrennanConroy added this to the Next sprint planning milestone Jul 17, 2020
@ghost
Copy link

ghost commented Jul 17, 2020

Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We will evaluate the request when we are planning the work for the next milestone. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@davidfowl davidfowl self-assigned this Aug 1, 2020
davidfowl added a commit that referenced this issue Aug 1, 2020
- overrride Span and Memory overloads and implement array overloads in terms of those overloads.
- Implemented CopyToAsync (but not CopyTo)
- Added tests

Fixes #24032
davidfowl added a commit that referenced this issue Aug 3, 2020
* Implement CopyToAsync in the FileBufferingReadStream
- overrride Span and Memory overloads and implement array overloads in terms of those overloads.
- Implemented CopyToAsync (but not CopyTo)
- Added tests

Fixes #24032
@ghost ghost locked as resolved and limited conversation to collaborators Sep 2, 2020
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions bug This issue describes a behavior which is not expected - a bug.
Projects
None yet
5 participants