- Notifications
You must be signed in to change notification settings - Fork5.2k
Use UnixFileStream's ReadAsync implementation on Windows when !IsAsync#56682
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
UnixFileStream's ReadAsync implementation uses a reusable IValueTaskSource implementation to avoid allocating a new work item on every read. We can push that implementation down to OSFileStreamStrategy, and then use it for the Windows implementation of ReadAsync as well when IsAsync==false, rather than delegating to the base Stream implementation.This PR almost entirely just moves code around. The only change to logic is in RandomAccess.Windows.cs, to only set an offset into the NativeOverlapped if the SafeFileHandle is seekable; otherwise, it fails when used with pipes.
ghost commentedAug 1, 2021
Tagging subscribers to this area: @dotnet/area-system-io Issue DetailsUnixFileStream's ReadAsync implementation uses a reusable IValueTaskSource implementation to avoid allocating a new work item on every read. We can push that implementation down to OSFileStreamStrategy, and then use it for the Windows implementation of ReadAsync as well when IsAsync==false, rather than delegating to the base Stream implementation. This PR almost entirely just moves code around. The only change to logic is in RandomAccess.Windows.cs, to only set an offset into the NativeOverlapped if the SafeFileHandle is seekable; otherwise, it fails when used with pipes.
privatebyte[]_buffer=newbyte[4096];privateFileStream_stream;[GlobalSetup]publicvoidSetup(){byte[]bytes=newbyte[40_000_000];newRandom().NextBytes(bytes);stringpath=Path.GetTempFileName();File.WriteAllBytes(path,bytes);_stream=File.OpenRead(path);}[GlobalCleanup]publicvoidCleanup(){_stream.Close();File.Delete(_stream.Name);}[Benchmark]publicvoidRead(){_stream.Position=0;while(_stream.Read(_buffer)!=0);}[Benchmark]publicasyncTaskReadAsync(){_stream.Position=0;while(await_stream.ReadAsync(_buffer)!=0);} cc:@adamsitnik
|
adamsitnik left a comment
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.
LGTM, thank you@stephentoub !
src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.csShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| returnMemoryMarshal.TryGetArray(buffer,outArraySegment<byte>segment)? | ||
| newValueTask(BeginWriteInternal(segment.Array!,segment.Offset,segment.Count,null,null,serializeAsynchronously:true,apm:false)): | ||
| base.WriteAsync(buffer,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.
after this code removal,SyncWindowsFileStreamStrategy andUnixFileStreamStrategy become de facto classes that do not change or extend the behaviour ofOSFileStreamStrategy.
Have you considered the removal ofSyncWindowsFileStreamStrategy andUnixFileStreamStrategy and usingOSFileStreamStrategy directly?
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.
No strong opinion. If you'd like to consolidate, go for it. If not, I'm ok keeping the derived types.
adamsitnik commentedAug 2, 2021
I am going to merge it now so I can get updated numbers for the blog post ;) |
adamsitnik commentedAug 2, 2021
I am not sure if this was intentional, but after moving all the logic from |
UnixFileStream's ReadAsync implementation uses a reusable IValueTaskSource implementation to avoid allocating a new work item on every read. We can push that implementation down to OSFileStreamStrategy, and then use it for the Windows implementation of ReadAsync as well when IsAsync==false, rather than delegating to the base Stream implementation.
This PR almost entirely just moves code around. The only change to logic is in RandomAccess.Windows.cs, to only set an offset into the NativeOverlapped if the SafeFileHandle is seekable; otherwise, it fails when used with pipes.
cc:@adamsitnik