Cache GetFileInformationByHandleEx (Length) when FileShare indicates that no one else can change it #49638

jozkee · 2021-03-15T08:39:23Z

This change builds on top of #49145
Adds caching logic for GetFileInformationByHandleEx when the FileStream is constructed with FileShare.Read or FileShare.None.

Fixes #49145

The following benchmark results show ~15-19% gains on ReadAsync when is indicated to use Asynchronous I/O.
I am using the new FileStream implementation (DOTNET_SYSTEM_IO_USELEGACYFILESTREAM = false).

Method	Job	fileSize	userBufferSize	options	Mean	Error	Ratio	Allocated
ReadAsync	Old	1024	512	None	89.38 us	1.723 us	1.00	5,120 B
ReadAsync	New	1024	512	None	87.69 us	0.558 us	0.98	5,144 B

WriteAsync	Old	1024	512	None	885.15 us	48.616 us	1.00	4,464 B
WriteAsync	New	1024	512	None	807.87 us	29.672 us	0.92	4,488 B

ReadAsync	Old	1024	512	Asynchronous	105.15 us	1.423 us	1.00	5,288 B
ReadAsync	New	1024	512	Asynchronous	104.00 us	1.445 us	0.99	5,312 B

WriteAsync	Old	1024	512	Asynchronous	879.85 us	35.749 us	1.00	4,879 B
WriteAsync	New	1024	512	Asynchronous	874.74 us	30.282 us	1.00	4,884 B

ReadAsync	Old	1024	4096	None	86.27 us	1.289 us	1.00	792 B
ReadAsync	New	1024	4096	None	86.22 us	1.293 us	1.00	816 B

WriteAsync	Old	1024	4096	None	166.10 us	7.568 us	1.00	256 B
WriteAsync	New	1024	4096	None	172.56 us	8.485 us	1.04	280 B

ReadAsync	Old	1024	4096	Asynchronous	102.13 us	1.818 us	1.00	952 B
ReadAsync	New	1024	4096	Asynchronous	102.81 us	1.699 us	1.01	976 B

WriteAsync	Old	1024	4096	Asynchronous	167.29 us	9.127 us	1.00	304 B
WriteAsync	New	1024	4096	Asynchronous	176.79 us	7.937 us	1.06	328 B

ReadAsync	Old	1048576	512	None	1,483.95 us	18.988 us	1.00	86,721 B
ReadAsync	New	1048576	512	None	1,479.23 us	26.571 us	1.00	86,747 B

WriteAsync	Old	1048576	512	None	6,568.55 us	198.439 us	1.00	78,544 B
WriteAsync	New	1048576	512	None	6,760.47 us	259.103 us	1.03	78,546 B

ReadAsync	Old	1048576	512	Asynchronous	4,497.23 us	93.600 us	1.00	95,049 B
ReadAsync	New	1048576	512	Asynchronous	3,815.88 us	115.670 us	0.85	95,068 B

WriteAsync	Old	1048576	512	Asynchronous	10,620.21 us	233.398 us	1.00	86,819 B
WriteAsync	New	1048576	512	Asynchronous	10,656.66 us	277.135 us	1.00	86,950 B

ReadAsync	Old	1048576	4096	None	1,144.11 us	22.094 us	1.00	29,352 B
ReadAsync	New	1048576	4096	None	1,179.12 us	21.193 us	1.03	29,378 B

WriteAsync	Old	1048576	4096	None	6,079.26 us	233.995 us	1.00	29,357 B
WriteAsync	New	1048576	4096	None	6,141.99 us	159.788 us	1.01	29,381 B

ReadAsync	Old	1048576	4096	Asynchronous	4,611.24 us	88.743 us	1.00	80,520 B
ReadAsync	New	1048576	4096	Asynchronous	3,897.85 us	77.199 us	0.85	80,538 B

WriteAsync	Old	1048576	4096	Asynchronous	10,712.67 us	244.516 us	1.00	80,581 B
WriteAsync	New	1048576	4096	Asynchronous	10,789.44 us	299.409 us	1.01	80,530 B

ReadAsync	Old	104857600	512	None	155,293.59 us	1,602.725 us	1.00	8,196,848 B
ReadAsync	New	104857600	512	None	157,621.19 us	1,571.001 us	1.01	8,196,872 B

WriteAsync	Old	104857600	512	None	241,062.33 us	3,875.219 us	1.00	7,379,888 B
WriteAsync	New	104857600	512	None	246,557.78 us	3,062.351 us	1.02	7,378,440 B

ReadAsync	Old	104857600	512	Asynchronous	465,425.69 us	6,185.534 us	1.00	9,014,856 B
ReadAsync	New	104857600	512	Asynchronous	378,101.69 us	6,850.969 us	0.81	9,016,752 B

WriteAsync	Old	104857600	512	Asynchronous	506,735.09 us	11,340.181 us	1.00	8,198,504 B
WriteAsync	New	104857600	512	Asynchronous	500,059.39 us	9,153.116 us	0.99	8,197,048 B

ReadAsync	Old	104857600	4096	None	121,764.07 us	1,451.441 us	1.00	2,867,928 B
ReadAsync	New	104857600	4096	None	121,319.25 us	1,313.024 us	1.00	2,867,928 B

WriteAsync	Old	104857600	4096	None	180,849.73 us	3,577.698 us	1.00	2,867,920 B
WriteAsync	New	104857600	4096	None	180,161.77 us	2,351.753 us	1.00	2,867,944 B

ReadAsync	Old	104857600	4096	Asynchronous	468,659.47 us	8,279.625 us	1.00	7,988,624 B
ReadAsync	New	104857600	4096	Asynchronous	382,810.03 us	7,409.432 us	0.82	7,991,592 B

WriteAsync	Old	104857600	4096	Asynchronous	494,416.48 us	7,939.347 us	1.00	7,988,616 B
WriteAsync	New	104857600	4096	Asynchronous	497,001.13 us	9,644.305 us	1.01	7,988,640 B

remove finalizer from FileStream, keep it only in DerivedFileStreamStrategy and LegacyFileStreamStrategy

… functional they can now be used directly without any buffering on top of them!

…NothingToFlush_CompletesSynchronously passing

…mplementation

…ithin the file, not a removal and addition)

when users have a race condition in their code (i.e. they call Close when calling another method on Stream like Read).

…a bug

…re not providing too much gains

…chronize it

…et in memory

…llow other processes to modify it

adamsitnik

The perf gains are really impressive!

adamsitnik · 2021-03-17T19:08:10Z

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

            // Note: Cleaner to set the following fields in ValidateAndInitFromHandle,
            // but we can't as they're readonly.
            _access = access;
+            _share = FileStream.DefaultShare;


instead of using FileStream.DefaultShare here you can add FileShare share to the ctor arguments and pass FileStream.DefaultShare from FileStream. We are already doing similar thing:

runtime/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs

Line 106 in 72a4e94

this(path, mode, access, DefaultShare, DefaultBufferSize, DefaultIsAsync)

adamsitnik · 2021-03-17T19:12:20Z

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

        private readonly bool _isPipe;      // Whether to disable async buffering code.

        private long _appendStart; // When appending, prevent overwriting file.
+        private long? _length;


what would be the difference in the allocated memory if we would use non-nullable long field and use negative values instead null to determine whether the field was initialized or not?

This field doesn't actually allocate for being nullable; Nullable<T> is a struct, but it makes sense to use negatives instead.

This field doesn't actually allocate for being nullable

Is sizeof(long) == sizeof(long?)?

Is sizeof(long) == sizeof(long?)?

Nope, it is 8 and 16 respectively on my machine. I thought you were referring to heap allocations, I will prefer long for the upcoming pull request, thanks.

carlossanlop

LGTM so far. Questions:

Do you plan on waiting for Adam's refactoring PR to get merged to avoid any potential conflicts, or are we expecting this PR to get merged first?
Adam merged his rewrite PR already. Can you please rebase this?

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

jozkee · 2021-03-18T03:09:03Z

Adam merged his rewrite PR already. Can you please rebase this?

@carlossanlop I will close this PR and send a new one that combines #49145 and this one, see also #49145 (comment).

Do you plan on waiting for Adam's refactoring PR to get merged to avoid any potential conflicts, or are we expecting this PR to get merged first?

For the upcoming PR that I mentioned above, I don't have a preference, if Adam's PR (#49750) gets in first, I can work on fixing the conflics.

adamsitnik and others added 20 commits March 11, 2021 20:08

introduce WindowsFileStreamStrategy

30d8ef7

introduce SyncWindowsFileStreamStrategy

5c8018d

introduce AsyncWindowsFileStreamStrategy

2e159c4

some minor improvements after reading the code again

5e87d72

only DerivedFileStreamStrategy needs to have a reference to FileStream

f5c2470

remove finalizer from FileStream, keep it only in DerivedFileStreamStrategy and LegacyFileStreamStrategy

implement ReadByte and WriteByte to make new windows strategies fully…

71e7d43

… functional they can now be used directly without any buffering on top of them!

implement FlushAsync for no buffering strategies as nop to get Flush_…

367ad84

…NothingToFlush_CompletesSynchronously passing

use the new strategies when buffering is not enabled

5579e94

introduce BufferedFileStreamStrategy

8bbbcee

fix the Mono build

9e9cb1f

use the Legacy strategy by default for now, add tests for the other i…

53752e7

…mplementation

restore old file name to make it easier to review the code (as diff w…

e2eec8b

…ithin the file, not a removal and addition)

Don't set the buffer to null, to avoid a NullReferenceException

20640fa

when users have a race condition in their code (i.e. they call Close when calling another method on Stream like Read).

fix the browser build?

dd7e9ba

reverting the flushing changes as it looks that they have introduced …

2a35ae6

…a bug

simplify the source code by removing some of the optimizations that a…

b9afbff

…re not providing too much gains

don't verify OS handle position

2029317

track the file offset in memory, don't use expensive sys calls to syn…

f6c8647

…chronize it

there is no need to set the Length since we are now tracking the offs…

60eedb4

…et in memory

Cache GetFileInformationByHandleEx (Length) when FileShare does not a…

72a4e94

…llow other processes to modify it

jozkee added the area-System.IO label Mar 15, 2021

jozkee added this to the 6.0.0 milestone Mar 15, 2021

jozkee requested review from adamsitnik, carlossanlop and stephentoub March 15, 2021 08:39

jozkee self-assigned this Mar 15, 2021

adamsitnik mentioned this pull request Mar 17, 2021

FileStream rewrite part II #48813

Merged

adamsitnik reviewed Mar 17, 2021

View reviewed changes

carlossanlop reviewed Mar 18, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/WindowsFileStreamStrategy.cs Show resolved Hide resolved

src/libraries/System.Private.CoreLib/src/System/IO/WindowsFileStreamStrategy.cs Show resolved Hide resolved

jozkee mentioned this pull request Mar 22, 2021

FileStream optimizations #49975

Merged

jozkee closed this Mar 22, 2021

jozkee deleted the fs_length branch March 24, 2021 19:29

ghost locked as resolved and limited conversation to collaborators Apr 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Cache GetFileInformationByHandleEx (Length) when FileShare indicates that no one else can change it #49638

Cache GetFileInformationByHandleEx (Length) when FileShare indicates that no one else can change it #49638

Uh oh!

jozkee commented Mar 15, 2021 •

edited

Loading

Uh oh!

adamsitnik left a comment

Uh oh!

adamsitnik Mar 17, 2021

Uh oh!

adamsitnik Mar 17, 2021

Uh oh!

jozkee Mar 17, 2021

Uh oh!

adamsitnik Mar 17, 2021

Uh oh!

jozkee Mar 17, 2021

Uh oh!

carlossanlop left a comment

Uh oh!

Uh oh!

Uh oh!

jozkee commented Mar 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Cache GetFileInformationByHandleEx (Length) when FileShare indicates that no one else can change it #49638

Cache GetFileInformationByHandleEx (Length) when FileShare indicates that no one else can change it #49638

Uh oh!

Conversation

jozkee commented Mar 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamsitnik left a comment

Choose a reason for hiding this comment

Uh oh!

adamsitnik Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

adamsitnik Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

jozkee Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

adamsitnik Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

jozkee Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

carlossanlop left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jozkee commented Mar 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jozkee commented Mar 15, 2021 •

edited

Loading