Add generator driver cache to VBCSCompiler #55023
Conversation
- Create a new cache type - Make the compiler optionally take it - Pass in the cache when running on the server - Check the cache (if there is one) for a generator driver before creating a new one
|
My other concern here is misbehaving generators. If they store global state / do shenanigans they can effectively cause incremental builds to start failing. I think I'm ok with it, because we can always tell the user to not use the compiler server (or say 'dont use that generator' its broken) to get back to a good compilation state. |
|
@dotnet/roslyn-compiler for review please |
|
Done review pass (commit 1) |
Add feature flag to disable cache
|
Ping @dotnet/roslyn-compiler for a second review please |
src/Compilers/Core/Portable/SourceGeneration/GeneratorDriverCache.cs
Outdated
Show resolved
Hide resolved
src/Compilers/Core/Portable/SourceGeneration/GeneratorDriverCache.cs
Outdated
Show resolved
Hide resolved
|
Really would like to see an MSBuild property capable of bypassing this cache... Edit: Looks like |
| RunWithCacheDisabled(); | ||
| Assert.Equal(3, sourceCallbackCount); | ||
|
|
||
| // Clean up temp files |
sharwell
changed the title
| @@ -734,7 +736,45 @@ public virtual int Run(TextWriter consoleOutput, CancellationToken cancellationT | |||
| /// <param name="additionalTexts">Any additional texts that should be passed to the generators when run.</param> | |||
| /// <param name="generatorDiagnostics">Any diagnostics that were produced during generation</param> | |||
| /// <returns>A compilation that represents the original compilation with any additional, generated texts added to it.</returns> | |||
| private protected virtual Compilation RunGenerators(Compilation input, ParseOptions parseOptions, ImmutableArray<ISourceGenerator> generators, AnalyzerConfigOptionsProvider analyzerConfigOptionsProvider, ImmutableArray<AdditionalText> additionalTexts, DiagnosticBag generatorDiagnostics) { return input; } | |||
| private protected Compilation RunGenerators(Compilation input, ParseOptions parseOptions, ImmutableArray<ISourceGenerator> generators, AnalyzerConfigOptionsProvider analyzerConfigOptionsProvider, ImmutableArray<AdditionalText> additionalTexts, DiagnosticBag generatorDiagnostics) | |||
There was a problem hiding this comment.
One item I think that is missing here is a description of what is and is not valid to cache.
For example one invariant that must be true for this PR to be "safe" is that two different projects which have the same name and generators can re-use each others GeneratorDriver entries and that will not impact the correctness of the compilation. Given what we save in the driver today that should be safe. But surely there are some items that are not safe to save and it may be helpful to list them. Likely as a doc / doc comment on the GeneratorDriver itself.
|
@cston Any further feedback on this? |
| // Obviously that would remove the point of the cache, so we also key off of the output file name | ||
| // and output path so that collisions are unlikely and we'll usually get the correct cache for any | ||
| // given compilation. | ||
|
|
||
| PooledStringBuilder sb = PooledStringBuilder.GetInstance(); | ||
| sb.Builder.Append(Arguments.GetOutputFilePath(Arguments.OutputFileName)); |
There was a problem hiding this comment.
Were we going to add the path + file name to make multi-target caching work better?
There was a problem hiding this comment.
We already are, if you look at GetOuputFilePath
Co-authored-by: Charles Stoner <chucks@microsoft.com>
ghost
added this to the 
This enables incrementalism in the compiler server so that subsequent command line builds will be faster when using incremental generators.