-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Use PipeWriter to write files #24851
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
Conversation
// End of the source stream. | ||
if (read == 0) | ||
{ | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should you advance here to get the writer in a working state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that only matters if you write something. At least in the Pipe implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought if you call GetMemory, you can't call GetMemory afterwards without advancing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope.
@@ -117,7 +117,7 @@ private static async Task SendFileAsyncCore(HttpResponse response, IFileInfo fil | |||
{ | |||
fileContent.Seek(offset, SeekOrigin.Begin); | |||
} | |||
await StreamCopyOperation.CopyToAsync(fileContent, response.Body, count, cancellationToken); | |||
await StreamCopyOperation.CopyToAsync(fileContent, response.BodyWriter, count, cancellationToken); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My guess is this will be good for kestrel but bad for IIS/HTTP.SYS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Http.Sys implements SendFile so this code path is never hit there. I can't recall if IIS implements the feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot all servers now implement SendFile so the only change would be if somebody call the extension method explicitly called with an IFileInfo that doesn't have a physical path 😄
bee6d9f
to
12e00f8
Compare
{ | ||
try | ||
{ | ||
if (range == null) | ||
{ | ||
await StreamCopyOperation.CopyToAsync(fileStream, outputStream, count: null, bufferSize: BufferSize, cancel: context.RequestAborted); | ||
await StreamCopyOperation.CopyToAsync(fileStream, outputPipeWriter, count: null, cancel: context.RequestAborted); | ||
} | ||
else | ||
{ | ||
fileStream.Seek(range.From.Value, SeekOrigin.Begin); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does here also need a check for range.From.Value > 0
before seek?
Also does it need a null check for range.From
?
- Use Async Disposable for FileStream - Use PipeWriter in FileResult - Call StartAsync before using the PipeWriter - Add SendFileAsync to HttpProtocol and call StartAsync in there to make sure we're not buffering more than we need to. - Use the array pool as a fallback for large buffers - Max buffer size of 64K
681d2b3
to
b7f55e1
Compare
How about IIS? It doesn't directly support SendFile either.
|
Number 1 features in IIS I wish we implemented 😄 |
We should directly support the feature in IIS but it doesn't affect mainstream scenarios for this change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has new public API and should be API reviewed
Never mind, the previous PR was API reviewed.
Debug.Assert(buffer != null); | ||
|
||
while (true) | ||
{ | ||
// The natural end of the range. | ||
if (bytesRemaining.HasValue && bytesRemaining.GetValueOrDefault() <= 0) | ||
if (bytesRemaining.HasValue && bytesRemaining.Value <= 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you change these to Value
? Value checks whether the value is null and throws an error if it is. GetValueOrDefault
returns whatever the value is, making it faster.
I'm not merging this. For buffers > 4K the GC performance is worse. I'm down a rabbit hole in excel analyzing the array pool behavior with pipelines. The other approach is to use 4K buffers no matter what the size of the file is. This results in more allocations because of the overhead of FileStream.ReadAsync. |
Successor to #21989 cc @Kahbazi
TL;DR
After spending a couple of days with this PR I've noticed that it does allocate more for files over certain sizes. After deeper investigation, it seems like we're exhausting the ArrayPool for 16K buffers (which result in the array pool allocating).
My hunch is this happens for 2 possible reasons:
It's possible we need to use different strategies to maximize throughput and keep the GC happy.
Performance numbers
128K 32 connections
16K 10 connections
16K (default connections)
4K