Skip to content

Eliminate build process hop by loading MSBuild.dll into CLI process #16577

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

Merged
merged 22 commits into from
Apr 13, 2021

Conversation

ladipro
Copy link
Member

@ladipro ladipro commented Mar 29, 2021

Fixes #16362

This PR adds the possibility to load MSBuild.dll into the CLI process and directly call its entry point. This is in addition to the existing logic of launching a new dotnet process with MSBuild.dll as the startup assembly. The new behavior is enabled by default and can be disabled by setting a newly introduced DOTNET_CLI_RUN_MSBUILD_OUTOFPROC environment variable. The motivation for this change is performance as the extra process costs us 100 - 150 ms per MSBuild invocation, depending on platform and CPU specs.

Overall the change is straightforward but a few aspects deserve to be mentioned here:

  • MSBuild.dll is distributed in the Microsoft.Build.Runtime package which has non-standard layout and requires the assembly reference to be provided manually (see Microsoft.DotNet.Cli.Utils.csproj).
  • The .NET runtime currently has a feature gap and does not expose an API to set environment variables with empty values. This is worked around by falling back to executing MSBuild out-of-proc if such a variable is set. Hopefully this is not common.
  • The change is covered by existing tests. Some tests still take the old code path, simply because they are passing a non-default MSBuild.dll file path while end-to-end tests exercise in-proc MSBuild invocation. The difference between in-proc and out-of-proc is only in the way MSBuild is ultimately called. The underlying logic of argument passing, which most tests seem to be focused on, is the same so running tests twice (in-proc and out-of-proc) doesn't look warranted.

@ghost
Copy link

ghost commented Mar 29, 2021

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@danmoseley
Copy link
Member

Would it be better to just expose the method from MSBuild? It's clearly useful and that would avoid reflection on this fast path.

@ladipro
Copy link
Member Author

ladipro commented Mar 30, 2021

Would it be better to just expose the method from MSBuild? It's clearly useful and that would avoid reflection on this fast path.

Yes, I'm looking into packaging MSBuild.dll. Put it into either the existing Microsoft.Build.Runtime or a separate package. Still, even with reflection this should be about two orders of magnitude faster than launching a new process.

@danmoseley
Copy link
Member

I'm a little confused, how is packaging work necessary? You clearly have the dll present.

@ladipro ladipro force-pushed the ladipro/16362-inproc-msbuild branch from 0a4aec1 to 29f8e6b Compare March 31, 2021 14:14
@ladipro
Copy link
Member Author

ladipro commented Mar 31, 2021

The dll is present at run-time but the package it's coming from is built in a "special way" and not conducive to simply referencing it. The assembly is under contentFiles instead of lib, presumably for a good reason. I have attempted to work around it with 29f8e6b but it's apparently not complete yet.

@ladipro ladipro changed the title [WIP] Eliminate build process hop by loading MSBuild.dll into CLI process Eliminate build process hop by loading MSBuild.dll into CLI process Apr 5, 2021
@ladipro ladipro marked this pull request as ready for review April 5, 2021 19:34
@ladipro
Copy link
Member Author

ladipro commented Apr 5, 2021

This is now ready for review.

@marcpopMSFT marcpopMSFT requested a review from dsplaisted April 7, 2021 21:55
@ladipro
Copy link
Member Author

ladipro commented Apr 12, 2021

@dsplaisted @dotnet/dotnet-cli
I am ready to push the Merge button. I am not familiar with the process in this repo, however, so please let me know if it's fine to do so. Thank you!

@dsplaisted dsplaisted merged commit ee0e154 into dotnet:main Apr 13, 2021
@dsplaisted
Copy link
Member

I am ready to push the Merge button. I am not familiar with the process in this repo, however, so please let me know if it's fine to do so. Thank you!

FYI, generally when I approve a PR I'm OK with folks going ahead and merging it if they haven't made major changes unrelated to the PR feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Eliminate process hop in dotnet build by hosting MSBuild.dll in the CLI process
3 participants