-
Notifications
You must be signed in to change notification settings - Fork 211
Remove separate component tag helper discovery #8721
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
Conversation
|
||
var tagHelperFeature = new StaticCompilationTagHelperFeature(); | ||
var discoveryProjectEngine = GetDiscoveryProjectEngine(compilation.References.ToImmutableArray(), tagHelperFeature); | ||
|
||
tagHelperFeature.Compilation = compilation; | ||
tagHelperFeature.TargetSymbol = compilation.Assembly; | ||
var compilationWithDeclarations = compilation.AddSyntaxTrees(generatedDeclarationSyntaxTrees); |
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 is a pretty significant performance de-opt. I don't think I'm understanding why it's necessary; what's actually causing the errors here?
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.
Right, forgot to explain, sorry. For example, consider the test added in this PR. The component is discovered both in tagHelpersFromComponents
step and tagHelpersFromCompilation
step. In the latter, the descriptor is different than in the former, because the component from the .cs
file has a property ChildContent
(appears as BoundAttribute
on the descriptor), so both tag helper descriptors remain.
Yes, we could deduplicate them somehow, I guess. But I'm not sure if this is such de-opt, since compilation.AddSyntaxTrees(generatedDeclarationSyntaxTrees);
was already present (in the tagHelpersFromComponents
step). So I did it this way because I think it's safer (more similar to how it worked previously, before the regression was introduced).
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.
The previous code did not need to do a full discovery on the entire compilation every time a razor file changed. This will do that, and that reduction in generator activity is where the performance savings came from. I think we need to investigate the deduping; separately, we need to decide if #8718 is a regression that needs to be serviced. If it is, then maybe we take this change and immediately work on the dedup problem.
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.
Not sure what you mean by previous code. But before the perf improvements, the code did a full discovery on the entire compilation every time a razor file changed:
razor/src/Compiler/Microsoft.NET.Sdk.Razor.SourceGenerators/RazorSourceGenerator.cs
Lines 94 to 131 in fcae931
var tagHelpersFromCompilation = compilation | |
.Combine(generatedDeclarationSyntaxTrees.Collect()) | |
.Combine(razorSourceGeneratorOptions) | |
.Select(static (pair, _) => | |
{ | |
RazorSourceGeneratorEventSource.Log.DiscoverTagHelpersFromCompilationStart(); | |
var ((compilation, generatedDeclarationSyntaxTrees), razorSourceGeneratorOptions) = pair; | |
var tagHelperFeature = new StaticCompilationTagHelperFeature(); | |
var discoveryProjectEngine = GetDiscoveryProjectEngine(compilation.References.ToImmutableArray(), tagHelperFeature); | |
var compilationWithDeclarations = compilation.AddSyntaxTrees(generatedDeclarationSyntaxTrees); | |
tagHelperFeature.Compilation = compilationWithDeclarations; | |
tagHelperFeature.TargetSymbol = compilationWithDeclarations.Assembly; | |
var result = (IList<TagHelperDescriptor>)tagHelperFeature.GetDescriptors(); | |
RazorSourceGeneratorEventSource.Log.DiscoverTagHelpersFromCompilationStop(); | |
return result; | |
}) | |
.WithLambdaComparer(static (a, b) => | |
{ | |
if (a.Count != b.Count) | |
{ | |
return false; | |
} | |
for (var i = 0; i < a.Count; i++) | |
{ | |
if (!a[i].Equals(b[i])) | |
{ | |
return false; | |
} | |
} | |
return true; | |
}, getHashCode: static a => a.Count); |
Right now in main
, you're right it's better, the discovery still happens for the full compilation (since fixing another regression in #8614) but it's scoped per component (tagHelperFeature.TargetSymbol = targetSymbol;
):
razor/src/Compiler/Microsoft.NET.Sdk.Razor.SourceGenerators/RazorSourceGenerator.cs
Lines 97 to 128 in 5803c80
var tagHelpersFromComponents = generatedDeclarationSyntaxTrees | |
.Combine(generatedDeclarationSyntaxTrees.Collect()) | |
.Combine(compilation) | |
.Combine(razorSourceGeneratorOptions) | |
.SelectMany(static (pair, ct) => | |
{ | |
var (((generatedDeclarationSyntaxTree, allGeneratedDeclarationSyntaxTrees), compilation), razorSourceGeneratorOptions) = pair; | |
RazorSourceGeneratorEventSource.Log.DiscoverTagHelpersFromComponentStart(generatedDeclarationSyntaxTree.FilePath); | |
var tagHelperFeature = new StaticCompilationTagHelperFeature(); | |
var discoveryProjectEngine = GetDiscoveryProjectEngine(compilation.References.ToImmutableArray(), tagHelperFeature); | |
var compilationWithDeclarations = compilation.AddSyntaxTrees(allGeneratedDeclarationSyntaxTrees); | |
// try and find the specific root class this component is declaring, falling back to the assembly if for any reason the code is not in the shape we expect | |
ISymbol targetSymbol = compilationWithDeclarations.Assembly; | |
var root = generatedDeclarationSyntaxTree.GetRoot(ct); | |
if (root is CompilationUnitSyntax { Members: [NamespaceDeclarationSyntax { Members: [ClassDeclarationSyntax classSyntax, ..] }, ..] }) | |
{ | |
var declaredClass = compilationWithDeclarations.GetSemanticModel(generatedDeclarationSyntaxTree).GetDeclaredSymbol(classSyntax, ct); | |
Debug.Assert(declaredClass is null || declaredClass is { AllInterfaces: [{ Name: "IComponent" }, ..] }); | |
targetSymbol = declaredClass ?? targetSymbol; | |
} | |
tagHelperFeature.Compilation = compilationWithDeclarations; | |
tagHelperFeature.TargetSymbol = targetSymbol; | |
var result = tagHelperFeature.GetDescriptors(); | |
RazorSourceGeneratorEventSource.Log.DiscoverTagHelpersFromComponentStop(generatedDeclarationSyntaxTree.FilePath); | |
return result; | |
}); |
Scheme = "http" | ||
}; | ||
requestFeature.Headers.Host = "localhost"; | ||
httpContext.Features.Set<IHttpRequestFeature>(requestFeature); |
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.
Small improvement of the test infra to allow rendering also Blazor components.
Rejected in favor of the full revert. |
Fixes #8718 (see also #8718 (comment)).
This is a regression introduced in #8212.
I've removed the
tagHelpersFromComponents
step and made thetagHelpersFromCompilation
step also go over components by appending all component declaration files to the compilation. Since #8614, all component declaration files were appended to compilation anyway (in the now removed step), so this seems simplest. If we figure out how to get rid of #8614 (appending all declaration files), then we will be able to alternatively fix this by deduplicating tag helpers from components+compilation. For example, I would go over tag helpers from compilation, checking forpartial
classes, and those that have the same fully qualified name already present in tag helpers from components could be removed from the final set of all tag helpers.