This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
Optimize overlapped I/O FileStream.CopyToAsync implementation on Windows #11569
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| // whatever read operation may currently be in progress, if there is one. It's possible the cancellation | ||
| // request could come in between operations, in which case we flag that with explicit calls to ThrowIfCancellationRequested | ||
| // in the read/write copy loop. | ||
| cancellationReg = cancellationToken.Register(s => |
There was a problem hiding this comment.
Can you just avoid this if cancellationToken = None?
There was a problem hiding this comment.
Yes, I'll do that, though it won't save very much: one of the first things Register does is check that (or, rather, CanBeCanceled), and exits early if it can't.
|
LGTM |
stephentoub
force-pushed
the
filestream_copytoasync
branch
2 times, most recently
from
119c770 to
93e928e
Compare
| unsafe | ||
| { | ||
| lock (innerAwaitable.CancellationLock) // synchronize with cleanup of the overlapped | ||
| { |
stephentoub
force-pushed
the
filestream_copytoasync
branch
4 times, most recently
from
bd71bdb to
5b4ece3
Compare
|
I've fixed the issue that was causing the CI runs to hang. |
|
😄 Waiting for this change |
| @@ -1684,6 +1685,299 @@ private int GetLastWin32ErrorAndDisposeHandleIfInvalid(bool throwIfInvalidHandle | |||
| return errorCode; | |||
| } | |||
|
|
|||
| public override Task CopyToAsync(Stream destination, int bufferSize, CancellationToken cancellationToken) | |||
| { | |||
| // Validate arguments as would the base implementation | |||
There was a problem hiding this comment.
These lines hurt my eyes. Is this really in line with our style guidelines?
There was a problem hiding this comment.
Is this really in line with our style guidelines?
We allow for single lines like this. I find this easier to follow than tripling the size of the arg validation by moving the bodies to their own lines and surrounding by braces on their own lines. But I don't feel strongly about it and can change it.
There was a problem hiding this comment.
My preference is to match the pattern in the rest of the file.
|
lgtm, thanks |
Add an override of CopyToAsync in the Windows implementation of FileStream for use when the stream is constructed in async mode. The base stream implementation does a simple loop that reads from the source and writes to the destination. For the Windows implementation of FileStream in async mode, each of these read operations involves overhead, such as allocating the task to return from the operation, registering with the providing cancellation token, etc. For CopyToAsync, we’re responsible for all of the reads, which means we can avoid these per-read costs. Copying a 10MB file to a MemoryStream with the default buffer size and with a cancelable token improved throughput by 50%, and (not including the copy buffer) reduced the number of allocations from 860 to 11 and the bytes allocated from 52K to ~730b. Copying a 100 byte file to a MemoryStream with the default buffer size and with a non-cancelable token improved throughput by 30% and (not including the copy buffer) reduced the number of allocations from 46 to 11 and the bytes allocated from 1.1K to ~670b. (I briefly explored adding an override for when in sync mode, but the savings there aren’t nearly as significant or measurable. At best it can avoid a Task per read. We can look more seriously at doing that separately, if desired; that could likely also be done for the Unix implementation.)
stephentoub
force-pushed
the
filestream_copytoasync
branch
from
5b4ece3 to
5a8285f
Compare
picenka21
pushed a commit
to picenka21/runtime
that referenced
this pull request
Feb 18, 2022
…pytoasync Optimize overlapped I/O FileStream.CopyToAsync implementation on Windows Commit migrated from dotnet/corefx@f3a2e16
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add an override of CopyToAsync in the Windows implementation of FileStream for use when the stream is constructed in async mode.
The base stream implementation does a simple loop that reads from the source and writes to the destination. For the Windows implementation of FileStream in async mode, each of these read operations involves overhead, such as allocating the task to return from the operation, registering with the providing cancellation token, etc. For CopyToAsync, we’re responsible for all of the reads, which means we can avoid these per-read costs.
Copying a 10MB file to a MemoryStream with the default buffer size and with a cancelable token improved throughput by 50%, and (not including the copy buffer) reduced the number of allocations from 860 to 11 and the bytes allocated from 52K to ~730b. Copying a 100 byte file to a MemoryStream with the default buffer size and with a non-cancelable token improved throughput by 30% and (not including the copy buffer) reduced the number of allocations from 46 to 11 and the bytes allocated from 1.1K to ~670b.
(I briefly explored adding an override for when in sync mode, but the savings there aren’t nearly as significant or measurable. At best it can avoid a Task per read. We can look more seriously at doing that separately, if desired; that could likely also be done for the Unix implementation.)
Fixes #11539
cc: @ericstj, @JeremyKuhne, @ianhays, @davidfowl