HostWriter Performance Improvements #48774

brianrob · 2021-02-25T19:46:33Z

Use CopyOnWrite MemoryMappedFiles to avoid copying the template host.
Avoid a file open/read when attempting to remove a MachO signature when we know the input host is a PE file.

…'t attempt to remove the MachO signature for known PE files.

ghost · 2021-02-25T19:46:39Z

Tagging subscribers to this area: @vitek-karas, @agocke
See info in area-owners.md if you want to be subscribed.

Issue Details

Use CopyOnWrite MemoryMappedFiles to avoid copying the template host.
Avoid a file open/read when attempting to remove a MachO signature when we know the input host is a PE file.

Author:	brianrob
Assignees:	-
Labels:	`area-HostModel`
Milestone:	-

brianrob · 2021-02-25T19:50:21Z

@vitek-karas, @agocke I've marked this as a draft PR so that I can get some additional coverage. At the same time, I'm interested in your thoughts. I'm looking to improve this since it shows up as an area of opportunity in the dev inner-loop performance effort, and the file copy has a relatively heavy impact on Windows when antivirus is involved. I still need to put together some comparison numbers to see if this has a material performance benefit.

brianrob · 2021-02-25T21:57:16Z

On my local machine, I see a net improvement of 66ms (59.45%) in disk and antivirus time for the host exe during _CreateAppHost. This is on a console hello-world build.

Baseline:

Total disk/AV time for the host exe: 111ms (5 scan requests)

With this PR:

Total disk/AV time for the host exe: 45ms (2 scan requests)

brianrob · 2021-02-25T21:57:31Z

cc: @DamianEdwards, @stephentoub

…le permission. This is OK, because the installed version of the apphost has -rwxr-xr-x permissions.

brianrob · 2021-02-26T00:30:17Z

For additional test, I verified with the latest bits that the host, after transform is identical to one without my change on Windows and Linux. It appears that on OSX, we don't emit a host. Is this new? If so, we can probably trim out the MachO signature removal, unless this is just temporary.

danmoseley · 2021-02-26T23:03:22Z

src/installer/managed/Microsoft.NET.HostModel/AppHost/HostWriter.cs

+                    }
+                    finally
+                    {
+                        if (memoryMappedViewAccessor != null)


can either of these actually be null here?

Yes. As an example, if MemoryMappedFile.CreateFromFile throws, then memoryMappedViewAccessor would be null.

danmoseley · 2021-02-26T23:05:29Z

src/installer/managed/Microsoft.NET.HostModel/AppHost/BinaryUtils.cs

+            int pos = 0;
+            int bufSize = 16384; //16K
+
+            byte[] buf = new byte[bufSize];


is ArrayPool available?

Yes, though I purposely did not use it here. This code is invoked from MSBuild once per-process, and the only existing use of ArrayPool that I've been able to find is from RegularExpressions, which allocations char[]. My goal here is to avoid incurring the cost of initializing an ArrayPool<byte> when we only need one byte[] in this code for the life of the process.

brianrob · 2021-03-01T18:04:04Z

FYI @ericstj

ericstj · 2021-03-01T19:16:17Z

I believe this will fix #3832

ericstj · 2021-03-01T19:20:22Z

src/installer/managed/Microsoft.NET.HostModel/AppHost/HostWriter.cs

+                RetryUtil.RetryOnWin32Error(UpdateResources);
+
+                RetryUtil.RetryOnIOError(RemoveSignatureIfMachO);


Won't both of these reopen the file? Can they be rewritten to use the memory mapped file?

Yes, both of these will re-open the file. UpdateResources uses win32 APIs to enumerate and add the resources. There might be a way to do this against the existing open MemoryMappedFile, but likely the cost of doing so is going to be higher than we want to pay, if it's possible. It would require that we re-write the functionality.

RemoveSignatureIfMachO is potentially possible, but it became much harder to implement because the size of the file has to change. That said, I did change the code so that this doesn't get called unconditionally for PE files.

We may eventually want to rewrite UpdateResources so it can be supported xplat, but the work is not planned right now.

There's a trick where you can keep an open handle read/write with sharing and it will prevent some other process from getting exclusive access. So even if you didn't re-implement these over the file mapping you might be able to keep a handle open to prevent interference. This works so long as the Windows APIs you're calling specify read/write sharing. If the Windows APIs themselves don't allow sharing your held handle prevents them from opening the file.
You can procmon the Windows API to see how it opens the file.

I did some experiments with this, but it doesn't look like this is possible. The file is loaded via LoadLibraryEx, which does not appear to allow for sharing when an existing handle has write permissions.

I see, I guess when UpdateResource is rewritten that would fix this. Presumably some code can be copied from CSC since it knows how to do that.

For RemoveSignatureIfMachO just make your new BinaryUtils.SaveFile take a stream and open the stream above. Then change RemoveSignatureIfMachO to take a stream and give it the written file stream (assuming reordering of these ops is safe). This is minimal change and saves one roll-of-the-dice around retries.

Good call. At this point it is for sure safe because resources can only be copied into a PEFile, and the signature can only be removed from MachO binaries. So the operations cannot be run on the same binary.

Another thought on UpdateResource: right now this needs to be incremental on the binary output of CSC, which means the host needs to be rewritten whenever CSC recompiles. I wonder if instead you could use the input to CSC here (eg: -win32res parameter). Potentially eliminating this task from most normal inner-loop scenarios.

That's an interesting point. I have filed dotnet/sdk#16173 to track this.

agocke

LGTM aside from a few comments. Thanks for the work!

agocke · 2021-03-01T21:57:04Z

src/installer/managed/Microsoft.NET.HostModel/AppHost/HostWriter.cs

+                RetryUtil.RetryOnWin32Error(UpdateResources);
+
+                RetryUtil.RetryOnIOError(RemoveSignatureIfMachO);


We may eventually want to rewrite UpdateResources so it can be supported xplat, but the work is not planned right now.

agocke · 2021-03-02T23:35:11Z

src/installer/managed/Microsoft.NET.HostModel/AppHost/HostWriter.cs

+                        if (memoryMappedViewAccessor != null)
+                        {
+                            memoryMappedViewAccessor.Dispose();
+                        }
+                        if (memoryMappedFile != null)
+                        {
+                            memoryMappedFile.Dispose();
+                        }


Suggested change

if (memoryMappedViewAccessor != null)

{

memoryMappedViewAccessor.Dispose();

}

if (memoryMappedFile != null)

{

memoryMappedFile.Dispose();

}

memoryMappedViewAccessor?.Dispose();

memoryMappedFile?.Dispose();

agocke · 2021-03-02T23:37:47Z

src/installer/managed/Microsoft.NET.HostModel/AppHost/HostWriter.cs

+
+                RetryUtil.RetryOnIOError(RemoveSignatureIfMachO);
+
+                RetryUtil.RetryOnIOError(SetLastWriteTime);


Is this necessary if we're calling SaveFile? It looks like the previous concern was that simple memory map modification didn't update the last write time, but I'm unclear if that's also true of SaveFile.

Good point. It looks like the last write time is updated when we open and write the stream, so I've removed this call.

@brianrob Any comment on this?

Never mind, the diff here just didn't reparent after your changes.

brianrob · 2021-03-03T01:46:09Z

I think I've hit all the feedback. @ericstj let me know if you have anything else based on the latest iteration.

ericstj · 2021-03-03T02:17:05Z

src/installer/managed/Microsoft.NET.HostModel/AppHost/HostWriter.cs

+                RetryUtil.RetryOnWin32Error(UpdateResources);
+
+                RetryUtil.RetryOnIOError(RemoveSignatureIfMachO);


Another thought on UpdateResource: right now this needs to be incremental on the binary output of CSC, which means the host needs to be rewritten whenever CSC recompiles. I wonder if instead you could use the input to CSC here (eg: -win32res parameter). Potentially eliminating this task from most normal inner-loop scenarios.

Use CopyOnWrite MemoryMappedFiles to avoid a host copy operation. Don…

1e539a1

…'t attempt to remove the MachO signature for known PE files.

dotnet-issue-labeler bot added the area-HostModel Microsoft.NET.HostModel issues label Feb 25, 2021

Fix test. CopyOnWrite MemoryMappedFiles requires at least one writeab…

b290fd6

…le permission. This is OK, because the installed version of the apphost has -rwxr-xr-x permissions.

brianrob marked this pull request as ready for review February 26, 2021 00:30

danmoseley reviewed Feb 26, 2021

View reviewed changes

Base automatically changed from master to main March 1, 2021 09:08

ericstj reviewed Mar 1, 2021

View reviewed changes

agocke approved these changes Mar 2, 2021

View reviewed changes

brianrob added 2 commits March 2, 2021 17:34

Merge remote-tracking branch 'upstream/main' into hostwriter-perf

fe717db

Code review feedback.

825932c

brianrob force-pushed the hostwriter-perf branch from 7077c09 to 825932c Compare March 3, 2021 01:44

ericstj approved these changes Mar 3, 2021

View reviewed changes

brianrob mentioned this pull request Mar 3, 2021

Avoid Invoking _CreateAppHost Task When Not Necessary dotnet/sdk#16173

Open

brianrob merged commit 872d9e4 into dotnet:main Mar 3, 2021

brianrob deleted the hostwriter-perf branch March 3, 2021 15:06

This was referenced Mar 8, 2021

UnauthorizedAccessException for apphost.exe when trying to use latest nightly dotnet build #49332

Closed

Fix access mode of memory-mapped apphost #49341

Merged

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

karelz added this to the 6.0.0 milestone May 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HostWriter Performance Improvements #48774

HostWriter Performance Improvements #48774

brianrob commented Feb 25, 2021

ghost commented Feb 25, 2021

brianrob commented Feb 25, 2021

brianrob commented Feb 25, 2021

brianrob commented Feb 25, 2021

brianrob commented Feb 26, 2021

danmoseley Feb 26, 2021

brianrob Feb 26, 2021

danmoseley Feb 26, 2021

danmoseley Feb 26, 2021

brianrob Feb 26, 2021

brianrob commented Mar 1, 2021

ericstj commented Mar 1, 2021

ericstj Mar 1, 2021

brianrob Mar 1, 2021 •

edited

Loading

agocke Mar 1, 2021

ericstj Mar 2, 2021 •

edited

Loading

brianrob Mar 2, 2021

ericstj Mar 3, 2021 •

edited

Loading

brianrob Mar 3, 2021

ericstj Mar 3, 2021

brianrob Mar 3, 2021

agocke left a comment

agocke Mar 1, 2021

agocke Mar 2, 2021

brianrob Mar 3, 2021

agocke Mar 2, 2021

brianrob Mar 3, 2021

agocke Mar 3, 2021

agocke Mar 3, 2021

brianrob commented Mar 3, 2021

ericstj Mar 3, 2021

		RetryUtil.RetryOnWin32Error(UpdateResources);

		RetryUtil.RetryOnIOError(RemoveSignatureIfMachO);


		RetryUtil.RetryOnIOError(RemoveSignatureIfMachO);

		RetryUtil.RetryOnIOError(SetLastWriteTime);

HostWriter Performance Improvements #48774

HostWriter Performance Improvements #48774

Conversation

brianrob commented Feb 25, 2021

ghost commented Feb 25, 2021

brianrob commented Feb 25, 2021

brianrob commented Feb 25, 2021

brianrob commented Feb 25, 2021

brianrob commented Feb 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianrob commented Mar 1, 2021

ericstj commented Mar 1, 2021

Choose a reason for hiding this comment

brianrob Mar 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericstj Mar 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericstj Mar 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agocke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianrob commented Mar 3, 2021

Choose a reason for hiding this comment

brianrob Mar 1, 2021 •

edited

Loading

ericstj Mar 2, 2021 •

edited

Loading

ericstj Mar 3, 2021 •

edited

Loading