Reuse CSC arguments in file-based app runs #50635
Conversation
| internal static bool ArgumentContainsWhitespace(string argument) => | ||
| argument.Contains(" ") || argument.Contains("\t") || argument.Contains("\n"); | ||
|
|
||
| // Taken from Roslyn's CommandLineParser. |
There was a problem hiding this comment.
nit: it would be good to provide a permalink so that if things change over time we can compare.
| // Finds /out: argument and extract the file path from it. | ||
| string? FindOutputFile() | ||
| { | ||
| const string outPrefix = "/out:"; |
There was a problem hiding this comment.
Just curious, did you consider using CSharpCommandLineParser.Parse for this? Probably that allocates quite a bit more.
There was a problem hiding this comment.
Hm, you're right it's doing unnecessary work but I won't need to depend on internal compiler command line parsing details by using it, hence I like it!
| { | ||
| rspPath = Path.Join(ArtifactsPath, "csc.rsp"); | ||
|
|
||
| if (!CscArguments.IsDefaultOrEmpty) |
There was a problem hiding this comment.
any risk of an unexpected behavior here if this changes from empty to non-empty across different runs? e.g. from a stale .rsp file being present in the directory.
There was a problem hiding this comment.
Good catch, I think there is indeed a problem when the "reusing CSC args" vs "built-in CSC args" optimizations are used after each other. I will add a test and fix that.
Other than that, I think there should be no problems. The "reusing CSC args" optimization always regenerates the rsp file. The "build-in CSC args" optimization should regenerate the rsp file whenever it's out of date.
| // See https://github.com/dotnet/msbuild/blob/main/documentation/specs/build-nonexistent-projects-by-default.md. | ||
| { "_BuildNonexistentProjectsByDefault", bool.TrueString }, | ||
| { "RestoreUseSkipNonexistentTargets", bool.FalseString }, | ||
| { "ProvideCommandLineArgs", bool.TrueString }, |
There was a problem hiding this comment.
nit: looks like capacity is out of date now, should the capacity just be deleted?
| return null; | ||
| } | ||
|
|
||
| void GetCscArguments(CacheInfo cache, BuildResult result, ProjectInstance projectInstance) |
There was a problem hiding this comment.
It feels like a method should not be named Get... if it returns void. Maybe CacheCscArguments or TryCacheCscArguments.
|
|
||
| public bool DetermineFinalCanReuseAuxiliaryFiles() | ||
| { | ||
| if (PreviousEntry?.CscArguments.IsDefaultOrEmpty == false) |
There was a problem hiding this comment.
Kind of an aside, but, is it valid to say that we can reuse something from the previous run, if we don't even have a cache entry from the previous run? Possibly I am misunderstanding something?
Basically I am wondering if things can be simplified slightly by just saying if (PreviousEntry is null) return false;.
There was a problem hiding this comment.
If PreviousEntry is null, we will actually return false on line 512 but also write a verbose message saying why ("CSC auxiliary files can NOT be reused because previous build level was not CSC (it was N/A)."). In case we have CSC arguments from previous run (PreviousEntry?.CscArguments.IsDefaultOrEmpty == false), I intentionally didn't want to write any message because that would be superfluous ("csc auxiliary file reuse" is irrelevant in that code path).
| Reporter.Verbose.WriteLine("We have CSC arguments but not build result file. That's unexpected."); | ||
| } | ||
| else if (cache.PreviousEntry.Directives.Length != cache.CurrentEntry.Directives.Length || | ||
| !cache.PreviousEntry.Directives.SequenceEqual(cache.CurrentEntry.Directives)) |
There was a problem hiding this comment.
nit: SequenceEqual already returns false when lengths are not equal. No need to test it in advance.
There was a problem hiding this comment.
IEnumerable one wouldn't do that, but you're right this one does since it's on ImmutableArray args, thanks
| // Don't reuse CSC arguments, this is the "simple" CSC-only build. | ||
| if (cache.PreviousEntry?.CscArguments.IsDefaultOrEmpty == false) | ||
| { | ||
| cache.PreviousEntry.CscArguments = []; |
There was a problem hiding this comment.
It feels reasonable to me to always assign these in this code path. I am unsure if, for example, we want to avoid doing these assignments when CscArguments.IsDefault.
There was a problem hiding this comment.
You're right, I'm not sure why I put the condition there.
| } | ||
|
|
||
| /// <summary> | ||
| /// Up-to-date checks and optimizations currently don't support other included files. |
There was a problem hiding this comment.
Should we block the optimizations, implicitly/by default, when such files are present? Or is there follow-up coming to address this?
There was a problem hiding this comment.
Well, detecting that such files are present might be slow, hence checking for them would slow down the optimized paths. (We are also fine with the dotnet run file.cs optimizations being only "best effort" in general, although included files might be too important to ignore.) Anyway, I'm planning a follow up to investigate the options here (it is kind of unrelated to this PR though, the pre-existing optimizations already ignore included files).
| } | ||
|
|
||
| private void Build(TestDirectory testInstance, BuildLevel level, ReadOnlySpan<string> args = default, string expectedOutput = "Hello from Program") | ||
| private void Build(TestDirectory testInstance, BuildLevel level, ReadOnlySpan<string> args = default, string expectedOutput = "Hello from Program", string programFileName = "Program.cs") |
There was a problem hiding this comment.
I feel parameter level should be renamed to expectedLevel, to make it more clear this parameter is not instructing the build process to behave a certain way, we are verifying that it made the choice we expected it to make.
It might be helpful for clarity of the tests, to split this between Build and Verify helpers like many of the compiler tests do. e.g. Build(testInstance, args, programFileName).Verify(expectedLevel, expectedOutput). You may wish to consider making such a change at your discretion.
There was a problem hiding this comment.
Hm, a Build/Verify split would improve readability but I also often see in the roslyn codebase people unintentionally forgetting to call VerifyDiagnostics after calling CreateCompilation, hence I'm unsure if it's worth it.
|
@333fred for a review, thanks |
4 participants
Part of #48011.
This optimization saves CSC arguments when MSBuild runs and then reuses them in subsequent runs so
dotnet run file.cscan skip MSBuild and invoke only the compiler.In my simple experiments, previously the re-runs (using MSBuild) would take ~4 seconds, now (using CSC) they take ~1 second.