Skip to content
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

Merged
merged 7 commits into from
Jul 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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() });
Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

b.Features.Add(tagHelperFeature);
b.Features.Add(new DefaultTagHelperDescriptorProvider());

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 it's 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) =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new LambdaComparer<MetadataReference>

Should we cache this LambdaComparer in some static field to avoid allocating it on each call?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);

static Guid GetMVID(IModuleSymbol m) => m.GetMetadata()?.GetModuleVersionId() ?? Guid.Empty;
}

return false;
})))
{
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using System.IO;
using System.Linq;
using Microsoft.AspNetCore.Razor.Language;
using Microsoft.CodeAnalysis;

Expand Down Expand Up @@ -47,7 +48,21 @@ public override Stream Read()
=> throw new NotSupportedException("This API should not be invoked. We should instead be relying on " +
"the RazorSourceDocument associated with this item instead.");

public bool Equals(SourceGeneratorProjectItem? other) => other is not null && AdditionalText == other.AdditionalText;
public bool Equals(SourceGeneratorProjectItem? other)
{
if (ReferenceEquals(AdditionalText, other?.AdditionalText))
{
return true;
}

// In the compiler server when the generator driver cache is enabled the
// additional files are always different instances even if their content is the same.
// It's technically possible for these hashes to collide, but other things would
// 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not a fan of non-null objects being considered equal to null.

}

public override int GetHashCode() => AdditionalText.GetHashCode();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3175,5 +3175,93 @@ 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);
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);
Assert.Empty(eventListener.Events);

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);

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);
Assert.Empty(eventListener.Events);

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));
}
}
}