Skip to content

Commit a2c7019

Browse files
DustinCampbellDustinCampbell
authored
Compiler: Reduce allocations and do less work when visiting intermediate nodes (#12005)
This represents a handful of changes that improve the efficiency of `IntermediateNodeWalker` and extension method helpers that visit intermediate node trees. The key commits are below. The remaining commits represent updates that react to the changes in these commits and general clean up. Reviewing commit-by-commit should be pretty straighforward. - Improve `IntermediateNodeWalker` efficiency (cbc5cc2) - Improve `FindDescendantReferences` extension method (f6812e8) - Improve `FindDirectiveReferences` extension method (7287162) - Improve `FindDirectiveNodes` extension method (5604593) ---- CI Build: https://dev.azure.com/dnceng/internal/_build/results?buildId=2741969&view=results Test Insertion: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/648672 Toolset Run: https://dev.azure.com/dnceng/internal/_build/results?buildId=2741970&view=results
2 parents dc47633 + f844d3c commit a2c7019

File tree

37 files changed

+494
-439
lines changed

37 files changed

+494
-439
lines changed

‎src/Compiler/Microsoft.AspNetCore.Razor.Language/test/Components/ComponentWhitespacePassTest.cs

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,8 @@ public void Execute_RemovesLeadingAndTrailingWhitespace()
5454

5555
// Assert
5656
var method = documentNode.FindPrimaryMethod();
57+
Assert.NotNull(method);
58+
5759
var child = Assert.IsType<MarkupElementIntermediateNode>(Assert.Single(method.Children));
5860
Assert.Equal("span", child.TagName);
5961
}
@@ -74,7 +76,10 @@ public void Execute_RemovesLeadingAndTrailingWhitespaceInsideElement()
7476
Pass.Execute(document, documentNode);
7577

7678
// Assert
77-
var parentElement = Assert.IsType<MarkupElementIntermediateNode>(Assert.Single(documentNode.FindPrimaryMethod().Children));
79+
var method = documentNode.FindPrimaryMethod();
80+
Assert.NotNull(method);
81+
82+
var parentElement = Assert.IsType<MarkupElementIntermediateNode>(Assert.Single(method.Children));
7883
var childElement = Assert.IsType<MarkupElementIntermediateNode>(Assert.Single(parentElement.Children));
7984
Assert.Equal("child", childElement.TagName);
8085
Assert.Collection(childElement.Children,
@@ -102,7 +107,10 @@ public void Execute_LeavesWhitespaceBetweenSiblingElements()
102107
Pass.Execute(document, documentNode);
103108

104109
// Assert
105-
Assert.Collection(documentNode.FindPrimaryMethod().Children,
110+
var method = documentNode.FindPrimaryMethod();
111+
Assert.NotNull(method);
112+
113+
Assert.Collection(method.Children,
106114
node => Assert.IsType<MarkupElementIntermediateNode>(node),
107115
node => Assert.IsType<HtmlContentIntermediateNode>(node),
108116
node => Assert.IsType<MarkupElementIntermediateNode>(node));
@@ -128,7 +136,10 @@ public void Execute_RemovesWhitespacePrecedingAndTrailingCSharpCode()
128136
Pass.Execute(document, documentNode);
129137

130138
// Assert
131-
var parentElement = Assert.IsType<MarkupElementIntermediateNode>(Assert.Single(documentNode.FindPrimaryMethod().Children));
139+
var method = documentNode.FindPrimaryMethod();
140+
Assert.NotNull(method);
141+
142+
var parentElement = Assert.IsType<MarkupElementIntermediateNode>(Assert.Single(method.Children));
132143
Assert.Collection(parentElement.Children,
133144
node =>
134145
{

‎src/Compiler/Microsoft.AspNetCore.Razor.Language/test/Extensions/DefaultTagHelperOptimizationPassTest.cs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,8 @@ public void DefaultTagHelperOptimizationPass_Execute_ReplacesChildren()
4444
var documentNode = processor.GetDocumentNode();
4545

4646
var @class = documentNode.FindPrimaryClass();
47+
Assert.NotNull(@class);
48+
4749
Assert.IsType<DefaultTagHelperRuntimeIntermediateNode>(@class.Children[0]);
4850

4951
var fieldDeclaration = Assert.IsType<FieldDeclarationIntermediateNode>(@class.Children[1]);

‎src/Compiler/Microsoft.AspNetCore.Razor.Language/test/Intermediate/IntermediateNodeWalkerTest.cs

Lines changed: 44 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,6 @@
11
// Licensed to the .NET Foundation under one or more agreements.
22
// The .NET Foundation licenses this file to you under the MIT license.
33

4-
#nullable disable
5-
64
using System;
75
using System.Collections.Generic;
86
using System.Linq;
@@ -20,21 +18,24 @@ public void IntermediateNodeWalker_Visit_TraversesEntireGraph()
2018

2119
var nodes = new IntermediateNode[]
2220
{
23-
new BasicIntermediateNode("Root"),
24-
new BasicIntermediateNode("Root->A"),
25-
new BasicIntermediateNode("Root->B"),
26-
new BasicIntermediateNode("Root->B->1"),
27-
new BasicIntermediateNode("Root->B->2"),
28-
new BasicIntermediateNode("Root->C"),
21+
new BasicIntermediateNode("Root"),
22+
new BasicIntermediateNode("Root->A"),
23+
new BasicIntermediateNode("Root->B"),
24+
new BasicIntermediateNode("Root->B->1"),
25+
new BasicIntermediateNode("Root->B->2"),
26+
new BasicIntermediateNode("Root->C"),
2927
};
3028

3129
var builder = new DefaultRazorIntermediateNodeBuilder();
30+
3231
builder.Push(nodes[0]);
3332
builder.Add(nodes[1]);
33+
3434
builder.Push(nodes[2]);
3535
builder.Add(nodes[3]);
3636
builder.Add(nodes[4]);
3737
builder.Pop();
38+
3839
builder.Add(nodes[5]);
3940

4041
var root = builder.Pop();
@@ -43,7 +44,7 @@ public void IntermediateNodeWalker_Visit_TraversesEntireGraph()
4344
walker.Visit(root);
4445

4546
// Assert
46-
Assert.Equal(nodes, walker.Visited.ToArray());
47+
Assert.Equal(nodes, walker.Visited);
4748
}
4849

4950
[Fact]
@@ -54,37 +55,43 @@ public void IntermediateNodeWalker_Visit_SetsParentAndAncestors()
5455

5556
var nodes = new IntermediateNode[]
5657
{
57-
new BasicIntermediateNode("Root"),
58-
new BasicIntermediateNode("Root->A"),
59-
new BasicIntermediateNode("Root->B"),
60-
new BasicIntermediateNode("Root->B->1"),
61-
new BasicIntermediateNode("Root->B->2"),
62-
new BasicIntermediateNode("Root->C"),
58+
new BasicIntermediateNode("Root"),
59+
new BasicIntermediateNode("Root->A"),
60+
new BasicIntermediateNode("Root->B"),
61+
new BasicIntermediateNode("Root->B->1"),
62+
new BasicIntermediateNode("Root->B->2"),
63+
new BasicIntermediateNode("Root->C"),
6364
};
6465

6566
var ancestors = new Dictionary<string, string[]>()
66-
{
67-
{ "Root", new string[]{ } },
68-
{ "Root->A", new string[] { "Root" } },
69-
{ "Root->B", new string[] { "Root" } },
70-
{ "Root->B->1", new string[] { "Root->B", "Root" } },
71-
{ "Root->B->2", new string[] { "Root->B", "Root" } },
72-
{ "Root->C", new string[] { "Root" } },
73-
};
74-
75-
walker.OnVisiting = (n) =>
7667
{
77-
Assert.Equal(ancestors[((BasicIntermediateNode)n).Name], walker.Ancestors.Cast<BasicIntermediateNode>().Select(b => b.Name));
78-
Assert.Equal(ancestors[((BasicIntermediateNode)n).Name].FirstOrDefault(), ((BasicIntermediateNode)walker.Parent)?.Name);
68+
{ "Root", [] },
69+
{ "Root->A", ["Root"] },
70+
{ "Root->B", ["Root"] },
71+
{ "Root->B->1", ["Root->B", "Root"] },
72+
{ "Root->B->2", ["Root->B", "Root"] },
73+
{ "Root->C", ["Root"] },
74+
};
75+
76+
walker.OnVisiting = (n, a) =>
77+
{
78+
var basicNode = Assert.IsType<BasicIntermediateNode>(n);
79+
var parent = a.Length > 0 ? (BasicIntermediateNode)a[0] : null;
80+
81+
Assert.Equal(ancestors[basicNode.Name], a.Cast<BasicIntermediateNode>().Select(b => b.Name));
82+
Assert.Equal(ancestors[basicNode.Name].FirstOrDefault(), parent?.Name);
7983
};
8084

8185
var builder = new DefaultRazorIntermediateNodeBuilder();
86+
8287
builder.Push(nodes[0]);
8388
builder.Add(nodes[1]);
89+
8490
builder.Push(nodes[2]);
8591
builder.Add(nodes[3]);
8692
builder.Add(nodes[4]);
8793
builder.Pop();
94+
8895
builder.Add(nodes[5]);
8996

9097
var root = builder.Pop();
@@ -93,49 +100,36 @@ public void IntermediateNodeWalker_Visit_SetsParentAndAncestors()
93100
walker.Visit(root);
94101
}
95102

96-
private class DerivedIntermediateNodeWalker : IntermediateNodeWalker
103+
private sealed class DerivedIntermediateNodeWalker : IntermediateNodeWalker
97104
{
98-
public new IReadOnlyList<IntermediateNode> Ancestors => base.Ancestors;
99-
100-
public new IntermediateNode Parent => base.Parent;
101-
102-
public List<IntermediateNode> Visited { get; } = new List<IntermediateNode>();
105+
public List<IntermediateNode> Visited { get; } = [];
103106

104-
public Action<IntermediateNode> OnVisiting { get; set; }
107+
public Action<IntermediateNode, IntermediateNode[]>? OnVisiting { get; set; }
105108

106109
public override void VisitDefault(IntermediateNode node)
107110
{
108111
Visited.Add(node);
109112

110-
OnVisiting?.Invoke(node);
113+
OnVisiting?.Invoke(node, Ancestors.ToArray());
111114
base.VisitDefault(node);
112115
}
113116

114-
public virtual void VisitBasic(BasicIntermediateNode node)
117+
public void VisitBasic(BasicIntermediateNode node)
115118
{
116119
VisitDefault(node);
117120
}
118121
}
119122

120-
private class BasicIntermediateNode : IntermediateNode
123+
private sealed class BasicIntermediateNode(string name) : IntermediateNode
121124
{
122-
public BasicIntermediateNode(string name)
123-
{
124-
Name = name;
125-
}
125+
public string Name { get; } = name;
126126

127-
public string Name { get; }
128-
129-
public override IntermediateNodeCollection Children { get; } = new IntermediateNodeCollection();
127+
public override IntermediateNodeCollection Children { get; } = [];
130128

131129
public override void Accept(IntermediateNodeVisitor visitor)
132-
{
133-
((DerivedIntermediateNodeWalker)visitor).VisitBasic(this);
134-
}
130+
=> ((DerivedIntermediateNodeWalker)visitor).VisitBasic(this);
135131

136132
public override string ToString()
137-
{
138-
return Name;
139-
}
133+
=> Name;
140134
}
141135
}

‎src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/Components/ComponentBindLoweringPass.cs

Lines changed: 16 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -42,17 +42,18 @@ protected override void ExecuteCore(RazorCodeDocument codeDocument, DocumentInte
4242
}
4343

4444
// For each @bind *usage* we need to rewrite the tag helper node to map to basic constructs.
45+
using var _ = ReferenceEqualityHashSetPool<IntermediateNode>.GetPooledObject(out var parents);
4546
var references = documentNode.FindDescendantReferences<TagHelperDirectiveAttributeIntermediateNode>();
4647
var parameterReferences = documentNode.FindDescendantReferences<TagHelperDirectiveAttributeParameterIntermediateNode>();
4748

48-
var parents = new HashSet<IntermediateNode>();
49-
for (var i = 0; i < references.Count; i++)
49+
foreach (var reference in references)
5050
{
51-
parents.Add(references[i].Parent);
51+
parents.Add(reference.Parent);
5252
}
53-
for (var i = 0; i < parameterReferences.Count; i++)
53+
54+
foreach (var parameterReference in parameterReferences)
5455
{
55-
parents.Add(parameterReferences[i].Parent);
56+
parents.Add(parameterReference.Parent);
5657
}
5758

5859
foreach (var parent in parents)
@@ -65,9 +66,9 @@ protected override void ExecuteCore(RazorCodeDocument codeDocument, DocumentInte
6566
// We don't have to worry about duplicate bound attributes in the same element
6667
// like, <Foo @bind="bar" @bind="bar" />, because IR lowering takes care of that.
6768
var bindEntries = new Dictionary<(IntermediateNode, string), BindEntry>();
68-
for (var i = 0; i < references.Count; i++)
69+
70+
foreach (var reference in references)
6971
{
70-
var reference = references[i];
7172
var parent = reference.Parent;
7273
var node = (TagHelperDirectiveAttributeIntermediateNode)reference.Node;
7374

@@ -85,11 +86,10 @@ protected override void ExecuteCore(RazorCodeDocument codeDocument, DocumentInte
8586

8687
// Do a pass to look for (@bind:get, @bind:set) pairs as this alternative form might have been used
8788
// to define the binding.
88-
for (var i = 0; i < parameterReferences.Count; i++)
89+
foreach (var parameterReference in parameterReferences)
8990
{
90-
var reference = parameterReferences[i];
91-
var parent = reference.Parent;
92-
var node = (TagHelperDirectiveAttributeParameterIntermediateNode)reference.Node;
91+
var parent = parameterReference.Parent;
92+
var node = (TagHelperDirectiveAttributeParameterIntermediateNode)parameterReference.Node;
9393

9494
if (!parent.Children.Contains(node))
9595
{
@@ -105,9 +105,9 @@ protected override void ExecuteCore(RazorCodeDocument codeDocument, DocumentInte
105105
node.Source,
106106
node.AttributeName));
107107
}
108-
if (!bindEntries.TryGetValue((reference.Parent, node.AttributeNameWithoutParameter), out var existingEntry))
108+
if (!bindEntries.TryGetValue((parameterReference.Parent, node.AttributeNameWithoutParameter), out var existingEntry))
109109
{
110-
bindEntries[(reference.Parent, node.AttributeNameWithoutParameter)] = new BindEntry(reference);
110+
bindEntries[(parameterReference.Parent, node.AttributeNameWithoutParameter)] = new BindEntry(parameterReference);
111111
}
112112
else
113113
{
@@ -119,9 +119,8 @@ protected override void ExecuteCore(RazorCodeDocument codeDocument, DocumentInte
119119
}
120120

121121
// Now collect all the parameterized attributes and store them along with their corresponding @bind or @bind-* attributes.
122-
for (var i = 0; i < parameterReferences.Count; i++)
122+
foreach (var parameterReference in parameterReferences)
123123
{
124-
var parameterReference = parameterReferences[i];
125124
var parent = parameterReference.Parent;
126125
var node = (TagHelperDirectiveAttributeParameterIntermediateNode)parameterReference.Node;
127126

@@ -1098,12 +1097,12 @@ private void RewriteNodesForElementEventCallbackBind(
10981097
private static IntermediateToken GetAttributeContent(IntermediateNode node)
10991098
{
11001099
var nodes = node.FindDescendantNodes<TemplateIntermediateNode>();
1101-
var template = nodes.Count > 0 ? nodes[0] : default;
1100+
var template = nodes.Length > 0 ? nodes[0] : default;
11021101
if (template != null)
11031102
{
11041103
// See comments in TemplateDiagnosticPass
11051104
node.AddDiagnostic(ComponentDiagnosticFactory.Create_TemplateInvalidLocation(template.Source));
1106-
return new IntermediateToken() { Kind = TokenKind.CSharp, Content = string.Empty, };
1105+
return new IntermediateToken() { Kind = TokenKind.CSharp, Content = string.Empty };
11071106
}
11081107

11091108
if (node.Children[0] is HtmlContentIntermediateNode htmlContentNode)

‎src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/Components/ComponentChildContentDiagnosticPass.cs

Lines changed: 17 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,6 @@
11
// Licensed to the .NET Foundation under one or more agreements.
22
// The .NET Foundation licenses this file to you under the MIT license.
33

4-
#nullable disable
5-
6-
using System;
74
using Microsoft.AspNetCore.Razor.Language.Intermediate;
85

96
namespace Microsoft.AspNetCore.Razor.Language.Components;
@@ -25,7 +22,7 @@ protected override void ExecuteCore(RazorCodeDocument codeDocument, DocumentInte
2522
visitor.Visit(documentNode);
2623
}
2724

28-
private class Visitor : IntermediateNodeWalker
25+
private sealed class Visitor : IntermediateNodeWalker
2926
{
3027
public override void VisitComponent(ComponentIntermediateNode node)
3128
{
@@ -43,7 +40,7 @@ public override void VisitComponent(ComponentIntermediateNode node)
4340
}
4441
}
4542

46-
base.VisitDefault(node);
43+
VisitDefault(node);
4744
}
4845

4946
public override void VisitComponentChildContent(ComponentChildContentIntermediateNode node)
@@ -52,26 +49,31 @@ public override void VisitComponentChildContent(ComponentChildContentIntermediat
5249
// because the parameter name can be implicit, and it doesn't work well when nested.
5350
if (node.IsParameterized)
5451
{
55-
for (var i = 0; i < Ancestors.Count - 1; i++)
52+
var ancestors = Ancestors;
53+
var parentComponent = (ComponentIntermediateNode)ancestors[0];
54+
55+
// Skip the immediate parent component as we've already validated against it.
56+
// Loop to ancestors.Length - 1 because we're always checking pairs.
57+
58+
for (var i = 1; i < ancestors.Length - 1; i++)
5659
{
57-
var ancestor = Ancestors[i] as ComponentChildContentIntermediateNode;
58-
if (ancestor != null &&
59-
ancestor.IsParameterized &&
60-
string.Equals(node.ParameterName, ancestor.ParameterName, StringComparison.Ordinal))
60+
if (ancestors[i] is ComponentChildContentIntermediateNode { IsParameterized: true } ancestor &&
61+
ancestor.ParameterName == node.ParameterName &&
62+
ancestors[i + 1] is ComponentIntermediateNode ancestorParentComponent)
6163
{
6264
// Duplicate name. We report an error because this will almost certainly also lead to an error
6365
// from the C# compiler that's way less clear.
6466
node.AddDiagnostic(ComponentDiagnosticFactory.Create_ChildContentRepeatedParameterName(
6567
node.Source,
66-
node,
67-
(ComponentIntermediateNode)Ancestors[0], // Enclosing component
68-
ancestor, // conflicting child content node
69-
(ComponentIntermediateNode)Ancestors[i + 1])); // Enclosing component of conflicting child content node
68+
childContent1: node,
69+
component1: parentComponent,
70+
childContent2: ancestor,
71+
component2: ancestorParentComponent));
7072
}
7173
}
7274
}
7375

74-
base.VisitDefault(node);
76+
VisitDefault(node);
7577
}
7678
}
7779
}

0 commit comments

Comments
 (0)