Improve perf in generator cache cases #10577
Conversation
| @@ -74,7 +74,6 @@ private static StaticCompilationTagHelperFeature GetStaticTagHelperFeature(Compi | |||
| // the tagHelperFeature will have its Engine property set as part of adding it to the engine, which is used later when doing the actual discovery | |||
| var discoveryProjectEngine = RazorProjectEngine.Create(RazorConfiguration.Default, new VirtualRazorProjectFileSystem(), b => | |||
| { | |||
| b.Features.Add(new DefaultMetadataReferenceFeature { References = compilation.References.ToImmutableArray() }); | |||
There was a problem hiding this comment.
This was never actually used, but incurred cost enumerating the references.
There was a problem hiding this comment.
Doesn't seem to be hooked up on the tooling side. Perhaps we can delete the feature?
There was a problem hiding this comment.
Yeah, looks like its only tests using it now. Will submit a follow up to clean this up.
src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/SourceGenerators/RazorSourceGenerator.cs
Outdated
Show resolved
Hide resolved
...ler/Microsoft.CodeAnalysis.Razor.Compiler/src/SourceGenerators/SourceGeneratorProjectItem.cs
Outdated
Show resolved
Hide resolved
src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/SourceGenerators/RazorSourceGenerator.cs
Outdated
Show resolved
Hide resolved
...ler/Microsoft.CodeAnalysis.Razor.Compiler/src/SourceGenerators/SourceGeneratorProjectItem.cs
Outdated
Show resolved
Hide resolved
src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/SourceGenerators/RazorSourceGenerator.cs
Outdated
Show resolved
Hide resolved
| if (!compilationA.References.SequenceEqual(compilationB.References)) | ||
| // when using the generator cache in the compiler its possible to encounter metadata references that are different instances | ||
| // but ultimately represent the same underlying assembly. We compare the module version ids to determine if the references are the same | ||
| if (!compilationA.References.SequenceEqual(compilationB.References, new LambdaComparer<MetadataReference>((old, @new) => |
There was a problem hiding this comment.
new LambdaComparer<MetadataReference>
Should we cache this LambdaComparer in some static field to avoid allocating it on each call?
There was a problem hiding this comment.
We can't because the internal lambda is capturing the outer compilations.
src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/SourceGenerators/RazorSourceGenerator.cs
Outdated
Show resolved
Hide resolved
...ler/Microsoft.CodeAnalysis.Razor.Compiler/src/SourceGenerators/SourceGeneratorProjectItem.cs
Outdated
Show resolved
Hide resolved
|
|
||
| var result = RunGenerator(compilation!, ref driver) | ||
| .VerifyPageOutput( | ||
| @"#pragma checksum ""Pages/Index.razor"" ""{ff1816ec-aa5e-4d10-87f7-6f4963833460}"" ""6b5db227a6aa2228c777b0771108b184b1fc5df3"" |
There was a problem hiding this comment.
Could we save this page output which is the same across many tests in some common variable so we don't have to update so many tests when we change codegen?
Alternatively, I think we might not need to verify page output in these new tests at all since their purpose is not to verify the codegen but rather the incrementality of the source generator.
(Or we have .VerifyOutputsMatchBaseline(); that would move the baseline into an automatically-updatable file.)
There was a problem hiding this comment.
I just removed the strings, as we aren't actually testing the codegen here. Agree we should consider moving these tests over to external baselines though.
src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/SourceGenerators/RazorSourceGenerator.cs
Outdated
Show resolved
Hide resolved
src/Compiler/test/Microsoft.NET.Sdk.Razor.SourceGenerators.Tests/RazorSourceGeneratorTests.cs
Outdated
Show resolved
Hide resolved
...ler/Microsoft.CodeAnalysis.Razor.Compiler/src/SourceGenerators/SourceGeneratorProjectItem.cs
Outdated
Show resolved
Hide resolved
| // also break in those cases, so for now we're okay with this. | ||
| var thisHash = AdditionalText.GetText()?.GetContentHash() ?? []; | ||
| var otherHash = other?.AdditionalText.GetText()?.GetContentHash() ?? []; | ||
| return thisHash.SequenceEqual(otherHash); |
There was a problem hiding this comment.
i'm not a fan of non-null objects being considered equal to null.
When using the generator cache feature of VBCSCompiler, the additional files and metadata references passed to the generator driver are recreated each time. While the long-term fix is to fix this in VBCSCompiler, previous attempts broke other users, and the fix is quite involved.
In order to allow users to make use of the generator cache feature as-is, this change adds some extra checks for the additional files and metadata references. When they have changed, but their content remains the same, we treat them as being cached.
On large projects this brings significant speed ups when combined with the generator cache feature.
In the IDE these items are reference equal, so these new comparisons won't be used. In compilation scenarios without the cache, nothing is cached, so they are never used. They will only be used in the cases where the user explicitly opts-in to the compiler cache feature.