File.*AllText* optimizations by adamsitnik · Pull Request #58167 · dotnet/runtime

adamsitnik · 2021-08-26T10:29:36Z

The reduced memory allocations come from lack of StreamWriter and FileStream buffer allocations.

The reduced CPU time comes mostly from using bigger buffers and setting file preallocation size.

Windows

Details

Method	Job	size	Ratio	Allocated
AppendAllText	after	100	1.00	120 B
AppendAllText	before	100	1.00	6,576 B

WriteAllText	after	100	1.00	120 B
WriteAllText	before	100	1.00	6,576 B

AppendAllTextAsync	after	100	1.00	697 B
AppendAllTextAsync	before	100	1.00	10,586 B

WriteAllTextAsync	after	100	1.00	761 B
WriteAllTextAsync	before	100	1.00	10,649 B

AppendAllText	after	10000	0.87	121 B
AppendAllText	before	10000	1.00	9,680 B

WriteAllText	after	10000	0.80	125 B
WriteAllText	before	10000	1.00	9,683 B

AppendAllTextAsync	after	10000	0.85	698 B
AppendAllTextAsync	before	10000	1.00	11,049 B

WriteAllTextAsync	after	10000	0.43	768 B
WriteAllTextAsync	before	10000	1.00	11,256 B

WriteAllText	after	30000	0.75	125 B
WriteAllText	before	30000	1.00	9,683 B

WriteAllTextAsync	after	30000	0.54	765 B
WriteAllTextAsync	before	30000	1.00	11,997 B

WriteAllText	after	70000	0.79	126 B
WriteAllText	before	70000	1.00	9,683 B

WriteAllTextAsync	after	70000	0.49	767 B
WriteAllTextAsync	before	70000	1.00	13,387 B

WriteAllText	after	100000	0.77	124 B
WriteAllText	before	100000	1.00	9,683 B

WriteAllTextAsync	after	100000	0.58	770 B
WriteAllTextAsync	before	100000	1.00	14,630 B

Ubuntu

Details

Method	Toolchain	size	Ratio	Allocated
AppendAllText	/after/corerun	100	0.73	112 B
AppendAllText	/before/corerun	100	1.00	6,568 B

WriteAllText	/after/corerun	100	0.94	112 B
WriteAllText	/before/corerun	100	1.00	6,568 B

AppendAllTextAsync	/after/corerun	100	0.60	480 B
AppendAllTextAsync	/before/corerun	100	1.00	10,368 B

WriteAllTextAsync	/after/corerun	100	0.81	544 B
WriteAllTextAsync	/before/corerun	100	1.00	10,427 B

AppendAllText	/after/corerun	10000	0.72	112 B
AppendAllText	/before/corerun	10000	1.00	9,672 B

WriteAllText	/after/corerun	10000	0.69	112 B
WriteAllText	/before/corerun	10000	1.00	9,672 B

AppendAllTextAsync	/after/corerun	10000	0.63	480 B
AppendAllTextAsync	/before/corerun	10000	1.00	10,890 B

WriteAllTextAsync	/after/corerun	10000	0.44	497 B
WriteAllTextAsync	/before/corerun	10000	1.00	11,072 B

WriteAllText	/after/corerun	100000	0.56	112 B
WriteAllText	/before/corerun	100000	1.00	9,673 B

WriteAllTextAsync	/after/corerun	100000	0.43	545 B
WriteAllTextAsync	/before/corerun	100000	1.00	14,413 B

ghost · 2021-08-26T10:29:40Z

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

Issue Details

The reduced memory allocations come from lack of StreamWriter and FileStream buffer allocations.

The reduced CPU time comes mostly from using bigger buffers and setting file preallocation size.

Method	Job	size	Ratio	Allocated
AppendAllText	after	100	1.00	121 B
AppendAllText	before	100	1.00	6,576 B

WriteAllText	after	100	1.01	121 B
WriteAllText	before	100	1.00	6,577 B

AppendAllTextAsync	after	100	1.00	713 B
AppendAllTextAsync	before	100	1.00	10,585 B

WriteAllTextAsync	after	100	0.98	778 B
WriteAllTextAsync	before	100	1.00	10,648 B

AppendAllText	after	10000	0.91	121 B
AppendAllText	before	10000	1.00	9,680 B

WriteAllText	after	10000	0.85	123 B
WriteAllText	before	10000	1.00	9,682 B

AppendAllTextAsync	after	10000	0.84	714 B
AppendAllTextAsync	before	10000	1.00	11,048 B

WriteAllTextAsync	after	10000	0.89	789 B
WriteAllTextAsync	before	10000	1.00	11,242 B

WriteAllText	after	100000	0.85	126 B
WriteAllText	before	100000	1.00	9,684 B

WriteAllTextAsync	after	100000	0.72	785 B
WriteAllTextAsync	before	100000	1.00	14,598 B

Author:	adamsitnik
Assignees:	-
Labels:	`area-System.IO`, `tenet-performance`
Milestone:	7.0.0

adamsitnik · 2021-08-26T10:52:55Z

@jozkee please let me know if you would like me to separate the refactor from the optimizations

src/libraries/System.Private.CoreLib/src/System/IO/File.cs

stephentoub · 2021-08-26T13:47:17Z

src/libraries/System.Private.CoreLib/src/System/IO/File.cs

        {
-            if (path == null)
-                throw new ArgumentNullException(nameof(path));
+            Validate(path, encoding);


It's probably fine, but this does change which exception will be thrown if there are multiple things wrong.

src/libraries/System.Private.CoreLib/src/System/IO/File.cs

src/libraries/System.Private.CoreLib/src/System/IO/File.netcoreapp.cs

danmoseley · 2021-08-26T14:33:34Z

I assume no new tests are warranted as it's purely perf, but having touched this code can you see any we maybe are missing?

…need to store the preamble

…nefit, as it requires additional sys-call(s)

Co-authored-by: Stephen Toub <stoub@microsoft.com>

adamsitnik · 2021-08-27T16:28:29Z

@stephentoub @danmoseley I've added missing tests, which identified a bug, fixed it and updated perf numbers to include Linux numbers as well. On Linux it's now even two times faster for large strings!

src/libraries/System.Private.CoreLib/src/System/IO/File.netcoreapp.cs

src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.cs

src/libraries/System.Private.CoreLib/src/System/IO/File.netcoreapp.cs

jozkee

Otherwise; LGTM, Thanks.

src/libraries/System.Private.CoreLib/src/System/IO/File.netcoreapp.cs

src/libraries/System.Private.CoreLib/src/System/IO/File.cs

src/libraries/System.IO.FileSystem/tests/File/ReadWriteAllText.cs

src/libraries/System.IO.FileSystem/tests/FileInfo/AppendText.cs

adamsitnik · 2021-10-12T07:35:36Z

I am closing the PR as it's pointless with the regression that has been recently introduced and backported to 6.0 (#59705)

stephentoub · 2021-10-12T11:54:08Z

Conversely, it was also buggy after:
#58726

https://github.com/adamsitnik/runtime/blob/e0dafc540a0d6c532fa3f97f269a1b59ecd6e8ca/src/libraries/System.Private.CoreLib/src/System/IO/File.netcoreapp.cs#L244

stephentoub · 2021-10-12T11:58:50Z

You'd made a bunch of style changes, consolidating input validation, etc. Do you want to keep those?

adamsitnik · 2021-10-12T13:05:21Z

Conversely, it was also buggy after

It was, because I wanted to clarify how preallocationSize should work, make sure that everyone agrees and then update this PR.

# Conflicts: # src/libraries/System.Private.CoreLib/src/System/IO/File.cs

Co-authored-by: David Cantú <dacantu@microsoft.com>

adamsitnik · 2021-10-14T14:39:39Z

I was able to get some nice perf wins anyway (mostly due to using larger buffers which reduced the number of sys-calls). The failures are not related (System.IO.Tests.FileStream_DeleteOnClose.OpenOrCreate_DeleteOnClose_UsableAsMutex), I am merging it.

danmoseley · 2021-10-14T19:03:12Z

Nice!

kunalspathak · 2021-10-19T15:27:21Z

Ubuntu x64 improvements - dotnet/perf-autofiling-issues#1897

kunalspathak · 2021-10-19T15:37:24Z

alpine improvements - dotnet/perf-autofiling-issues#1922

kunalspathak · 2021-10-19T15:42:00Z

windows x64 improvements - dotnet/perf-autofiling-issues#1933

danmoseley · 2021-10-19T17:41:36Z

woo hoo.

also, @kunalspathak @adamsitnik what happened here? it's not obvious to me that this change improved File.Exists --

kunalspathak · 2021-10-19T17:43:36Z

It is bimodal.

https://pvscmdupload.blob.core.windows.net/reports/allTestHistory/refs/heads/main_x64_Windows%2010.0.19042/amd/System.IO.Tests.Perf_File.Exists.html

danmoseley · 2021-10-19T17:46:43Z

A curious bimodality that seems to stay in one state for a week or more. I wonder whether iterations within an individual run are stable? I don't see a way to determine from the report.

AndyAyersMS · 2021-11-23T17:21:32Z

Improvement dotnet/perf-autofiling-issues#1795 (click on full history to see)

adamsitnik · 2021-11-23T18:29:28Z

It's interesting that it got both faster and more stable despite still being an IO benchmark. I suspect that it might be caused by using the preallocation size which avoids fragmentation.

danmoseley · 2021-11-24T05:22:35Z

That is a beautiful graph. Nice.

jeffhandley · 2021-11-26T04:01:08Z

That is a beautiful graph. Nice.

Wow; it really is! Great work, and I love seeing the data loop closed like this, attributing the improvements to the change.

adamsitnik added 3 commits August 26, 2021 11:38

optimize File.*AllText* APIs

c7dc3a9

remove code duplication

fe18dbf

use expression bodies

50a3528

adamsitnik added area-System.IO tenet-performance Performance related issue labels Aug 26, 2021

adamsitnik added this to the 7.0.0 milestone Aug 26, 2021

adamsitnik requested a review from jozkee August 26, 2021 10:29

adamsitnik changed the title ~~File optimizations~~ File.*AllText* optimizations Aug 26, 2021

stephentoub reviewed Aug 26, 2021

View reviewed changes

adamsitnik and others added 7 commits August 27, 2021 07:43

ensure there are no trailing zeroes when EOF < preallocationSize on Unix

9b81560

ensure that all code paths are covered by tests

878c4fa

fix bug discovered by new tests: when the content is empty, we still …

328cdaf

…need to store the preamble

flush the encoder on last loop iteration

8da83ee

for a single write operation setting preallocationSize has no perf be…

a992375

…nefit, as it requires additional sys-call(s)

Apply suggestions from code review

eb8f1bf

Co-authored-by: Stephen Toub <stoub@microsoft.com>

style consistency

7eaaa48

stephentoub reviewed Aug 30, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/File.netcoreapp.cs Outdated Show resolved Hide resolved

stephentoub reviewed Aug 30, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.cs Outdated Show resolved Hide resolved

stephentoub reviewed Aug 30, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/File.netcoreapp.cs Outdated Show resolved Hide resolved

address code review feedback

82190c5

campersau reviewed Aug 30, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/File.netcoreapp.cs Outdated Show resolved Hide resolved

don't write a preamble to non-empty file

e0dafc5

runfoapp bot mentioned this pull request Aug 30, 2021

Feed unreliability affecting CI #55449

Closed

adamsitnik mentioned this pull request Aug 30, 2021

Relax RandomAccess type requirements, make all Read*|Write* methods work with non-seekable files #58381

Open

3 tasks

jozkee approved these changes Sep 2, 2021

View reviewed changes

adamsitnik mentioned this pull request Sep 14, 2021

[release/6.0] Align preallocationSize behavior #59078

Closed

adamsitnik closed this Oct 12, 2021

adamsitnik added 4 commits October 13, 2021 15:14

Merge remote-tracking branch 'upstream/main' into fileOptimizations

982d1d6

# Conflicts: # src/libraries/System.Private.CoreLib/src/System/IO/File.cs

use exact size

256e5f0

Merge remote-tracking branch 'upstream/main' into fileOptimizations

95b474a

MS_IO_REDIST has been removed

dbecac2

adamsitnik reopened this Oct 14, 2021

adamsitnik and others added 2 commits October 14, 2021 12:11

Apply suggestions from code review

280009a

Co-authored-by: David Cantú <dacantu@microsoft.com>

remove redundant file size check that was failing for character devices

8a67c96

adamsitnik merged commit cf49643 into dotnet:main Oct 14, 2021

adamsitnik deleted the fileOptimizations branch October 14, 2021 14:40

This was referenced Oct 14, 2021

System.IO.Tests.File_ReadWriteAllBytes.ReadAllBytes_NonSeekableFileStream_InWindows failed #60427

Open

system.io.tests.file_readwriteallbytes.readallbytes_nonseekablefilestream_inwindows #60444

Closed

adamsitnik mentioned this pull request Nov 15, 2021

Minor File.ReadAllBytes* improvements #61519

Merged

ghost locked as resolved and limited conversation to collaborators Nov 18, 2021

Conversation

adamsitnik commented Aug 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Windows

Ubuntu

Uh oh!

ghost commented Aug 26, 2021

Uh oh!

adamsitnik commented Aug 26, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stephentoub Aug 26, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danmoseley commented Aug 26, 2021

Uh oh!

adamsitnik commented Aug 27, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jozkee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adamsitnik commented Oct 12, 2021

Uh oh!

stephentoub commented Oct 12, 2021

Uh oh!

stephentoub commented Oct 12, 2021

Uh oh!

adamsitnik commented Oct 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamsitnik commented Oct 14, 2021

Uh oh!

danmoseley commented Oct 14, 2021

Uh oh!

kunalspathak commented Oct 19, 2021

Uh oh!

kunalspathak commented Oct 19, 2021

Uh oh!

kunalspathak commented Oct 19, 2021

Uh oh!

danmoseley commented Oct 19, 2021

Uh oh!

kunalspathak commented Oct 19, 2021

Uh oh!

danmoseley commented Oct 19, 2021

Uh oh!

AndyAyersMS commented Nov 23, 2021

Uh oh!

adamsitnik commented Nov 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danmoseley commented Nov 24, 2021

Uh oh!

jeffhandley commented Nov 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

adamsitnik commented Aug 26, 2021 •

edited

Loading

adamsitnik commented Oct 12, 2021 •

edited

Loading

adamsitnik commented Nov 23, 2021 •

edited

Loading