Skip to content
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.

Sign up for GitHub

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

Jump to bottom

all RandomAccess methods should work for both sync and async file handles #54266

Merged
merged 17 commits into from
Jul 2, 2021
Merged

all RandomAccess methods should work for both sync and async file handles #54266

merged 17 commits into from
Jul 2, 2021

Conversation

adamsitnik
Copy link
Member

No description provided.

@adamsitnik adamsitnik added this to the 6.0.0 milestone Jun 16, 2021
@ghost
Copy link

ghost commented Jun 16, 2021

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details
Author: adamsitnik
Assignees: -
Labels:

area-System.IO
Milestone: 6.0.0

@adamsitnik
Copy link
Member Author

/azp list

@azure-pipelines
Copy link

CI/CD Pipelines for this repository:

@adamsitnik
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop-windows

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@adamsitnik
Copy link
Member Author

For the following benchmarks I got these results:

Method Toolchain fileSize userBufferSize options Mean Ratio Allocated
Read \after\corerun.exe 1024 1024 None 28.10 us 1.00 4,352 B
Read \before\corerun.exe 1024 1024 None 28.04 us 1.00 4,352 B
Write \after\corerun.exe 1024 1024 None 401.22 us 1.04 4,352 B
Write \before\corerun.exe 1024 1024 None 386.63 us 1.00 4,352 B
Read \after\corerun.exe 1024 1024 Asynchronous 37.42 us 0.54 4,584 B
Read \before\corerun.exe 1024 1024 Asynchronous 69.38 us 1.00 4,872 B
Write \after\corerun.exe 1024 1024 Asynchronous 424.80 us 0.96 4,585 B
Write \before\corerun.exe 1024 1024 Asynchronous 443.43 us 1.00 4,873 B
Read \after\corerun.exe 1048576 512 None 547.89 us 1.00 4,352 B
Read \before\corerun.exe 1048576 512 None 545.17 us 1.00 4,352 B
Write \after\corerun.exe 1048576 512 None 2,445.56 us 1.00 4,353 B
Write \before\corerun.exe 1048576 512 None 2,459.76 us 1.00 4,353 B
Read \after\corerun.exe 1048576 512 Asynchronous 2,010.98 us 0.43 55,587 B
Read \before\corerun.exe 1048576 512 Asynchronous 4,602.35 us 1.00 38,006 B
Write \after\corerun.exe 1048576 512 Asynchronous 4,028.92 us 0.52 55,591 B
Write \before\corerun.exe 1048576 512 Asynchronous 7,819.17 us 1.00 47,291 B
Read \after\corerun.exe 1048576 4096 None 509.97 us 1.01 232 B
Read \before\corerun.exe 1048576 4096 None 503.23 us 1.00 232 B
Write \after\corerun.exe 1048576 4096 None 2,524.78 us 1.05 233 B
Write \before\corerun.exe 1048576 4096 None 2,433.72 us 1.00 233 B
Read \after\corerun.exe 1048576 4096 Asynchronous 1,939.38 us 0.73 51,467 B
Read \before\corerun.exe 1048576 4096 Asynchronous 2,660.54 us 1.00 30,688 B
Write \after\corerun.exe 1048576 4096 Asynchronous 3,955.73 us 0.53 51,471 B
Write \before\corerun.exe 1048576 4096 Asynchronous 7,520.06 us 1.00 43,405 B
Read_NoBuffering \after\corerun.exe 1048576 16384 None 169.65 us 1.01 168 B
Read_NoBuffering \before\corerun.exe 1048576 16384 None 167.56 us 1.00 168 B
Write_NoBuffering \after\corerun.exe 1048576 16384 None 2,178.20 us 1.00 169 B
Write_NoBuffering \before\corerun.exe 1048576 16384 None 2,186.12 us 1.00 169 B
Read_NoBuffering \after\corerun.exe 1048576 16384 Asynchronous 504.47 us 0.56 13,001 B
Read_NoBuffering \before\corerun.exe 1048576 16384 Asynchronous 906.59 us 1.00 8,271 B
Write_NoBuffering \after\corerun.exe 1048576 16384 Asynchronous 2,680.92 us 0.80 13,004 B
Write_NoBuffering \before\corerun.exe 1048576 16384 Asynchronous 3,351.94 us 1.00 11,769 B
Read \after\corerun.exe 104857600 4096 None 68,750.96 us 1.01 268 B
Read \before\corerun.exe 104857600 4096 None 68,023.65 us 1.00 480 B
Write \after\corerun.exe 104857600 4096 None 123,433.13 us 1.04 1,016 B
Write \before\corerun.exe 104857600 4096 None 118,885.82 us 1.00 376 B
Read \after\corerun.exe 104857600 4096 Asynchronous 195,459.53 us 0.32 5,120,696 B
Read \before\corerun.exe 104857600 4096 Asynchronous 638,034.73 us 1.00 4,429,328 B
Write \after\corerun.exe 104857600 4096 Asynchronous 341,186.65 us 0.35 5,121,416 B
Write \before\corerun.exe 104857600 4096 Asynchronous 958,261.77 us 1.00 4,499,688 B
Read_NoBuffering \after\corerun.exe 104857600 16384 None 29,770.87 us 1.02 186 B
Read_NoBuffering \before\corerun.exe 104857600 16384 None 29,304.48 us 1.00 203 B
Write_NoBuffering \after\corerun.exe 104857600 16384 None 58,898.54 us 0.91 898 B
Write_NoBuffering \before\corerun.exe 104857600 16384 None 65,061.17 us 1.00 898 B
Read_NoBuffering \after\corerun.exe 104857600 16384 Asynchronous 71,489.92 us 0.33 1,282,832 B
Read_NoBuffering \before\corerun.exe 104857600 16384 Asynchronous 222,073.56 us 1.00 1,114,448 B
Write_NoBuffering \after\corerun.exe 104857600 16384 Asynchronous 137,698.51 us 0.54 1,280,988 B
Write_NoBuffering \before\corerun.exe 104857600 16384 Asynchronous 254,104.98 us 1.00 1,126,552 B

So it's definitely a tradeoff (faster execution, but more allocations). If we use .NET 5.0 as base it's an improvement for both cpu and memory. I am inclined to say that it's a good trade off

adamsitnik and others added 2 commits June 16, 2021 19:14
@adamsitnik
pass the local variable in explicit way
ffb0f12
@adamsitnik @campersau
Apply suggestions from code review
51f6d1c 
Co-authored-by: campersau <buchholz.bastian@googlemail.com>
@stephentoub
Copy link
Member

I am inclined to say that it's a good trade off

+1

@adamsitnik
Copy link
Member Author

I can see that CI has discovered some actual bugs (either in the implementation or the tests themselves). I am going to take a look at this on Monday, as I am taking today and tomorrow off.

@adamsitnik
Merge remote-tracking branch 'upstream/main' into remainingFeedback
3802047 
# Conflicts:
#	src/libraries/System.IO.FileSystem/tests/System.IO.FileSystem.Tests.csproj
@adamsitnik
separate implementation for sync and async handles
518fff8
@adamsitnik
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop-windows

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@adamsitnik
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop-windows

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@adamsitnik
Copy link
Member Author

@stephentoub the PR is ready for review, all scenarios are working. Updated benchmark numbers below:

BenchmarkDotNet=v0.13.0.1559-nightly, OS=Windows 10.0.19041.1052 (2004/May2020Update/20H1)
AMD Ryzen Threadripper PRO 3945WX 12-Cores, 1 CPU, 24 logical and 12 physical cores
.NET SDK=6.0.100-preview.4.21255.9
  [Host]     : .NET 6.0.0 (6.0.21.25307), X64 RyuJIT
  Job-FWJDGP : .NET 6.0.0 (42.42.42.42424), X64 RyuJIT
  Job-ATTUMP : .NET 6.0.0 (42.42.42.42424), X64 RyuJIT
Method Tolchain fileSize userBufferSize options Mean Ratio Allocated
Read \after 1024 1024 Asynchronous 40.83 us 0.64 4,600 B
Read \before 1024 1024 Asynchronous 63.92 us 1.00 4,872 B
Write \after 1024 1024 Asynchronous 308.96 us 0.72 4,601 B
Write \before 1024 1024 Asynchronous 427.00 us 1.00 4,873 B
ReadByte \after 1024 ? Asynchronous 40.28 us 0.62 4,600 B
ReadByte \before 1024 ? Asynchronous 65.34 us 1.00 4,872 B
WriteByte \after 1024 ? Asynchronous 323.79 us 0.80 4,601 B
WriteByte \before 1024 ? Asynchronous 405.49 us 1.00 4,873 B
Flush \after 1024 ? Asynchronous 11,351.21 us 0.56 225,582 B
Flush \before 1024 ? Asynchronous 21,794.16 us 1.00 121,539 B
Read \after 1048576 512 Asynchronous 2,039.48 us 0.35 59,683 B
Read \before 1048576 512 Asynchronous 5,968.89 us 1.00 49,745 B
Write \after 1048576 512 Asynchronous 4,135.49 us 0.56 59,687 B
Write \before 1048576 512 Asynchronous 7,584.09 us 1.00 49,413 B
Read \after 1048576 4096 Asynchronous 1,936.68 us 0.61 55,563 B
Read \before 1048576 4096 Asynchronous 3,461.82 us 1.00 31,470 B
Write \after 1048576 4096 Asynchronous 4,068.42 us 0.55 55,567 B
Write \before 1048576 4096 Asynchronous 7,413.55 us 1.00 44,971 B
Read_NoBuffering \after 1048576 16384 Asynchronous 499.31 us 0.42 14,025 B
Read_NoBuffering \before 1048576 16384 Asynchronous 1,273.66 us 1.00 8,475 B
Write_NoBuffering \after 1048576 16384 Asynchronous 2,653.53 us 0.78 14,028 B
Write_NoBuffering \before 1048576 16384 Asynchronous 3,411.97 us 1.00 11,778 B
Read \after 104857600 4096 Asynchronous 210,845.14 us 0.56 5,530,296 B
Read \before 104857600 4096 Asynchronous 402,181.13 us 1.00 3,048,704 B
Write \after 104857600 4096 Asynchronous 340,977.05 us 0.45 5,530,296 B
Write \before 104857600 4096 Asynchronous 780,089.22 us 1.00 4,506,304 B
Read_NoBuffering \after 104857600 16384 Asynchronous 64,548.51 us 0.36 1,382,708 B
Read_NoBuffering \before 104857600 16384 Asynchronous 177,142.09 us 1.00 1,127,344 B
Write_NoBuffering \after 104857600 16384 Asynchronous 122,367.09 us 0.51 1,382,816 B
Write_NoBuffering \before 104857600 16384 Asynchronous 243,310.49 us 1.00 1,127,344 B

stephentoub
stephentoub approved these changes Jul 1, 2021
View reviewed changes
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks.

(My only remaining hesitation about this approach is, in theory, you could end up with a backlog of NativeOverlappeds that need to be freed, as we've effectively removed the implicit backpressure of that work being associated with the actual operation. But that's unlikely to be impactful in practice.)

@adamsitnik adamsitnik merged commit f4e88f4 into dotnet:main Jul 2, 2021
@adamsitnik adamsitnik deleted the remainingFeedback branch July 2, 2021 14:09
@DrewScoggins DrewScoggins mentioned this pull request Jul 6, 2021
Open
@DrewScoggins
Copy link
Member

DrewScoggins/performance-2#7210

@adamsitnik
Copy link
Member Author

@DrewScoggins thank you!

@karelz karelz mentioned this pull request Jul 20, 2021
Closed
@karelz karelz mentioned this pull request Jul 28, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Aug 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@campersau campersau campersau left review comments

@stephentoub stephentoub stephentoub approved these changes

Assignees
No one assigned
Labels
area-System.IO
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@adamsitnik @stephentoub @DrewScoggins @campersau