-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Add UnixFileStreamStrategy #55191
Add UnixFileStreamStrategy #55191
Conversation
Tagging subscribers to this area: @dotnet/area-system-io Issue DetailsFixes #53809 As with the Windows strategy, there are some subtle differences from the Net5Compat implementation (which remains behind the compat flag for now). In particular, ReadAsync no longer employs a semaphore to serialize operations, so attempts to use an unbuffered FileStream.ReadAsync concurrently on Unix will not work well (nor should it). cc: @adamsitnik
|
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.
Big thanks for doing this @stephentoub!
Could you please enable the .NET 5 Compat tests for Unix as well?
Line 5 in d25af09
<!-- Windows is currently the only OS for which we provide a new strategy, so we test the Net5Compat only for Windows --> |
- <!-- Windows is currently the only OS for which we provide a new strategy, so we test the Net5Compat only for Windows -->
- <TargetFrameworks>$(NetCoreAppCurrent)-windows</TargetFrameworks>
+ <TargetFrameworks>$(NetCoreAppCurrent)-windows;$(NetCoreAppCurrent)-Unix;</TargetFrameworks>
src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/ValueTask.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/Strategies/UnixFileStreamStrategy.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/Strategies/UnixFileStreamStrategy.cs
Outdated
Show resolved
Hide resolved
@stephentoub please let me know if you need some help in getting the benchmark numbers |
a6b4540
to
ea8ca67
Compare
Thanks. I've run them. I'd welcome seeing results from another machine, though. Seems like the ones that matter most got better, but you're more familiar with these tests, their stability, and their relative impact. dotnet run --base ~/results/before --diff ~/results/after --threshold 5%
|
9980217
to
1543ec2
Compare
I've run the benchmarks on my Ubuntu 18.04 machine (not a VM, bare metal with SSD drive) using the commit from 10h ago using 100 iterations and no outliers removal (to have a LOT of samples and full distribution) which took 1.15h. --minIterationCount 100 --maxIterationCount 101 --outliers dontremove And it looks good to me! BenchmarkDotNet=v0.13.0.1559-nightly, OS=ubuntu 18.04
Intel Xeon CPU E5-1650 v4 3.60GHz, 1 CPU, 12 logical and 6 physical cores
.NET SDK=6.0.100-preview.7.21356.4
[Host] : .NET 6.0.0 (6.0.21.35602), X64 RyuJIT
Job-BUSLRH : .NET 6.0.0 (42.42.42.42424), X64 RyuJIT
Job-VCXHFI : .NET 6.0.0 (42.42.42.42424), X64 RyuJIT
|
It seems that the CI failures come from this assert: And we just need to not set the cached length for non Windows OS: but since you have not modified this line of code, I don't know how to use GitHub UI to provide a suggestion for it |
1543ec2
to
513e033
Compare
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, again big thanks for the PR @stephentoub !
513e033
to
b3a6212
Compare
Fixes #53809
As with the Windows strategy, there are some subtle differences from the Net5Compat implementation (which remains behind the compat flag for now). In particular, ReadAsync no longer employs a semaphore to serialize operations, so attempts to use an unbuffered FileStream.ReadAsync concurrently on Unix will not work well (nor should it).
cc: @adamsitnik