Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Performance]: MSBuild.exe spends 300 ms JITting Microsoft.DotNet.MSBuildSdkResolver.dll #9303

Closed
2 tasks
ladipro opened this issue Oct 3, 2023 · 2 comments · Fixed by #9439
Closed
2 tasks

Comments

@ladipro
Copy link
Member

ladipro commented Oct 3, 2023

Issue Description

We currently load SDK resolvers using Assembly.LoadFrom on .NET Framework, which disqualifies it from using native images even if they existed. We should:

  • Figure out a way to use regular Assembly.Load, e.g. load into a separate appdomain with a generated app config.
  • Make sure the .NET SDK resolver and the NuGet SDK resolver assemblies are added to the NGEN queue by Visual Studio installer and produce usable images.

The potential perf gain is about 300 ms of JITting saved, out of 1100 ms total for a simple app build using MSBuild.exe. It should also lay some groundwork for eliminating another big chunk of JITting: Microsoft.NET.Build.Tasks.dll.

Steps to Reproduce

Collect JIT stats of:

> MSBuild.exe SimpleProject.csproj

Data

image

Analysis

No response

Versions & Configurations

No response

Regression

  • yes
  • no

Regression Details

No response

@ladipro ladipro added performance needs-triage Have yet to determine what bucket this goes in. size:5 Iteration:2023October labels Oct 3, 2023
@ladipro ladipro self-assigned this Oct 3, 2023
@rainersigwald
Copy link
Member

  • Make sure the .NET SDK resolver and the NuGet SDK resolver assemblies are added to the NGEN queue by Visual Studio installer and produce usable images.

This one will surely be Interesting™️. We should talk to @joeloff.

@ladipro
Copy link
Member Author

ladipro commented Oct 3, 2023

My understanding is that the resolvers are shipped with VS, located under MSBuild\Current\Bin\SdkResolvers so they have to already be playing well with the VS installer. Binaries shipped in the SDK (e.g. Microsoft.NET.Build.Tasks.dll) are a different story, indeed.

@AR-May AR-May removed the needs-triage Have yet to determine what bucket this goes in. label Oct 5, 2023
ladipro added a commit to dotnet/installer that referenced this issue Nov 9, 2023
…7732)

MSBuild.exe currently spends a significant amount of time JITting `Microsoft.DotNet.MSBuildSdkResolver` and its dependencies, see dotnet/msbuild#9303 for details.

This PR makes Visual Studio installer add these assemblies to the NGEN queue, which is a necessary condition for eliminating JITting. Just like `Microsoft.Build.*` assemblies, we need to NGEN these with two configurations: vsn.exe so it works in the devenv process, and MSBuild.exe so it works in MSBuild satellite processes.
ladipro pushed a commit to dotnet/installer that referenced this issue Nov 14, 2023
…s dependencies (#17750)

Backport of #17732 to release/8.0.2xx

MSBuild.exe currently spends a significant amount of time JITting `Microsoft.DotNet.MSBuildSdkResolver` and its dependencies, see dotnet/msbuild#9303 for details.

This PR makes Visual Studio installer add these assemblies to the NGEN queue, which is a necessary condition for eliminating JITting. Just like `Microsoft.Build.*` assemblies, we need to NGEN these with two configurations: vsn.exe so it works in the devenv process, and MSBuild.exe so it works in MSBuild satellite processes.
ladipro added a commit to dotnet/sdk that referenced this issue Nov 20, 2023
MSBuild.exe currently spends a significant amount of time JITting `Microsoft.DotNet.MSBuildSdkResolver` and its dependencies, see dotnet/msbuild#9303 for details.

The most straightforward way to fix this is to load the assembly into the default load context using `Assembly.Load` rather than the load-from context where native images are not used. For this to work without creating a secondary AppDomain with a custom app.config, MSBuild needs to know the version of the resolver assembly statically - at its own build time. Since MSBuild and SDK insert into Visual Studio independently, we have basically two options:

- Be OK with a temporary perf regression every time SDK inserts vNext into VS (or trying to make a combined SDK + MSBuild insertion with MSBuild compensating for the version bump). Note: The assembly load can be made such that it falls back to `LoadFrom` if the assembly identity doesn't match.
- Freeze the version of the assembly. The exact version is not important as long as it's fixed. This should be safe because the assembly should have no other consumers outside of MSBuild.

There are other approaches but I consider them non-practical / unattractive:

- Redesign the code flow so MSBuild always knows the version of the SDK it's going to run with in VS.
- Update `MSBuild.exe.config` as a post-install step on the customer machine.
ladipro added a commit to dotnet/sdk that referenced this issue Nov 20, 2023
… version (#37034)

MSBuild.exe currently spends a significant amount of time JITting `Microsoft.DotNet.MSBuildSdkResolver` and its dependencies, see dotnet/msbuild#9303 for details.

The most straightforward way to fix this is to load the assembly into the default load context using `Assembly.Load` rather than the load-from context where native images are not used. For this to work without creating a secondary AppDomain with a custom app.config, MSBuild needs to know the version of the resolver assembly statically - at its own build time. Since MSBuild and SDK insert into Visual Studio independently, we have basically two options:

- Be OK with a temporary perf regression every time SDK inserts vNext into VS (or trying to make a combined SDK + MSBuild insertion with MSBuild compensating for the version bump). Note: The assembly load can be made such that it falls back to `LoadFrom` if the assembly identity doesn't match.
- Freeze the version of the assembly. The exact version is not important as long as it's fixed. This should be safe because the assembly should have no other consumers outside of MSBuild.

There are other approaches but I consider them non-practical / unattractive:

- Redesign the code flow so MSBuild always knows the version of the SDK it's going to run with in VS.
- Update `MSBuild.exe.config` as a post-install step on the customer machine.
@AR-May AR-May closed this as completed in 6257b8e Dec 15, 2023
ladipro added a commit that referenced this issue Dec 19, 2023
)

Related to #9303

### Context

We load `NuGet.Frameworks` lazily by path when the project being evaluated calls one of a few property functions that need it. All standard .NET projects use such functionality so this load is executed pretty much always. Since we're loading the assembly by path, the loader is unable to take advantage of the native image and we spend 50-100 ms JITting.

### Changes Made

Improved MSBuild.exe startup (or, more precisely, first evaluation) by 50-100 ms by loading `NuGet.Frameworks` into a separate AppDomain using `Assembly.Load`. The AppDomain is set up with the right binding redirects for the loader to load the assembly by strong name and use its native image.

### Testing

Experimental insertions.

### Notes

There are several other assemblies that require non-trivial JITting on startup. The reason why we're tackling `NuGet.Frameworks` here are:
- The assembly has no dependencies other than Framework assemblies, so the AD setup is relatively simple.
- The functionality provided by this assembly has simple signatures so the cross-domain call cost is negligible.
- A compatible native image for this assembly already exists.

Other notes:
- There is no impact on VS or any other MSBuild host because creating ADs may be risky and disruptive. We're changing only MSBuild.exe.
- This PR is about the `NuGet.Frameworks` copy that lives in `{VS-installation}\Common7\IDE\CommonExtensions\Microsoft\NuGet`. The MSBuild process may also load SDK-specific copies from paths like `C:\Program Files\dotnet\sdk\8.0.100\Sdks\Microsoft.NET.Sdk\tools\net472` which are out of scope here.

This is what the AD looks like in SOS:

```
Domain 2:           000001dff97c7e70
Stage:              OPEN
Name:               NuGetFrameworkWrapper
Assembly:           000001dff6bac490 [C:\windows\Microsoft.Net\assembly\GAC_64\mscorlib\v4.0_4.0.0.0__b77a5c561934e089\mscorlib.dll]
Assembly:           000001dff9732960 [C:\src\msbuild\artifacts\bin\bootstrap\net472\MSBuild\Current\Bin\Microsoft.Build.dll]
Assembly:           000001dff97e2410 [C:\src\msbuild\artifacts\bin\bootstrap\net472\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.Frameworks.dll]
Assembly:           000001dff8de7250 [C:\windows\Microsoft.Net\assembly\GAC_MSIL\System.Core\v4.0_4.0.0.0__b77a5c561934e089\System.Core.dll]
Assembly:           000001dff8dd6100 [C:\windows\Microsoft.Net\assembly\GAC_MSIL\System\v4.0_4.0.0.0__b77a5c561934e089\System.dll]
Assembly:           000001dff8e1d9a0 [C:\windows\Microsoft.Net\assembly\GAC_MSIL\System.Xml\v4.0_4.0.0.0__b77a5c561934e089\System.Xml.dll]
```
@AR-May AR-May added the triaged label Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants