Avoid unnecessary evaluations during dotnet run file.cs
#49991
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes the dotnet run file.cs command by avoiding unnecessary MSBuild evaluations when run properties can be cached or reused from previous builds. The changes focus on caching run properties during builds and reusing them to skip expensive project evaluations.
Key changes include:
- Enhanced caching mechanism to store and reuse run properties from previous builds
- Modified run command logic to use cached properties when available instead of re-evaluating projects
- Updated test expectations to reflect the reduced number of MSBuild evaluations
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
test/dotnet.Tests/CommandTests/Run/RunFileTests.cs |
Added tests for ComputeRunArguments target and updated binary log expectations to reflect fewer MSBuild invocations |
src/Cli/dotnet/Commands/Test/TestApplication.cs |
Updated property names from Run* to simpler names in RunProperties |
src/Cli/dotnet/Commands/Test/SolutionAndProjectUtility.cs |
Updated RunProperties property references and method calls |
src/Cli/dotnet/Commands/Test/MSBuildHandler.cs |
Updated logging to use new RunProperties property names |
src/Cli/dotnet/Commands/Run/VirtualProjectBuildingCommand.cs |
Enhanced caching logic to store run properties and avoid re-evaluation |
src/Cli/dotnet/Commands/Run/RunProperties.cs |
Expanded RunProperties to include additional metadata and restructured creation methods |
src/Cli/dotnet/Commands/Run/RunCommand.cs |
Modified to use cached run properties when available instead of always evaluating projects |
src/Cli/dotnet/Commands/Run/CSharpCompilerCommand.cs |
Removed redundant verbose logging that's now handled elsewhere |
src/Cli/dotnet/Commands/Run/Api/RunApiCommand.cs |
Updated method call to match new signature |
src/Cli/dotnet/Commands/Restore/RestoringCommand.cs |
Updated method name to use plural form |
src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildArgs.cs |
Generalized method to accept multiple targets instead of single target |
Comments suppressed due to low confidence (1)
src/Cli/dotnet/Commands/Run/VirtualProjectBuildingCommand.cs:205
- This debug assertion was removed but the logic it was protecting against may still be relevant. If the combination of NoRestore and NoBuild is invalid, this should be validated elsewhere or the removal should be explained.
var verbosity = MSBuildArgs.Verbosity ?? VerbosityOptions.quiet;
| if (applicationArgs.Length != 0) | ||
| { | ||
| runArguments += " " + ArgumentEscaper.EscapeAndConcatenateArgArrayForProcessStart(applicationArgs); | ||
| return this with { Arguments = Arguments + " " + ArgumentEscaper.EscapeAndConcatenateArgArrayForProcessStart(applicationArgs) }; |
There was a problem hiding this comment.
... That's interesting syntax. Can you use with on anything or just this?
There was a problem hiding this comment.
Any record type AFAIK: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/operators/with-expression
| } | ||
|
|
||
| MarkBuildStart(); | ||
| } | ||
| else | ||
| else // if (NoBuild) |
There was a problem hiding this comment.
That's not commented code, that's my attempt at making it clearer what "state" the else branch is in since it's a bit far away from the if (!NoBuild) above.
There was a problem hiding this comment.
It may be easier to follow if the condition were inverted since the 'else' would be closer to the 'if' in that case. However, there isn't a need to make a change here.
| FacadeLogger? logger = LoggerUtility.DetermineBinlogger([..MSBuildArgs.OtherMSBuildArgs], "dotnet-run"); | ||
| if (cachedRunProperties != null) | ||
| { | ||
| // We can also skip project evaluation if we already evaluated the project during virtual build |
There was a problem hiding this comment.
"virtual build" means when csc is invoked directly?
There was a problem hiding this comment.
No, it's referring to the VirtualProjectBuildingCommand invoking MSBuild on a "virtual" project. Then we get cachedRunProperties from there.
| @@ -969,7 +1017,7 @@ public void BinaryLog_ArgumentForms(string arg) | |||
| new DirectoryInfo(testInstance.Path) | |||
| .EnumerateFiles("*.binlog", SearchOption.TopDirectoryOnly) | |||
| .Select(f => f.Name) | |||
| .Should().BeEquivalentTo([$"{fileName}.binlog", $"{fileName}-dotnet-run.binlog"]); | |||
| .Should().BeEquivalentTo([$"{fileName}.binlog"]); | |||
There was a problem hiding this comment.
Is the second binlog just something we never need any more?
There was a problem hiding this comment.
Correct (for file-based apps only, project-based apps are unchanged). That dotnet-run.binlog was used when building the "ComputeRunArguments" target separately, but it's all in one binlog now.
| } | ||
|
|
||
| MarkBuildStart(); | ||
| } | ||
| else | ||
| else // if (NoBuild) |
There was a problem hiding this comment.
It may be easier to follow if the condition were inverted since the 'else' would be closer to the 'if' in that case. However, there isn't a need to make a change here.
Part of #48011.