Update List of Assemblies to Crossgen by brianrob · Pull Request #9635 · dotnet/installer

brianrob · 2021-02-05T00:31:40Z

Context

The installer is responsible for precompiling the libraries that compose the final SDK. Over time, several new SDKs have been added, and several have been upgraded, which has resulted in a number of SDK assemblies that are not pre-compiled, and thus contribute to higher JIT times and overall wall-clock latency.

Changes Made

Updated paths to assemblies in crossgen.targets that contain assemblies that should be pre-compiled.

Testing

Verified that basic SDK scenarios work.
Captured before/after profiles to confirm wins.

Notes

This PR contains the following performance wins:

-548 drop in method jitting overall
210.1ms improvement in JIT time (26.1%)
7% improvement overall

I have filed #9634 to track work to catch this if/when it happens again.

cc: @stephentoub, @DamianEdwards, @marcpopMSFT

stephentoub

Nice!

brianrob · 2021-02-05T21:19:52Z

@pranavkm, this may improve some of the ASP.NET build scenarios you're looking at.

pranavkm · 2021-02-05T21:33:36Z

      <RemainingFiles Include="$(SdkOutputDirectory)Sdks\NuGet.Build.Tasks.Pack\CoreCLR\**\*" />
-      <RemainingFiles Include="$(SdkOutputDirectory)Sdks\Microsoft.NET.Sdk.Razor\tasks\netcoreapp*\*" />
-      <RemainingFiles Include="$(SdkOutputDirectory)Sdks\Microsoft.NET.Sdk.Razor\tools\netcoreapp*\**\*" />
+      <RemainingFiles Include="$(SdkOutputDirectory)Sdks\Microsoft.NET.Sdk.Razor\tasks\net6.0\**\*" />


Would it help if we put these in a netcoreapp directory? That way we wouldn't have to worry about updating these every major release. The TFM version isn't really useful to the SDK

/cc @captainsafia

Could be worthwhile from a maintenance perspective. I suspect that we're going to need to do something to catch new additions to the SDK (likely a test that looks for new non-crossgen'd assemblies), and so it may not be immediately necessary, but if this validation becomes costly or difficult, then renaming the directory could be a good way to ensure that things don't break going forward.

It's not incredibly difficult to fix up the SDK to produce fixed paths, but I think adding a check would be a better approach.

brianrob · 2021-02-08T20:19:47Z

@marcpopMSFT, can you recommend someone to review this please?

pranavkm · 2021-02-08T21:09:28Z

      <RemainingFiles Include="$(SdkOutputDirectory)Sdks\NuGet.Build.Tasks.Pack\CoreCLR\**\*" />
-      <RemainingFiles Include="$(SdkOutputDirectory)Sdks\Microsoft.NET.Sdk.Razor\tasks\netcoreapp*\*" />
-      <RemainingFiles Include="$(SdkOutputDirectory)Sdks\Microsoft.NET.Sdk.Razor\tools\netcoreapp*\**\*" />
+      <RemainingFiles Include="$(SdkOutputDirectory)Sdks\Microsoft.NET.Sdk.Razor\tasks\net6.0\**\*" />


It's not incredibly difficult to fix up the SDK to produce fixed paths, but I think adding a check would be a better approach.

pranavkm · 2021-02-08T21:09:41Z

-      <RemainingFiles Include="$(SdkOutputDirectory)Sdks\Microsoft.NET.Sdk.Publish\tools\netcoreapp*\**\*" />
+      <RemainingFiles Include="$(SdkOutputDirectory)Sdks\Microsoft.NET.ILLink.Tasks\tools\net5.0\**\*" />
+      <RemainingFiles Include="$(SdkOutputDirectory)Sdks\Microsoft.NET.Sdk.Publish\tools\net6.0\**\*" />
+      <RemainingFiles Include="$(SdkOutputDirectory)Sdks\Microsoft.NET.Sdk.Web\analyzers\cs\**\*" />


Analyzers run in VS. Is this correct?

That's a good question. If they run in VS, then probably not. I'll remove this one.

Do you think it would be useful to multi-target these assemblies to allow the files to be crossgened? They appear in every compilation for a webapp

Do you have a trace where these show up? It just depends on how much JIT time shows up.

It should show up as part of Csc, but I don't have one off hand.

I will take a look at the amount of JIT time that comes from these, and report back. Thanks for pointing this out.

joeloff · 2021-02-08T23:49:59Z

+      <RemainingFiles Include="$(SdkOutputDirectory)Sdks\Microsoft.NET.Sdk.Publish\tools\net6.0\**\*" />
+      <RemainingFiles Include="$(SdkOutputDirectory)Sdks\Microsoft.NET.Sdk.Web\tools\net6.0\**\*" />
+      <RemainingFiles Include="$(SdkOutputDirectory)Sdks\Microsoft.NET.Sdk.Web.ProjectSystem\tools\net6.0\**\*" />
+      <RemainingFiles Include="$(SdkOutputDirectory)Sdks\Microsoft.NET.Sdk.Worker\tools\net6.0\**\*" />


These are oddly specific version folders

The reason that the version is present is because many of these have a net472 version, and those should not get precompiled.

joeloff · 2021-02-08T23:53:49Z

How much does this bloat the installer in terms of number of files/size?

brianrob · 2021-02-09T01:07:24Z

@joeloff, this does not impact the number of files, it just precompiles files that are already included in the SDK. Here is a before and after comparison of size:

Zip File:

Before: 196MB
After: 211MB (+15MB / +7.7%)

Extracted:

Before: 545MB
After: 578MB (+33MB / +6.1%)

I would consider this a reasonable increase given the improvement that comes with it.

joeloff · 2021-02-09T01:39:24Z

@brianrob I thought there might be more files, but the increase seems very reasonable. We're doing triage on Wednesday, okay if we merge then or is this blocking anything?

brianrob · 2021-02-09T02:36:21Z

Not blocking. Wednesday is great. Thank you!

Update list of assemblies to be crossgen'd.

bf5d05c

stephentoub reviewed Feb 5, 2021

View reviewed changes

pranavkm reviewed Feb 5, 2021

View reviewed changes

pranavkm approved these changes Feb 8, 2021

View reviewed changes

Remove analyzers from crossgen list.

991ebff

marcpopMSFT requested a review from joeloff February 8, 2021 23:39

joeloff reviewed Feb 8, 2021

View reviewed changes

pranavkm mentioned this pull request Feb 10, 2021

Add support for source generators in Razor compiler/SDK dotnet/sdk#15756

Merged

joeloff approved these changes Feb 10, 2021

View reviewed changes

marcpopMSFT merged commit a5eebdb into dotnet:master Feb 10, 2021

brianrob deleted the fix-crossgen branch February 11, 2021 01:18

captainsafia mentioned this pull request Feb 22, 2021

Large disk size increase in Microsoft.NET.Sdk.Razor folder dotnet/aspnetcore#30379

Closed

mthalman mentioned this pull request Feb 23, 2021

Tracking: Large disk size increase in Microsoft.NET.Sdk.Razor folder dotnet/dotnet-docker#2616

Closed

Conversation

brianrob commented Feb 5, 2021

Context

Changes Made

Testing

Notes

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Uh oh!

brianrob commented Feb 5, 2021

Uh oh!

pranavkm Feb 5, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brianrob commented Feb 8, 2021

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joeloff commented Feb 8, 2021

Uh oh!

brianrob commented Feb 9, 2021

Uh oh!

joeloff commented Feb 9, 2021

Uh oh!

brianrob commented Feb 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pranavkm Feb 5, 2021 •

edited

Loading