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.

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

change if order to improve FileStream perf for cases when both buffers are of the same size #51489

Merged
merged 3 commits into from
May 10, 2021

Conversation

adamsitnik
Copy link
Member

When nothing has been written to the buffer yet (_writePos == 0) and the size of the buffer passed to WriteAsync is the same as internal FileStream buffer, we should omit buffering logic and just call the actual strategy directly.

This is ofc an edge case, but since we already have a benchmark for it and it's relatively small change we should IMO change it.

See the Allocated column:

Method Toolchain fileSize userBufferSize options Mean Ratio Allocated
WriteAsync \after\corerun.exe 1048576 4096 None 3,708.7 us 1.00 29,321 B
WriteAsync \before\corerun.exe 1048576 4096 None 3,719.5 us 1.00 55,969 B
WriteAsync \after\corerun.exe 1048576 4096 Asynchronous 4,141.6 us 0.98 913 B
WriteAsync \before\corerun.exe 1048576 4096 Asynchronous 4,238.3 us 1.00 27,561 B
WriteAsync \after\corerun.exe 104857600 4096 None 212,481.6 us 0.86 2,867,896 B
WriteAsync \before\corerun.exe 104857600 4096 None 246,750.4 us 1.00 5,124,856 B
WriteAsync \after\corerun.exe 104857600 4096 Asynchronous 348,312.7 us 0.98 960 B
WriteAsync \before\corerun.exe 104857600 4096 Asynchronous 355,626.4 us 1.00 2,257,880 B

@adamsitnik adamsitnik added area-System.IO tenet-performance Performance related issue labels Apr 19, 2021
@adamsitnik adamsitnik added this to the 6.0.0 milestone Apr 19, 2021
@ghost
Copy link

ghost commented Apr 19, 2021

Tagging subscribers to this area: @carlossanlop
See info in area-owners.md if you want to be subscribed.

Issue Details

When nothing has been written to the buffer yet (_writePos == 0) and the size of the buffer passed to WriteAsync is the same as internal FileStream buffer, we should omit buffering logic and just call the actual strategy directly.

This is ofc an edge case, but since we already have a benchmark for it and it's relatively small change we should IMO change it.

See the Allocated column:

Method Toolchain fileSize userBufferSize options Mean Ratio Allocated
WriteAsync \after\corerun.exe 1048576 4096 None 3,708.7 us 1.00 29,321 B
WriteAsync \before\corerun.exe 1048576 4096 None 3,719.5 us 1.00 55,969 B
WriteAsync \after\corerun.exe 1048576 4096 Asynchronous 4,141.6 us 0.98 913 B
WriteAsync \before\corerun.exe 1048576 4096 Asynchronous 4,238.3 us 1.00 27,561 B
WriteAsync \after\corerun.exe 104857600 4096 None 212,481.6 us 0.86 2,867,896 B
WriteAsync \before\corerun.exe 104857600 4096 None 246,750.4 us 1.00 5,124,856 B
WriteAsync \after\corerun.exe 104857600 4096 Asynchronous 348,312.7 us 0.98 960 B
WriteAsync \before\corerun.exe 104857600 4096 Asynchronous 355,626.4 us 1.00 2,257,880 B
Author: adamsitnik
Assignees: -
Labels:

area-System.IO, tenet-performance

Milestone: 6.0.0

@adamsitnik adamsitnik mentioned this pull request Apr 19, 2021
5 tasks
@jeffhandley
Copy link
Member

@adamsitnik / @carlossanlop / @jozkee -- do we want to get this in for Preview 5 with the other FileStream follow-up work?

@adamsitnik adamsitnik requested a review from carlossanlop May 10, 2021 09:48
@adamsitnik
Copy link
Member Author

@carlossanlop @jozkee I've added a comment that hopefully explains why bypassing the cache is possible in this particular scenario. PTAL

Updated perf numbers:

Method Toolchain fileSize userBufferSize options Allocated
WriteAsync \after\corerun.exe 1048576 4096 Asynchronous 937 B
WriteAsync \before\corerun.exe 1048576 4096 Asynchronous 27,561 B
WriteAsync \after\corerun.exe 104857600 4096 None 2,867,896 B
WriteAsync \before\corerun.exe 104857600 4096 None 5,125,608 B
WriteAsync \after\corerun.exe 104857600 4096 Asynchronous 2,552 B
WriteAsync \before\corerun.exe 104857600 4096 Asynchronous 2,259,464 B

Copy link
Member

@jozkee jozkee 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.

@jozkee jozkee merged commit de4c481 into dotnet:main May 10, 2021
@adamsitnik adamsitnik deleted the edgeCasePerf branch May 11, 2021 07:16
@ghost ghost locked as resolved and limited conversation to collaborators Jun 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.IO tenet-performance Performance related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants