-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
FileStream optimizations #49975
FileStream optimizations #49975
Conversation
…llow other processes to modify 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.
src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Windows.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs
Outdated
Show resolved
Hide resolved
...ibraries/System.Private.CoreLib/src/System/IO/Strategies/LegacyFileStreamStrategy.Windows.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>
This reverts commit ff4e862.
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.
@jozkee the perf results look amazing <3 (up to two times faster ReadAsync
and up to five times faster WriteAsync
!!)
Could you please re-run the following benchmarks:
Faster Read and Write sync - I am surprised that we have gained something here as we have more or less not touched the sync code path besides using a different set of parameters for ReadFile
and WriteFile
? (it's great to have the gain, but it would be good to understand it)
Method | Toolchain | fileSize | userBufferSize | options | Mean | Error | StdDev | Median | Min | Max | Ratio | Allocated |
---|---|---|---|---|---|---|---|---|---|---|---|---|
Read | base | 1048576 | 512 | None | 1,054.38 μs | 137.667 μs | 158.537 μs | 984.19 μs | 896.97 μs | 1,362.93 μs | 1.00 | 4,328 B |
Read | feature | 1048576 | 512 | None | 821.17 μs | 16.453 μs | 18.287 μs | 816.62 μs | 796.64 μs | 864.23 μs | 0.79 | 4,344 B |
Write | base | 1048576 | 512 | None | 6,409.50 μs | 261.978 μs | 280.314 μs | 6,381.70 μs | 6,035.65 μs | 7,055.64 μs | 1.00 | 4,329 B |
Write | feature | 1048576 | 512 | None | 6,052.51 μs | 206.714 μs | 238.052 μs | 6,025.02 μs | 5,631.29 μs | 6,402.86 μs | 0.94 | 4,345 B |
Why copying small files have regressed?
Method | Toolchain | fileSize | userBufferSize | options | Mean | Error | StdDev | Median | Min | Max | Ratio | Allocated |
---|---|---|---|---|---|---|---|---|---|---|---|---|
CopyToFileAsync | base | 1024 | ? | Asynchronous | 1,444.81 μs | 48.168 μs | 55.470 μs | 1,452.91 μs | 1,310.17 μs | 1,533.31 μs | 1.00 | 6,219 B |
CopyToFileAsync | feature | 1024 | ? | Asynchronous | 1,838.93 μs | 251.001 μs | 278.987 μs | 1,855.54 μs | 1,358.64 μs | 2,233.14 μs | 1.27 | 6,191 B |
Since most of the results look great and we have just one tiny regression that can be an outlier I am going to squash it right now (the sooner we do that the sooner we can test other repos if our changes have broken something)
src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs
Show resolved
Hide resolved
@adamsitnik I ran the suggested benchmarks again and the results show no regression nor improvement vs base (main), it seems to me that the benchmarks are a bit unstable. Read/Write sync:
CopyToFileAsync 1024 file size:
|
Breaking change doc created in dotnet/docs#24060. |
This PR just merges optimizations made in #49145 and #49638 and rebases them in top of the latest changes recently introduced by @adamsitnik in #48813.
Fixes #49541.
I had to do one adjustment (64e5174) in order to satisfy the tests added by #49754 but otherwise, everything else remains as it was in the old PRs.
Perf results:
There is an allocation increase of 16 B I suspect is caused by the newly added fields
_share
and_length
.