-
Notifications
You must be signed in to change notification settings - Fork 196
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve perf in generator cache cases #10577
Changes from 5 commits
3606e2d
eb903b7
bfbd9cc
968afab
88aadb0
23b9433
7db79f9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -145,7 +145,39 @@ public void Initialize(IncrementalGeneratorInitializationContext context) | |
var ((compilationA, razorSourceGeneratorOptionsA), hasRazorFilesA) = a; | ||
var ((compilationB, razorSourceGeneratorOptionsB), hasRazorFilesB) = b; | ||
|
||
if (!compilationA.References.SequenceEqual(compilationB.References)) | ||
// when using the generator cache in the compiler its possible to encounter metadata references that are different instances | ||
chsienki marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Should we cache this LambdaComparer in some static field to avoid allocating it on each call? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can't because the internal lambda is capturing the outer compilations. |
||
{ | ||
if (ReferenceEquals(old, @new)) | ||
{ | ||
return true; | ||
} | ||
|
||
if (old is null || @new is null) | ||
{ | ||
return false; | ||
} | ||
|
||
var oldSymbol = compilationA.GetAssemblyOrModuleSymbol(old); | ||
var newSymbol = compilationB.GetAssemblyOrModuleSymbol(@new); | ||
|
||
if (SymbolEqualityComparer.Default.Equals(oldSymbol, newSymbol)) | ||
{ | ||
return true; | ||
} | ||
|
||
if (oldSymbol is IAssemblySymbol oldAssembly && newSymbol is IAssemblySymbol newAssembly) | ||
{ | ||
var oldModuleMVIDs = oldAssembly.Modules.Select(GetMVID); | ||
var newModuleMVIDs = newAssembly.Modules.Select(GetMVID); | ||
return oldModuleMVIDs.SequenceEqual(newModuleMVIDs); | ||
|
||
Guid GetMVID(IModuleSymbol m) => m.GetMetadata()?.GetModuleVersionId() ?? Guid.Empty; | ||
chsienki marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
return false; | ||
}))) | ||
{ | ||
return false; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3175,5 +3175,202 @@ public class LambdaGenerator(Action<IncrementalGeneratorInitializationContext> a | |
{ | ||
public void Initialize(IncrementalGeneratorInitializationContext context) => action(context); | ||
} | ||
|
||
[Fact] | ||
public async Task IncrementalCompilation_NothingRuns_When_AdditionalFiles_HaveSameContent() | ||
{ | ||
// Arrange | ||
using var eventListener = new RazorEventListener(); | ||
var project = CreateTestProject(new() | ||
{ | ||
["Pages/Index.razor"] = "<h1>Hello world</h1>", | ||
["Pages/Counter.razor"] = "<h1>Counter</h1>", | ||
}); | ||
var compilation = await project.GetCompilationAsync(); | ||
var (driver, additionalTexts) = await GetDriverWithAdditionalTextAsync(project); | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
// <auto-generated/> | ||
#pragma warning disable 1591 | ||
namespace MyApp.Pages | ||
{ | ||
#line default | ||
using global::System; | ||
using global::System.Collections.Generic; | ||
using global::System.Linq; | ||
using global::System.Threading.Tasks; | ||
using global::Microsoft.AspNetCore.Components; | ||
#line default | ||
#line hidden | ||
#nullable restore | ||
public partial class Index : global::Microsoft.AspNetCore.Components.ComponentBase | ||
#nullable disable | ||
{ | ||
#pragma warning disable 1998 | ||
protected override void BuildRenderTree(global::Microsoft.AspNetCore.Components.Rendering.RenderTreeBuilder __builder) | ||
{ | ||
__builder.AddMarkupContent(0, ""<h1>Hello world</h1>""); | ||
} | ||
#pragma warning restore 1998 | ||
} | ||
} | ||
#pragma warning restore 1591 | ||
", | ||
@"#pragma checksum ""Pages/Counter.razor"" ""{ff1816ec-aa5e-4d10-87f7-6f4963833460}"" ""0de17e526cd536d59072aa0e924e99111b16b97a"" | ||
// <auto-generated/> | ||
#pragma warning disable 1591 | ||
namespace MyApp.Pages | ||
{ | ||
#line default | ||
using global::System; | ||
using global::System.Collections.Generic; | ||
using global::System.Linq; | ||
using global::System.Threading.Tasks; | ||
using global::Microsoft.AspNetCore.Components; | ||
#line default | ||
#line hidden | ||
#nullable restore | ||
public partial class Counter : global::Microsoft.AspNetCore.Components.ComponentBase | ||
#nullable disable | ||
{ | ||
#pragma warning disable 1998 | ||
protected override void BuildRenderTree(global::Microsoft.AspNetCore.Components.Rendering.RenderTreeBuilder __builder) | ||
{ | ||
__builder.AddMarkupContent(0, ""<h1>Counter</h1>""); | ||
} | ||
#pragma warning restore 1998 | ||
} | ||
} | ||
#pragma warning restore 1591 | ||
"); | ||
|
||
Assert.Empty(result.Diagnostics); | ||
Assert.Equal(2, result.GeneratedSources.Length); | ||
|
||
eventListener.Events.Clear(); | ||
|
||
result = RunGenerator(compilation!, ref driver) | ||
.VerifyOutputsMatch(result); | ||
|
||
Assert.Empty(result.Diagnostics); | ||
Assert.Equal(2, result.GeneratedSources.Length); | ||
|
||
project = project.RemoveAdditionalDocument(project.AdditionalDocumentIds[1]) | ||
.AddAdditionalDocument("Counter.razor", SourceText.From("<h1>Counter</h1>", Encoding.UTF8)) | ||
.Project; | ||
|
||
compilation = await project.GetCompilationAsync(); | ||
|
||
result = RunGenerator(compilation!, ref driver) | ||
.VerifyOutputsMatch(result); | ||
|
||
Assert.Empty(result.Diagnostics); | ||
Assert.Equal(2, result.GeneratedSources.Length); | ||
|
||
Assert.Empty(eventListener.Events); | ||
} | ||
|
||
[Fact] | ||
public async Task IncrementalCompilation_OnlyCompilationRuns_When_MetadataReferences_SameAssembly() | ||
{ | ||
// Arrange | ||
using var eventListener = new RazorEventListener(); | ||
var project = CreateTestProject(new() | ||
{ | ||
["Pages/Index.razor"] = "<h1>Hello world</h1>", | ||
["Pages/Counter.razor"] = "<h1>Counter</h1>", | ||
}); | ||
var compilation = await project.GetCompilationAsync(); | ||
var (driver, additionalTexts) = await GetDriverWithAdditionalTextAsync(project); | ||
|
||
var result = RunGenerator(compilation!, ref driver) | ||
.VerifyPageOutput( | ||
@"#pragma checksum ""Pages/Index.razor"" ""{ff1816ec-aa5e-4d10-87f7-6f4963833460}"" ""6b5db227a6aa2228c777b0771108b184b1fc5df3"" | ||
// <auto-generated/> | ||
#pragma warning disable 1591 | ||
namespace MyApp.Pages | ||
{ | ||
#line default | ||
using global::System; | ||
using global::System.Collections.Generic; | ||
using global::System.Linq; | ||
using global::System.Threading.Tasks; | ||
using global::Microsoft.AspNetCore.Components; | ||
#line default | ||
#line hidden | ||
#nullable restore | ||
public partial class Index : global::Microsoft.AspNetCore.Components.ComponentBase | ||
#nullable disable | ||
{ | ||
#pragma warning disable 1998 | ||
protected override void BuildRenderTree(global::Microsoft.AspNetCore.Components.Rendering.RenderTreeBuilder __builder) | ||
{ | ||
__builder.AddMarkupContent(0, ""<h1>Hello world</h1>""); | ||
} | ||
#pragma warning restore 1998 | ||
} | ||
} | ||
#pragma warning restore 1591 | ||
", | ||
@"#pragma checksum ""Pages/Counter.razor"" ""{ff1816ec-aa5e-4d10-87f7-6f4963833460}"" ""0de17e526cd536d59072aa0e924e99111b16b97a"" | ||
// <auto-generated/> | ||
#pragma warning disable 1591 | ||
namespace MyApp.Pages | ||
{ | ||
#line default | ||
using global::System; | ||
using global::System.Collections.Generic; | ||
using global::System.Linq; | ||
using global::System.Threading.Tasks; | ||
using global::Microsoft.AspNetCore.Components; | ||
#line default | ||
#line hidden | ||
#nullable restore | ||
public partial class Counter : global::Microsoft.AspNetCore.Components.ComponentBase | ||
#nullable disable | ||
{ | ||
#pragma warning disable 1998 | ||
protected override void BuildRenderTree(global::Microsoft.AspNetCore.Components.Rendering.RenderTreeBuilder __builder) | ||
{ | ||
__builder.AddMarkupContent(0, ""<h1>Counter</h1>""); | ||
} | ||
#pragma warning restore 1998 | ||
} | ||
} | ||
#pragma warning restore 1591 | ||
"); | ||
|
||
Assert.Empty(result.Diagnostics); | ||
Assert.Equal(2, result.GeneratedSources.Length); | ||
|
||
eventListener.Events.Clear(); | ||
|
||
result = RunGenerator(compilation!, ref driver) | ||
.VerifyOutputsMatch(result); | ||
|
||
Assert.Empty(result.Diagnostics); | ||
Assert.Equal(2, result.GeneratedSources.Length); | ||
|
||
var reference = (PortableExecutableReference) project.MetadataReferences[^1]; | ||
|
||
project = project.RemoveMetadataReference(reference) | ||
.AddMetadataReference(MetadataReference.CreateFromFile(reference.FilePath!)); | ||
|
||
compilation = await project.GetCompilationAsync(); | ||
|
||
result = RunGenerator(compilation!, ref driver) | ||
.VerifyOutputsMatch(result); | ||
|
||
Assert.Empty(result.Diagnostics); | ||
Assert.Equal(2, result.GeneratedSources.Length); | ||
|
||
// reference causes the compilation to change so we re-run tag helper discovery there | ||
// but we didn't re-check the actual reference itself | ||
Assert.Collection(eventListener.Events, | ||
e => Assert.Equal("DiscoverTagHelpersFromCompilationStart", e.EventName), | ||
e => Assert.Equal("DiscoverTagHelpersFromCompilationStop", e.EventName)); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was never actually used, but incurred cost enumerating the references.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't seem to be hooked up on the tooling side. Perhaps we can delete the feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, looks like its only tests using it now. Will submit a follow up to clean this up.