Fix generator caching in compiler server #57177
Conversation
|
@dotnet/roslyn-compiler for review please |
src/Compilers/Core/Portable/InternalUtilities/AdditionalTextComparer.cs
Outdated
Show resolved
Hide resolved
src/Compilers/Core/Portable/InternalUtilities/AdditionalTextComparer.cs
Outdated
Show resolved
Hide resolved
src/Compilers/Core/Portable/InternalUtilities/AdditionalTextComparer.cs
Outdated
Show resolved
Hide resolved
|
@dotnet/roslyn-compiler for a second review please :) |
chsienki
force-pushed
the
source-generators/fix-generator-cache
branch
from
23f8eae to
a14ef88
Compare
|
Rebased onto latest main |
| @@ -1448,14 +1448,18 @@ private bool WriteTouchedFiles(DiagnosticBag diagnostics, TouchedFileLogger? tou | |||
|
|
|||
| protected virtual ImmutableArray<AdditionalTextFile> ResolveAdditionalFilesFromArguments(List<DiagnosticInfo> diagnostics, CommonMessageProvider messageProvider, TouchedFileLogger? touchedFilesLogger) | |||
| { | |||
| var builder = ImmutableArray.CreateBuilder<AdditionalTextFile>(); | |||
| var builder = ArrayBuilder<AdditionalTextFile>.GetInstance(); | |||
| var filePaths = new HashSet<string>(StringComparer.OrdinalIgnoreCase); | |||
There was a problem hiding this comment.
Assuming this is the fix for the correctness issue you were debugging. Can you elaborate a bit on this? Does this mean we were seeing the same additional file multiple times?
There was a problem hiding this comment.
@jaredpar Yeah, specifically the PublicAPI.[Un]Shipped.txt files. They are bought in via our build and also added via the analyzer package. We were failing in our assertion that we succeed when adding to the set here
This is obviously a correctness issue: if we don't dedupe then a generator could end up generating the same tree twice for a given additional file.
There was a problem hiding this comment.
Where in the command line do we filter out the duplicate files?
There was a problem hiding this comment.
I was about to say you're commenting on it, then I looked at it and also wondered how it was working. Turns out I had a test bug too :(
Now fixed. We expand the paths using the same logic added in #56588 and then compare if they are the same.
| @@ -19,17 +19,19 @@ internal sealed class InputNode<T> : IIncrementalGeneratorNode<T> | |||
| { | |||
| private readonly Func<DriverStateTable.Builder, ImmutableArray<T>> _getInput; | |||
| private readonly Action<IIncrementalGeneratorOutputNode> _registerOutput; | |||
| private readonly IEqualityComparer<T> _inputComparer; | |||
There was a problem hiding this comment.
ResolveAdditionalFilesFromArguments drops duplicates that are passed in for a single compilation request. The _inputComparer compares between the de-duplicated inputs from this run compared with those from the previous one.
In general we used reference equality to compare additional texts in the compiler, as they are only created once at the start of the compilation request. That's no longer true, because we create a file per-request and have to actually compare content to determine if the file has changed between invocations.
| } | ||
|
|
||
| [Fact] | ||
| public void Compiler_DoesNotCache_Compilation() |
There was a problem hiding this comment.
We don't even try and compare one compilation to another as its such a deep comparison. Generally the author will select out the parts of interest which will be comparable so the pipeline is likely to cache out fairly early.
@jaredpar's work on compilation determinism may allow us to easily compare the compilation and start caching it.
|
@cston Any further feedback on this? |
Update tests to use conditions
chsienki
force-pushed
the
source-generators/fix-generator-cache
branch
from
3b3a155 to
9bb56d7
Compare
ghost
added this to the 
…rations * upstream/main: (396 commits) Update several ExpressionCompiler unit tests for inferred delegate types (dotnet#58203) Avoid calculating inferred delegate type unnecessarily in conversions and type inference (dotnet#58115) OmniSharp options (dotnet#58208) Fix generator caching in compiler server (dotnet#57177) Clarify use of null as an initialized value BoundDecisionDag.Rewrite - Avoid capturing the replacement map (dotnet#58137) Address feedback Add regex parser tests Add additional test for another bug report Add additional test for another bug report Add additional test for another bug report Add additional test for another bug report Add additional test for another bug report Add missing delegate casts (dotnet#58170) Add additional test for another bug report Add additional test for another bug report Add additional test for another bug report Add additional test for another bug report Add additional test for another bug report Add additional test for another bug report ...
Note We're not comparing the config provider: this means every invocation we'll think it was modified even when it wasn't. I'm thinking about putting in a separate cache for the configs anyway, which would actually fix this, but for now it doesn't matter too much. Although the config provider has changed, the values that come out of it will be the same, so downstream nodes will likely be considered cached as it just produces strings.