-
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 rewrite part II #48813
FileStream rewrite part II #48813
Conversation
remove finalizer from FileStream, keep it only in DerivedFileStreamStrategy and LegacyFileStreamStrategy
… functional they can now be used directly without any buffering on top of them!
…NothingToFlush_CompletesSynchronously passing
…ithin the file, not a removal and addition)
when users have a race condition in their code (i.e. they call Close when calling another method on Stream like Read).
…re not providing too much gains
@carlossanlop @jozkee @stephentoub PTAL ;) |
I like this part - it’d be great for our preview users to help us validate as we go along. |
@carlossanlop @jozkee @stephentoub first of all, big thanks for reviewing the PR and sharing a lot of very valuable feedback with me! I think that I've answered all the questions and addressed most of the concerns (3f6b541) However, two important things remain open:
|
I think it is worth switching if there's good value on doing so. Do you think that we could we use the benchmark results to make the decision? |
I suggest we start by putting the buffering logic in BufferedFileStreamStrategy. This still enables us to avoid all the buffering duplication we had across aspects of the FileStream implementation and not clutter up BufferedStream. We can then subsequently see if we can consolidate BufferedFileStreamStrategy with BufferedStream, and what new APIs on BufferedStream we might need to do so without contorting an internal contract.
I'm ok if you want to keep the old behavior, at least for now. I think the behavior is broken, though. And we shouldn't try to strengthen the old behavior in any way. |
…mStrategy # Conflicts: # src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs
…can rely on simple check
… config test runs
@stephentoub @jozkee @carlossanlop I've taken the best of |
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 looks good to me.
I couldn't find anything that would block merging, only feedback I have is a bunch of questions and possible concerns that could be addressed in follow-up PRs.
Thanks @adamsitnik!
} | ||
else | ||
{ | ||
throw Win32Marshal.GetExceptionForWin32Error(errorCode, _path); |
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: use a throw helper not to achieve inlining but to be consistent with other exceptions.
src/libraries/System.Private.CoreLib/src/System/IO/AsyncWindowsFileStreamStrategy.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/AsyncWindowsFileStreamStrategy.cs
Show resolved
Hide resolved
} | ||
else | ||
{ | ||
throw Win32Marshal.GetExceptionForWin32Error(errorCode, _path); |
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: use throw helper.
src/libraries/System.Private.CoreLib/src/System/IO/AsyncWindowsFileStreamStrategy.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/WindowsFileStreamStrategy.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/SyncWindowsFileStreamStrategy.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/SyncWindowsFileStreamStrategy.cs
Show resolved
Hide resolved
if (errorCode == ERROR_INVALID_PARAMETER) | ||
throw new ArgumentException(SR.Arg_HandleNotSync, "_fileHandle"); | ||
|
||
throw Win32Marshal.GetExceptionForWin32Error(errorCode, _path); |
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.
Same here.
// where the position is too large or for synchronous writes | ||
// to a handle opened asynchronously. | ||
if (errorCode == ERROR_INVALID_PARAMETER) | ||
throw new IOException(SR.IO_FileTooLongOrHandleNotSync); |
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.
And here.
@jozkee big thanks for the review! I am going to address the refactoring suggestions in a separate PR as soon as I merge this one. (edit: link to the PR: #49750) @carlossanlop @stephentoub Since the new strategy is disabled by default, I am going to merge this PR to unblock #49145 and #49638 and also make the memory investigation easier. If you have any feedback, I am going to address it in a separate PR. |
Breaking change doc: dotnet/docs#24060 |
Main changes:
AsyncWindowsFileStreamStrategy
andSyncWindowsFileStreamStrategy
.FileStreamHelpers.Windows
(to reduce code duplication and minimize assembly size growth)Position
,Length
etc) has been moved toWindowsFileStreamStrategy
(a base abstract type). TheLegacy
strategies were not changed.BufferedFileStreamStrategy
has been introduced. It wraps the actual strategy (FileStreamStrategy : Stream
), withBufferedStream
BufferedStream
has been adopted to meetFileStream
requirements like allowing for async 0 bytes blocking reads and some perf improvements.By replacing the custom buffering logic with
BufferedStream
, I was able to get #16341 and #27643 fixed out of the box.The code has also become much simpler, as Sync and Async strategies are now just managed wrappers for native syscalls.
The disadvantage is that
BufferedStream
acquires a lock to solve #16341 and #27643 and as a side-effect, it does not perform async reads and writes in parallel which was kind of supported until now. This is a breaking change, and that is why I've not enabled the new strategy by default. I would prefer to implement #24847 to allow users for truly thread-safe async reads and writes (set of methods that accepts offsets), and get #16354 and #25905 fixed, then test few products against it (clone dotnet/sdk set the env var, run all their tests), write a blog post and enable it for Preview 4. So we would give the users an explanation, better perf, and a way to mitigate breaking changes.fixes #27643
fixes #16341
Perf numbers:
Comment on perf:
FlushAsync
is now much more expensive (the benchmark which is quite artificial as it writes a single byte in a loop and flushes shows x2 regression). So far the buffer was getting flushed in a synchronous way. This is the price we pay for async. I don't expect it to be a problem as nobody should be callingFlushAsync
in a loop like this benchmark does.ReadAsync
with a small user buffer size (stream buffer size / 8
) has regressed by 18%, but again this is the cost of performing the read into the buffer in an async way (so far it was always synchronous).