Perf/generator #8212

chsienki · 2023-02-02T23:57:26Z

Change the structure of the generator to ensure we don't recalculate things that don't need to be re-calculated:

Break up tag helper discovery into components (from razor files) and compilation (from csharp)
When discovering tag helpers from components, look at each component individually rather than as a group
Break up parsing into multiple steps
- Do an initial parse only once
- If a document has changed, re-write the tag helpers, recording which ones were used
- Re-run the tag helper re-write step, but check if the tag helpers have changed, and skip if we can

- Update event log names - Update tests to match

chsienki · 2023-02-03T00:13:03Z

src/Compiler/Microsoft.NET.Sdk.Razor.SourceGenerators/RazorSourceGenerator.cs


-            var tagHelpersFromCompilation = compilation
-                .Combine(generatedDeclarationSyntaxTrees.Collect())
+            var tagHelpersFromComponents = generatedDeclarationSyntaxTrees


This allows us to only look at razor files that have changed to get their updated tag helpers, rather than re-discovering everything anytime a single file changes.

chsienki · 2023-02-03T00:14:06Z

src/Compiler/Microsoft.NET.Sdk.Razor.SourceGenerators/RazorSourceGenerator.cs

+                    var compilationWithDeclarations = compilation.AddSyntaxTrees(generatedDeclarationSyntaxTree);
+
+                    // try and find the specific class this component is declaring, falling back to the assembly if for any reason we can't
+                    ISymbol targetSymbol = compilationWithDeclarations.Assembly;


We need to look at the whole compilation with the component added (as it might reference stuff from csharp), but there's no need to actually discover anything else from the compilation, so only search the component itself.

chsienki · 2023-02-03T00:15:47Z

src/Compiler/Microsoft.NET.Sdk.Razor.SourceGenerators/RazorSourceGenerator.cs

-                .WithLambdaComparer(static (a, b) =>
+                });
+
+            var tagHelpersFromCompilation = compilation


We do a single compilation lookup here. If a user edits CSharp this runs without running any of the component stuff again. If the user edits a component, this no longer runs.

chsienki · 2023-02-03T00:23:15Z

src/Compiler/Microsoft.NET.Sdk.Razor.SourceGenerators/RazorSourceGenerator.cs

+				})
+
+				// Add the tag helpers in, but ignore if they've changed or not, only reprocessing the actual document changed
+				.Combine(allTagHelpers)


This is the most impactful part of the change:

We re-write the document if it has changed, but we ignore the status of the tag helpers.

The first time through all documents will be changed, so we run with an up-to-date set of tag helpers.

Next time through, only documents that have changed will be re-written (using the latest tag helpers). Those that haven't changed are skipped (and might have incorrect tag helpers at this point).

Then, on line 253 we re-run again, not skipping the tag helper check:

(If neither tag helper nor document has changed, there is no work to do)

When we re-process we pass the checkIdempotency flag to the engine. This will see if the same taghelpers that were used last time are being passed this time and skip if so.

This effectively makes the two steps mutually exclusive: we either ran the first step, and thus the tag helpers are up to date and we skip doing anything here; or the document didn't change (so we skipped the first step) but the tag helpers are different meaning we need to re-write them.

chsienki · 2023-02-03T00:25:47Z

src/Compiler/Microsoft.NET.Sdk.Razor.SourceGenerators/SourceGeneratorProjectEngine.cs

+		int startIndex = discoveryPhaseIndex;
+		var codeDocument = sgDocument.CodeDocument;
+		var previousTagHelpers = codeDocument.GetTagHelpers();
+		if (checkForIdempotency && previousTagHelpers is not null)


This is where we check the idempotency of the tag helpers. If they haven't changed, we can skip re-writing. If they have, we can check if the ones that changed can actually have an impact on the document and skip if not.

chsienki · 2023-02-03T00:27:04Z

@dotnet/razor-compiler for review please. It's not too big, but I did try and split things up logically with the commits so you may find it easier to go commit-by-commit. I've also added some extra comments explaining the logic a bit in a few places.

src/Compiler/Microsoft.NET.Sdk.Razor.SourceGenerators/RazorSourceGenerator.cs

src/Compiler/Microsoft.NET.Sdk.Razor.SourceGenerators/SourceGeneratorProjectEngine.cs

src/Compiler/Microsoft.NET.Sdk.Razor.SourceGenerators/RazorSourceGenerator.cs

jjonescz · 2023-02-03T13:26:26Z

src/Compiler/Microsoft.NET.Sdk.Razor.SourceGenerators/RazorSourceGenerator.cs

+
+                    // try and find the specific class this component is declaring, falling back to the assembly if for any reason we can't
+                    ISymbol targetSymbol = compilationWithDeclarations.Assembly;
+                    var classSyntax = generatedDeclarationSyntaxTree.GetRoot(ct).DescendantNodes().SingleOrDefault(n => n.IsKind(CodeAnalysis.CSharp.SyntaxKind.ClassDeclaration));


Can there be nested classes in the declaration syntax tree? Then SingleOrDefault could throw.

Great catch. I've added a test, and fixed via patterns instead.

src/Compiler/Microsoft.NET.Sdk.Razor.SourceGenerators/RazorSourceGenerator.cs

333fred

Putting out some comments now, will look more later this afternoon.

src/Compiler/Microsoft.NET.Sdk.Razor.SourceGenerators/RazorSourceGenerator.cs

src/Compiler/Microsoft.NET.Sdk.Razor.SourceGenerators/SourceGeneratorProjectEngine.cs

src/Compiler/Microsoft.NET.Sdk.Razor.SourceGenerators/RazorSourceGenerator.cs

chsienki · 2023-03-28T00:11:30Z

@333fred for a second review, thanks

chsienki · 2023-03-28T16:59:43Z

Hmm, merge looks bad. Investigating.

333fred

Done review pass. Only minor comments.

src/Compiler/Microsoft.CodeAnalysis.Razor/src/DefaultTagHelperDescriptorFactory.cs

src/Compiler/Microsoft.CodeAnalysis.Razor/src/SymbolExtensions.cs

src/Compiler/Microsoft.NET.Sdk.Razor.SourceGenerators/StaticCompilationTagHelperFeature.cs

This reverts commit 4ab512e.

* Revert "Perf/generator (#8212)" This reverts commit 4ab512e. * Add more tests

…elease/dev17.6-to-release/dev17.7 * upstream/release/dev17.6: [17.6] Revert "Perf/generator (#8212)" (#8742)

…elease/dev17.7-to-main * upstream/release/dev17.7: Update baseline. [17.6] Revert "Perf/generator (#8212)" (#8742)

Based on dotnet#8212 - Do an initial parse only once - If a document has changed, re-write the tag helpers, recording which ones were used - Re-run the tag helper re-write step, but check if the tag helpers have changed, and skip if we can

* Restore perf work. Based on #8212 - Do an initial parse only once - If a document has changed, re-write the tag helpers, recording which ones were used - Re-run the tag helper re-write step, but check if the tag helpers have changed, and skip if we can * PR Feedback. - Sealed, unreachable etc. - Refactor tag helper static feature aquisition to make it clearer - Remove unused tag helper methods * Update src/Compiler/Microsoft.NET.Sdk.Razor.SourceGenerators/SourceGeneratorRazorCodeDocument.cs Co-authored-by: Fred Silberberg <[email protected]> * PR Feedback --------- Co-authored-by: Fred Silberberg <[email protected]>

chsienki added 6 commits February 2, 2023 15:16

Don't re-run declaration code if nothing has changed

c7a95d3

Clean up test file

3523147

Split tag helper discovery into component and compilation discovery

04f79ff

Create a wrapper source generator engine that can selectively run phases

6a4a589

Ensure we don't re-use a rewritten syntax tree

89660be

Break output into multiple steps that can be largely skipped as needed

7616424

- Update event log names - Update tests to match

chsienki added perf 📊 area-compiler Umbrella for all compiler issues labels Feb 2, 2023

chsienki added this to the 17.6 P1 milestone Feb 2, 2023

Remove uneeded tag helper discovery from SG engine

489f81c

chsienki force-pushed the perf/generator branch from effb665 to 489f81c Compare February 3, 2023 00:11

chsienki commented Feb 3, 2023

View reviewed changes

chsienki marked this pull request as ready for review February 3, 2023 00:27

chsienki requested a review from a team as a code owner February 3, 2023 00:27

Swap asserts

ff6f5b4

jjonescz reviewed Feb 3, 2023

View reviewed changes

chsienki added 2 commits February 3, 2023 10:54

Formatting

312ff75

PR feedback

9422b03

jjonescz approved these changes Feb 6, 2023

View reviewed changes

src/Compiler/Microsoft.NET.Sdk.Razor.SourceGenerators/RazorSourceGenerator.cs Outdated Show resolved Hide resolved

Formatting

ca2a9e5

333fred reviewed Feb 6, 2023

View reviewed changes

src/Compiler/Microsoft.NET.Sdk.Razor.SourceGenerators/SourceGeneratorProjectEngine.cs Show resolved Hide resolved

jjonescz reviewed Feb 7, 2023

View reviewed changes

src/Compiler/Microsoft.NET.Sdk.Razor.SourceGenerators/RazorSourceGenerator.cs Outdated Show resolved Hide resolved

jjonescz mentioned this pull request Feb 14, 2023

Emit host outputs #8242

Merged

3 tasks

PR Feedback

fb07d1c

Fix tests

30c9815

333fred reviewed Feb 23, 2023

View reviewed changes

src/Compiler/Microsoft.NET.Sdk.Razor.SourceGenerators/RazorSourceGenerator.cs Outdated Show resolved Hide resolved

chsienki changed the base branch from release/dev17.6 to main February 23, 2023 19:40

Merge branch 'main' into perf/generator

baaddfa

chsienki force-pushed the perf/generator branch from 20bd66c to c60b4bf Compare March 28, 2023 00:04

Use array builder pools

46dbf1e

chsienki force-pushed the perf/generator branch from c60b4bf to 46dbf1e Compare March 28, 2023 00:05

Merge remote-tracking branch 'upstream/main' into perf/generator

5cf0d50

Fix merge

c430fea

333fred reviewed Mar 28, 2023

View reviewed changes

PR Feedback

5ce316d

333fred approved these changes Mar 28, 2023

View reviewed changes

chsienki enabled auto-merge (squash) March 28, 2023 20:25

chsienki merged commit 4ab512e into dotnet:main Mar 28, 2023

jjonescz mentioned this pull request Apr 19, 2023

Generator incorrectly prefixes relative namespaced types with global:: #8610

Closed

This was referenced May 17, 2023

Blazor regression in 7.0.302 SDK #8718

Closed

Remove separate component tag helper discovery #8721

Closed

ScarletKuro mentioned this pull request May 17, 2023

RZ10012 warning and MSBuild 17.6 dotnet/aspnetcore#48281

Closed

1 task

jjonescz mentioned this pull request May 19, 2023

[17.6] Components don't see other components for inheritance or nested types #8739

Closed

jjonescz added a commit to jjonescz/razor that referenced this pull request May 19, 2023

Revert "Perf/generator (dotnet#8212)"

9f101f7

This reverts commit 4ab512e.

jjonescz added a commit to jjonescz/razor that referenced this pull request May 19, 2023

Revert "Perf/generator (dotnet#8212)"

0783593

This reverts commit 4ab512e.

jaredpar pushed a commit that referenced this pull request May 22, 2023

[17.6] Revert "Perf/generator (#8212)" (#8742)

a6a61fd

* Revert "Perf/generator (#8212)" This reverts commit 4ab512e. * Add more tests

333fred added a commit that referenced this pull request May 22, 2023

Merge remote-tracking branch 'upstream/release/dev17.6' into merges/r…

350af0c

…elease/dev17.6-to-release/dev17.7 * upstream/release/dev17.6: [17.6] Revert "Perf/generator (#8212)" (#8742)

333fred added a commit that referenced this pull request May 30, 2023

Merge remote-tracking branch 'upstream/release/dev17.7' into merges/r…

c452eeb

…elease/dev17.7-to-main * upstream/release/dev17.7: Update baseline. [17.6] Revert "Perf/generator (#8212)" (#8742)

333fred added a commit that referenced this pull request May 31, 2023

Merge remote-tracking branch 'upstream/release/dev17.7' into merges/r…

a3c6a1d

…elease/dev17.7-to-main * upstream/release/dev17.7: Update baseline. [17.6] Revert "Perf/generator (#8212)" (#8742)

chsienki mentioned this pull request Jul 20, 2023

Restore perf work. #8995

Merged

Perf/generator #8212

Perf/generator #8212

Uh oh!

Conversation

chsienki commented Feb 2, 2023

Uh oh!

chsienki Feb 3, 2023

Choose a reason for hiding this comment

Uh oh!

chsienki Feb 3, 2023

Choose a reason for hiding this comment

Uh oh!

chsienki Feb 3, 2023

Choose a reason for hiding this comment

Uh oh!

chsienki Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chsienki Feb 3, 2023

Choose a reason for hiding this comment

Uh oh!

chsienki commented Feb 3, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jjonescz Feb 3, 2023

Choose a reason for hiding this comment

Uh oh!

chsienki Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chsienki commented Mar 28, 2023

Uh oh!

chsienki commented Mar 28, 2023

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chsienki Feb 3, 2023 •

edited

Loading

chsienki Feb 3, 2023 •

edited

Loading