- Notifications
You must be signed in to change notification settings - Fork4.9k
Optimize overlapped I/O FileStream.CopyToAsync implementation on Windows#11569
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| // 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can you just avoid this if cancellationToken = None?
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.
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.
davidfowl commentedSep 10, 2016
LGTM |
119c770 to93e928eCompare| unsafe | ||
| { | ||
| lock(innerAwaitable.CancellationLock)// synchronize with cleanup of the overlapped | ||
| { |
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.
Nit: indentation
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.
Fixed
bd71bdb to5b4ece3Comparestephentoub commentedSep 11, 2016
I've fixed the issue that was causing the CI runs to hang. |
davidfowl commentedSep 11, 2016
😄 Waiting for this change |
| publicoverrideTaskCopyToAsync(Streamdestination,intbufferSize,CancellationTokencancellationToken) | ||
| { | ||
| // Validate arguments as would the base 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.
These lines hurt my eyes. Is this really in line with our style guidelines?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
My preference is to match the pattern in the rest of the file.
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.
Ok.
JeremyKuhne commentedSep 12, 2016
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.)
5b4ece3 to5a8285fCompare…pytoasyncOptimize overlapped I/O FileStream.CopyToAsync implementation on WindowsCommit migrated fromdotnet/corefx@f3a2e16
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