Skip to content

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.

Sign up for GitHub

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

Closed
wants to merge 6 commits into from
Closed

Remove separate component tag helper discovery #8721

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c82595c
Add a test
jjonescz May 17, 2023
3a80d78
Remove separate component tag helper discovery
jjonescz May 17, 2023
c4399eb
Update tests
jjonescz May 17, 2023
5d79b44
Add test without base class in C#
jjonescz May 17, 2023
eab33ed
Improve tests
jjonescz May 18, 2023
e6ffc6c
Test with `[HtmlTargetElement]`
jjonescz May 18, 2023
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
52 changes: 9 additions & 43 deletions src/Compiler/Microsoft.NET.Sdk.Razor.SourceGenerators/RazorSourceGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,12 @@

using System;
using System.Collections.Immutable;
using System.Diagnostics;
using System.IO;
using System.Linq;
using Microsoft.AspNetCore.Razor.Language;
using Microsoft.AspNetCore.Razor.PooledObjects;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;

namespace Microsoft.NET.Sdk.Razor.SourceGenerators
{
Expand Down Expand Up @@ -94,52 +92,22 @@ public void Initialize(IncrementalGeneratorInitializationContext context)
return CSharpSyntaxTree.ParseText(generatedDeclarationCode, (CSharpParseOptions)parseOptions, filePath, cancellationToken: ct);
});

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

var tagHelpersFromCompilation = compilation
.Combine(generatedDeclarationSyntaxTrees.Collect())
.Combine(razorSourceGeneratorOptions)
.Select(static (pair, _) =>
{
RazorSourceGeneratorEventSource.Log.DiscoverTagHelpersFromCompilationStart();

var (compilation, razorSourceGeneratorOptions) = pair;
var ((compilation, generatedDeclarationSyntaxTrees), razorSourceGeneratorOptions) = pair;

var tagHelperFeature = new StaticCompilationTagHelperFeature();
var discoveryProjectEngine = GetDiscoveryProjectEngine(compilation.References.ToImmutableArray(), tagHelperFeature);

tagHelperFeature.Compilation = compilation;
tagHelperFeature.TargetSymbol = compilation.Assembly;
var compilationWithDeclarations = compilation.AddSyntaxTrees(generatedDeclarationSyntaxTrees);
Copy link
Member

@333fred 333fred May 17, 2023

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?

Copy link
Member Author

@jjonescz jjonescz May 17, 2023 •
edited
Loading

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

Copy link
Member

@333fred 333fred May 17, 2023 •
edited by jjonescz
Loading

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.

Copy link
Member Author

@jjonescz jjonescz May 18, 2023

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


tagHelperFeature.Compilation = compilationWithDeclarations;
tagHelperFeature.TargetSymbol = compilationWithDeclarations.Assembly;

var result = tagHelperFeature.GetDescriptors();
RazorSourceGeneratorEventSource.Log.DiscoverTagHelpersFromCompilationStop();
Expand Down Expand Up @@ -202,13 +170,12 @@ public void Initialize(IncrementalGeneratorInitializationContext context)
return descriptors.ToImmutable();
});

var allTagHelpers = tagHelpersFromComponents.Collect()
.Combine(tagHelpersFromCompilation)
var allTagHelpers = tagHelpersFromCompilation
.Combine(tagHelpersFromReferences)
.Select(static (pair, _) =>
{
var ((tagHelpersFromComponents, tagHelpersFromCompilation), tagHelpersFromReferences) = pair;
var count = tagHelpersFromCompilation.Length + tagHelpersFromReferences.Length + tagHelpersFromComponents.Length;
var (tagHelpersFromCompilation, tagHelpersFromReferences) = pair;
var count = tagHelpersFromCompilation.Length + tagHelpersFromReferences.Length;
if (count == 0)
{
return ImmutableArray<TagHelperDescriptor>.Empty;
Expand All @@ -217,7 +184,6 @@ public void Initialize(IncrementalGeneratorInitializationContext context)
using var pool = ArrayBuilderPool<TagHelperDescriptor>.GetPooledObject(out var allTagHelpers);
allTagHelpers.AddRange(tagHelpersFromCompilation);
allTagHelpers.AddRange(tagHelpersFromReferences);
allTagHelpers.AddRange(tagHelpersFromComponents);

return allTagHelpers.ToImmutable();
});
Expand Down
93 changes: 93 additions & 0 deletions ...test/Microsoft.NET.Sdk.Razor.SourceGenerators.Tests/RazorSourceGeneratorComponentTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Threading.Tasks;
using Microsoft.AspNetCore.Razor.Test.Common;
using Xunit;

namespace Microsoft.NET.Sdk.Razor.SourceGenerators;

public sealed class RazorSourceGeneratorComponentTests : RazorSourceGeneratorTestsBase
{
[Fact, WorkItem("https://github.com/dotnet/razor/issues/8718")]
public async Task PartialClass()
{
// Arrange
var project = CreateTestProject(new()
{
["Shared/Component1.razor"] = """
<Component2 />
""",
["Shared/Component2.razor"] = """
@inherits ComponentBase

@code {
[Parameter]
public RenderFragment? ChildContent { get; set; }
}
"""
}, new()
{
["Component2.razor.cs"] = """
using Microsoft.AspNetCore.Components;

namespace MyApp.Shared;

public partial class Component2 : ComponentBase
{

}
"""
});
var compilation = await project.GetCompilationAsync();
var driver = await GetDriverAsync(project);

// Act
var result = RunGenerator(compilation!, ref driver);

// Assert
Assert.Empty(result.Diagnostics);
Assert.Equal(2, result.GeneratedSources.Length);
}

[Fact, WorkItem("https://github.com/dotnet/razor/issues/8718")]
public async Task PartialClass_NoBaseInCSharp()
{
// Arrange
var project = CreateTestProject(new()
{
["Shared/Component1.razor"] = """
<Component2 />
""",
["Shared/Component2.razor"] = """
@inherits ComponentBase

@code {
[Parameter]
public RenderFragment? ChildContent { get; set; }
}
"""
}, new()
{
["Component2.razor.cs"] = """
using Microsoft.AspNetCore.Components;

namespace MyApp.Shared;

public partial class Component2
{

}
"""
});
var compilation = await project.GetCompilationAsync();
var driver = await GetDriverAsync(project);

// Act
var result = RunGenerator(compilation!, ref driver);

// Assert
Assert.Empty(result.Diagnostics);
Assert.Equal(2, result.GeneratedSources.Length);
}
}
42 changes: 16 additions & 26 deletions ...Compiler/test/Microsoft.NET.Sdk.Razor.SourceGenerators.Tests/RazorSourceGeneratorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,10 +171,6 @@ protected override void BuildRenderTree(global::Microsoft.AspNetCore.Components.
e => e.AssertSingleItem("GenerateDeclarationCodeStop", "Pages/Index.razor"),
e => e.AssertSingleItem("GenerateDeclarationCodeStart", "Pages/Counter.razor"),
e => e.AssertSingleItem("GenerateDeclarationCodeStop", "Pages/Counter.razor"),
e => e.AssertSingleItem("DiscoverTagHelpersFromComponentStart", "Pages/Index.razor"),
e => e.AssertSingleItem("DiscoverTagHelpersFromComponentStop", "Pages/Index.razor"),
e => e.AssertSingleItem("DiscoverTagHelpersFromComponentStart", "Pages/Counter.razor"),
e => e.AssertSingleItem("DiscoverTagHelpersFromComponentStop", "Pages/Counter.razor"),
e => Assert.Equal("DiscoverTagHelpersFromCompilationStart", e.EventName),
e => Assert.Equal("DiscoverTagHelpersFromCompilationStop", e.EventName),
e => Assert.Equal("DiscoverTagHelpersFromReferencesStart", e.EventName),
Expand Down Expand Up @@ -483,12 +479,12 @@ public class Person
Assert.Equal(2, result.GeneratedSources.Length);

Assert.Collection(eventListener.Events,
e => e.AssertSingleItem("DiscoverTagHelpersFromComponentStart", "Pages/Index.razor"),
e => e.AssertSingleItem("DiscoverTagHelpersFromComponentStop", "Pages/Index.razor"),
e => e.AssertSingleItem("DiscoverTagHelpersFromComponentStart", "Pages/Counter.razor"),
e => e.AssertSingleItem("DiscoverTagHelpersFromComponentStop", "Pages/Counter.razor"),
e => Assert.Equal("DiscoverTagHelpersFromCompilationStart", e.EventName),
e => Assert.Equal("DiscoverTagHelpersFromCompilationStop", e.EventName)
e => Assert.Equal("DiscoverTagHelpersFromCompilationStop", e.EventName),
e => e.AssertSingleItem("CheckAndRewriteTagHelpersStart", "Pages/Index.razor"),
e => e.AssertSingleItem("CheckAndRewriteTagHelpersStop", "Pages/Index.razor"),
e => e.AssertSingleItem("CheckAndRewriteTagHelpersStart", "Pages/Counter.razor"),
e => e.AssertSingleItem("CheckAndRewriteTagHelpersStop", "Pages/Counter.razor")
);
}

Expand Down Expand Up @@ -596,12 +592,12 @@ public class Person
Assert.Equal(2, result.GeneratedSources.Length);

Assert.Collection(eventListener.Events,
e => e.AssertSingleItem("DiscoverTagHelpersFromComponentStart", "Pages/Index.razor"),
e => e.AssertSingleItem("DiscoverTagHelpersFromComponentStop", "Pages/Index.razor"),
e => e.AssertSingleItem("DiscoverTagHelpersFromComponentStart", "Pages/Counter.razor"),
e => e.AssertSingleItem("DiscoverTagHelpersFromComponentStop", "Pages/Counter.razor"),
e => Assert.Equal("DiscoverTagHelpersFromCompilationStart", e.EventName),
e => Assert.Equal("DiscoverTagHelpersFromCompilationStop", e.EventName)
e => Assert.Equal("DiscoverTagHelpersFromCompilationStop", e.EventName),
e => e.AssertSingleItem("CheckAndRewriteTagHelpersStart", "Pages/Index.razor"),
e => e.AssertSingleItem("CheckAndRewriteTagHelpersStop", "Pages/Index.razor"),
e => e.AssertSingleItem("CheckAndRewriteTagHelpersStart", "Pages/Counter.razor"),
e => e.AssertSingleItem("CheckAndRewriteTagHelpersStop", "Pages/Counter.razor")
);
}

Expand Down Expand Up @@ -751,12 +747,12 @@ protected override void BuildRenderTree(global::Microsoft.AspNetCore.Components.
e => e.AssertSingleItem("ParseRazorDocumentStop", "Pages/Counter.razor"),
e => e.AssertSingleItem("GenerateDeclarationCodeStart", "Pages/Counter.razor"),
e => e.AssertSingleItem("GenerateDeclarationCodeStop", "Pages/Counter.razor"),
e => e.AssertSingleItem("DiscoverTagHelpersFromComponentStart", "Pages/Index.razor"),
e => e.AssertSingleItem("DiscoverTagHelpersFromComponentStop", "Pages/Index.razor"),
e => e.AssertSingleItem("DiscoverTagHelpersFromComponentStart", "Pages/Counter.razor"),
e => e.AssertSingleItem("DiscoverTagHelpersFromComponentStop", "Pages/Counter.razor"),
e => Assert.Equal("DiscoverTagHelpersFromCompilationStart", e.EventName),
e => Assert.Equal("DiscoverTagHelpersFromCompilationStop", e.EventName),
e => e.AssertSingleItem("RewriteTagHelpersStart", "Pages/Counter.razor"),
e => e.AssertSingleItem("RewriteTagHelpersStop", "Pages/Counter.razor"),
e => e.AssertSingleItem("CheckAndRewriteTagHelpersStart", "Pages/Index.razor"),
e => e.AssertSingleItem("CheckAndRewriteTagHelpersStop", "Pages/Index.razor"),
e => e.AssertSingleItem("CheckAndRewriteTagHelpersStart", "Pages/Counter.razor"),
e => e.AssertSingleItem("CheckAndRewriteTagHelpersStop", "Pages/Counter.razor"),
e => e.AssertSingleItem("RazorCodeGenerateStart", "Pages/Counter.razor"),
Expand Down Expand Up @@ -915,10 +911,8 @@ protected override void BuildRenderTree(global::Microsoft.AspNetCore.Components.
e => e.AssertSingleItem("ParseRazorDocumentStop", "Pages/Counter.razor"),
e => e.AssertSingleItem("GenerateDeclarationCodeStart", "Pages/Counter.razor"),
e => e.AssertSingleItem("GenerateDeclarationCodeStop", "Pages/Counter.razor"),
e => e.AssertSingleItem("DiscoverTagHelpersFromComponentStart", "Pages/Index.razor"),
e => e.AssertSingleItem("DiscoverTagHelpersFromComponentStop", "Pages/Index.razor"),
e => e.AssertSingleItem("DiscoverTagHelpersFromComponentStart", "Pages/Counter.razor"),
e => e.AssertSingleItem("DiscoverTagHelpersFromComponentStop", "Pages/Counter.razor"),
e => Assert.Equal("DiscoverTagHelpersFromCompilationStart", e.EventName),
e => Assert.Equal("DiscoverTagHelpersFromCompilationStop", e.EventName),
e => e.AssertSingleItem("RewriteTagHelpersStart", "Pages/Counter.razor"),
e => e.AssertSingleItem("RewriteTagHelpersStop", "Pages/Counter.razor"),
e => e.AssertSingleItem("CheckAndRewriteTagHelpersStart", "Pages/Index.razor"),
Expand Down Expand Up @@ -1061,10 +1055,6 @@ protected override void BuildRenderTree(global::Microsoft.AspNetCore.Components.
Assert.Equal(2, result.GeneratedSources.Length);

Assert.Collection(eventListener.Events,
e => e.AssertSingleItem("DiscoverTagHelpersFromComponentStart", "Pages/Index.razor"),
e => e.AssertSingleItem("DiscoverTagHelpersFromComponentStop", "Pages/Index.razor"),
e => e.AssertSingleItem("DiscoverTagHelpersFromComponentStart", "Pages/Counter.razor"),
e => e.AssertSingleItem("DiscoverTagHelpersFromComponentStop", "Pages/Counter.razor"),
e => Assert.Equal("DiscoverTagHelpersFromCompilationStart", e.EventName),
e => Assert.Equal("DiscoverTagHelpersFromCompilationStop", e.EventName),
e => Assert.Equal("DiscoverTagHelpersFromReferencesStart", e.EventName),
Expand Down