Reduce the cost of creating a MemoryMappedFile by adamsitnik · Pull Request #63790 · dotnet/runtime

adamsitnik · 2022-01-14T14:00:00Z

Explanation:

when creating a memory mapped file, we don't need to fetch the file size multiple times. We can do it once and reduce the number of sys-calls.
there is no need to use FileStream, we can use more lightweight SafeFileHandle. It reduces the managed allocations.

Windows

Method	Toolchain	capacity	Mean	Ratio	Allocated
CreateFromFile	\main\corerun.exe	10000	43.132 us	1.00	448 B
CreateFromFile	\pr\corerun.exe	10000	40.030 us	0.93	288 B

CreateFromFile	\main\corerun.exe	100000	43.919 us	1.00	448 B
CreateFromFile	\pr\corerun.exe	100000	40.961 us	0.93	288 B

CreateFromFile	\main\corerun.exe	1000000	47.123 us	1.00	448 B
CreateFromFile	\pr\corerun.exe	1000000	44.099 us	0.94	288 B

CreateFromFile	\main\corerun.exe	10000000	57.808 us	1.00	448 B
CreateFromFile	\pr\corerun.exe	10000000	54.653 us	0.94	288 B

Unix

Method	Toolchain	capacity	Mean	Ratio	Allocated
CreateNew	/main/corerun	10000	8.028 us	1.00	448 B
CreateNew	/pr/corerun	10000	6.197 us	0.77	280 B

CreateFromFile	/main/corerun	10000	43.379 us	1.00	480 B
CreateFromFile	/pr/corerun	10000	35.422 us	0.83	312 B

CreateNew	/main/corerun	100000	8.051 us	1.00	448 B
CreateNew	/pr/corerun	100000	6.189 us	0.77	280 B

CreateFromFile	/main/corerun	100000	39.091 us	1.00	480 B
CreateFromFile	/pr/corerun	100000	35.072 us	0.90	312 B

CreateNew	/main/corerun	1000000	7.765 us	1.00	448 B
CreateNew	/pr/corerun	1000000	5.935 us	0.77	280 B

CreateFromFile	/main/corerun	1000000	38.829 us	1.00	480 B
CreateFromFile	/pr/corerun	1000000	35.471 us	0.91	312 B

CreateNew	/main/corerun	10000000	7.913 us	1.00	448 B
CreateNew	/pr/corerun	10000000	5.934 us	0.75	280 B

CreateFromFile	/main/corerun	10000000	37.554 us	1.00	480 B
CreateFromFile	/pr/corerun	10000000	34.940 us	0.93	312 B

Related to #62768

…File

ghost · 2022-01-14T14:00:11Z

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

Issue Details

Explanation:

when creating a memory mapped file, we don't need to fetch the file size multiple times. We can do it once and reduce the number of sys-calls.
there is no need to use FileStream, we can use more lightweight SafeFileHandle. It reduces the managed allocations.

Windows

Method	Toolchain	capacity	Mean	Ratio	Allocated
CreateFromFile	\main\corerun.exe	10000	43.132 us	1.00	448 B
CreateFromFile	\pr\corerun.exe	10000	40.030 us	0.93	288 B

CreateFromFile	\main\corerun.exe	100000	43.919 us	1.00	448 B
CreateFromFile	\pr\corerun.exe	100000	40.961 us	0.93	288 B

CreateFromFile	\main\corerun.exe	1000000	47.123 us	1.00	448 B
CreateFromFile	\pr\corerun.exe	1000000	44.099 us	0.94	288 B

CreateFromFile	\main\corerun.exe	10000000	57.808 us	1.00	448 B
CreateFromFile	\pr\corerun.exe	10000000	54.653 us	0.94	288 B

Unix

Method	Toolchain	capacity	Mean	Ratio	Allocated
CreateNew	/main/corerun	10000	8.028 us	1.00	448 B
CreateNew	/pr/corerun	10000	6.197 us	0.77	280 B

CreateFromFile	/main/corerun	10000	43.379 us	1.00	480 B
CreateFromFile	/pr/corerun	10000	35.422 us	0.83	312 B

CreateNew	/main/corerun	100000	8.051 us	1.00	448 B
CreateNew	/pr/corerun	100000	6.189 us	0.77	280 B

CreateFromFile	/main/corerun	100000	39.091 us	1.00	480 B
CreateFromFile	/pr/corerun	100000	35.072 us	0.90	312 B

CreateNew	/main/corerun	1000000	7.765 us	1.00	448 B
CreateNew	/pr/corerun	1000000	5.935 us	0.77	280 B

CreateFromFile	/main/corerun	1000000	38.829 us	1.00	480 B
CreateFromFile	/pr/corerun	1000000	35.471 us	0.91	312 B

CreateNew	/main/corerun	10000000	7.913 us	1.00	448 B
CreateNew	/pr/corerun	10000000	5.934 us	0.75	280 B

CreateFromFile	/main/corerun	10000000	37.554 us	1.00	480 B
CreateFromFile	/pr/corerun	10000000	34.940 us	0.93	312 B

Related to #62768

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

...libraries/System.IO.MemoryMappedFiles/src/Microsoft/Win32/SafeMemoryMappedFileHandle.Unix.cs

jkotas · 2022-01-14T14:33:56Z

...libraries/System.IO.MemoryMappedFiles/src/Microsoft/Win32/SafeMemoryMappedFileHandle.Unix.cs

            SetHandle(handlePtr);
        }

-        protected override void Dispose(bool disposing)


Is this going be an observable behavior change for user defined FileStreams? The potential user-defined Dispose is not going to be called anymore.

Yes, although that looks like it's already a discrepancy between our Windows and Unix implementation, with the Windows implementation not Dispose'ing of the file stream and the Unix implementation doing so (unless I'm missing it in the code). While it would then be a behavior change on Unix, as the Unix implementation is trying to match the pre-existing Windows semantics it seems like a reasonable bug fix.

...libraries/System.IO.MemoryMappedFiles/src/Microsoft/Win32/SafeMemoryMappedFileHandle.Unix.cs

stephentoub · 2022-01-14T14:48:34Z

src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.cs

+            long fileSize = mode switch
+            {
+                FileMode.CreateNew or FileMode.Create => 0, // the file is brand new
+                _ => RandomAccess.GetLength(fileHandle)


If GetLength throws, the file handle won't be Dispose'd.

Also, doesn't this throw for non-regular files? Is the exception that occurs in such a case the same as it was before?

If GetLength throws, the file handle won't be Dispose'd.

that is correct, I am going to add a try-catch block

Also, doesn't this throw for non-regular files?

both FileStream.Length and RandomAccess.GetLength throw for non-seekable files.

the special character device files report that they are seekable and of a size equal zero

Do we have tests that validate the exception in this case, both with capacity == 0 and capacity != 0?

Do we have tests that validate the exception in this case, both with capacity == 0 and capacity != 0?

We don't. For both cases, the current implementation would throw for non-seekable file (without closing the handle).

For capacity == 0 here when accessing fileStream.Length:

runtime/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.cs

Line 159 in 8d8ef6d

if (capacity == 0 && fileStream.Length == 0)

For capacity != 0 here when accessing fileStream.Length:

runtime/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Windows.cs

Lines 16 to 22 in 8d8ef6d

if (access == MemoryMappedFileAccess.Read && capacity > fileStream.Length)

{

throw new ArgumentException(SR.Argument_ReadAccessWithLargeCapacity);

}

// one can always create a small view if they do not want to map an entire file

if (fileStream.Length > capacity)

@stephentoub I've sent a PR with tests #63927

Once it's gets merged, I am going to sync this PR with main branch.

@stephentoub I've merged the tests into this PR and they are all passing. PTAL

src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.cs

...braries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.cs

adamsitnik added 3 commits January 14, 2022 10:54

there is no need for buffering

c87f002

don't fetch the file size more than once when creating a MemoryMapped…

7be9d39

…File

use SafeFileHandle instead FileStream

61e83b6

adamsitnik added area-System.IO tenet-performance Performance related issue labels Jan 14, 2022

adamsitnik added this to the 7.0.0 milestone Jan 14, 2022

adamsitnik requested review from jozkee and stephentoub January 14, 2022 14:00

ghost assigned adamsitnik Jan 14, 2022

adamsitnik commented Jan 14, 2022

View reviewed changes

...libraries/System.IO.MemoryMappedFiles/src/Microsoft/Win32/SafeMemoryMappedFileHandle.Unix.cs Show resolved Hide resolved

jkotas reviewed Jan 14, 2022

View reviewed changes

stephentoub reviewed Jan 14, 2022

View reviewed changes

...libraries/System.IO.MemoryMappedFiles/src/Microsoft/Win32/SafeMemoryMappedFileHandle.Unix.cs Show resolved Hide resolved

stephentoub reviewed Jan 14, 2022

View reviewed changes

src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.cs Show resolved Hide resolved

stephentoub reviewed Jan 14, 2022

View reviewed changes

src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.cs Outdated Show resolved Hide resolved

stephentoub reviewed Jan 14, 2022

View reviewed changes

...braries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.cs Outdated Show resolved Hide resolved

apply code review feedback

b4d1d7e

adamsitnik requested review from MichalStrehovsky, SamMonoRT, imhameed, lambdageek, marek-safar, thaystg and vargaz as code owners January 19, 2022 07:44

ensure the handle is disposed when fetching the file size fails

59e4f24

adamsitnik force-pushed the moreMemoryMappedPerf branch from 37e5f05 to 59e4f24 Compare January 19, 2022 07:49

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

8eb958b

adamsitnik removed request for marek-safar and vargaz January 19, 2022 07:53

adamsitnik removed request for MichalStrehovsky, SamMonoRT, imhameed, lambdageek and thaystg January 19, 2022 07:53

stephentoub approved these changes Feb 7, 2022

View reviewed changes

adamsitnik merged commit a24364a into dotnet:main Feb 8, 2022

adamsitnik deleted the moreMemoryMappedPerf branch February 8, 2022 09:17

adamsitnik mentioned this pull request Feb 9, 2022

MemoryMappedFile always flushing on dispose - even if nothing was written? #62768

Closed

EgorBo mentioned this pull request Feb 15, 2022

[Perf] Changes at 2/8/2022 9:17:51 AM dotnet/perf-autofiling-issues#3530

Closed

hamzashoukat94 mentioned this pull request Feb 15, 2022

Lucene & Memory Mapped Files apache/lucenenet#262

Open

ghost locked as resolved and limited conversation to collaborators Mar 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce the cost of creating a MemoryMappedFile#63790

Reduce the cost of creating a MemoryMappedFile#63790
adamsitnik merged 6 commits intodotnet:mainfrom
adamsitnik:moreMemoryMappedPerf

adamsitnik commented Jan 14, 2022

Uh oh!

ghost commented Jan 14, 2022

Uh oh!

Uh oh!

jkotas Jan 14, 2022

Uh oh!

stephentoub Jan 14, 2022 •

edited

Loading

Uh oh!

Uh oh!

stephentoub Jan 14, 2022

Uh oh!

stephentoub Jan 14, 2022

Uh oh!

adamsitnik Jan 14, 2022

Uh oh!

stephentoub Jan 14, 2022

Uh oh!

adamsitnik Jan 18, 2022

Uh oh!

adamsitnik Jan 18, 2022

Uh oh!

adamsitnik Jan 19, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	if (access == MemoryMappedFileAccess.Read && capacity > fileStream.Length)
	{
	throw new ArgumentException(SR.Argument_ReadAccessWithLargeCapacity);
	}

	// one can always create a small view if they do not want to map an entire file
	if (fileStream.Length > capacity)

Conversation

adamsitnik commented Jan 14, 2022

Uh oh!

ghost commented Jan 14, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub Jan 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

stephentoub Jan 14, 2022 •

edited

Loading