Skip to content

Conversation

@ladipro
Copy link
Member

@ladipro ladipro commented Sep 3, 2024

Context

Now that dotnet/sdk#39573 has made it into VS, we can finally enable loading Microsoft.DotNet.MSBuildSdkResolver by assembly name, as opposed to file path, which makes it possible to use its NGEN image.

Changes Made

Re-did #9439 which had to be reverted because of the problematic dependency on Newtonsoft.Json. The .NET SDK resolver does not depend on Newtonsoft.Json anymore. All the other pieces are already in place:

  • Microsoft.DotNet.MSBuildSdkResolver is NGENed, both for MSBuild.exe and for devenv.exe.
  • devenv.exe.config contains binding redirects analogous to what's added in this PR.

Testing

Experimentally inserted into VS and verified that Microsoft.DotNet.MSBuildSdkResolver is no longer JITted, saving ~50-100 ms of MSBuild.exe startup time.

@ladipro ladipro changed the title Reapply "Load Microsoft.DotNet.MSBuildSdkResolver into default load context" () Reapply "Load Microsoft.DotNet.MSBuildSdkResolver into default load context" Sep 3, 2024
@JanKrivanek
Copy link
Member

Can you please link the mentioned exp VS insertion?

@ladipro
Copy link
Member Author

ladipro commented Sep 3, 2024

Can you please link the mentioned exp VS insertion?

https://devdiv.visualstudio.com/DevDiv/_git/VS/pullrequest/575724 though it's odd that it hasn't run Perf DDRITs. I just requested Perf DDRITs one more time. If it doesn't work I'll create a new insertion.

Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

I have minore comment for improving maintainability

@ladipro ladipro force-pushed the exp/redo-resolver-ngen3 branch from b70e50e to ea7ebd9 Compare September 4, 2024 07:18
@ladipro
Copy link
Member Author

ladipro commented Sep 4, 2024

I have addressed the comment and made another exp insertion, this time with Perf DDRITs results. It seems to be clean module some minor unrelated noise.
https://devdiv.visualstudio.com/DevDiv/_git/VS/pullrequest/575802

@ladipro ladipro enabled auto-merge (squash) September 4, 2024 07:28
Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me but I was surprised to not see an improvement in the MethodsJitted counters in the insertion. Are we sure it's working?

@ladipro ladipro merged commit 1b5033e into main Sep 11, 2024
@ladipro ladipro deleted the exp/redo-resolver-ngen3 branch September 11, 2024 09:59
@ladipro
Copy link
Member Author

ladipro commented Sep 11, 2024

This looks good to me but I was surprised to not see an improvement in the MethodsJitted counters in the insertion. Are we sure it's working?

I have high confidence that JITting of Microsoft.DotNet.MSBuildSdkResolver is eliminated in MSBuild.exe invoked from the command line. Now for PerfDDRITs, the CLR_AdjustedMethodsJitted_Total_NonDevenv counter is not showing any JITting in the resolver, neither baseline nor target build. This may be because the scenarios that measure this counter don't really cover startup / first evaluation. Or there's something else at play. My memory is faint, but isn't SDK resolution (in VS only?) optimized by forwarding requests from worker nodes back to the central node?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants