Compiler: Clean up and reduce allocations in declaration intermediate nodes #12024

DustinCampbell · 2025-07-08T20:30:43Z

This pull request cleans up and reduces allocations in declaration-level intermediate nodes: NamespaceDeclarationIntermediateNode, ClassDeclarationIntermediateNode, MethodDeclarationIntermediateNode, FieldDeclarationIntermediateNode, PropertyDeclarationIntermediateNode, and related types. In particular, various collection properties are no longer types as IList<T> and pre-assigned with a List<T> instance. Instead, these properties now return ImmutableArray<T>.

Truth be told, this doesn't result in a large reduction in allocations, but this pull request paves the way for future changes.

CI Build: https://dev.azure.com/dnceng/internal/_build/results?buildId=2745495&view=results
Toolset Run: https://dev.azure.com/dnceng/internal/_build/results?buildId=2745496&view=results

ToddGrun · 2025-07-08T20:52:23Z

...ft.CodeAnalysis.Razor.Compiler/src/Language/Intermediate/ClassDeclarationIntermediateNode.cs

-        visitor.VisitClassDeclaration(this);
-    }
+    public void UpdateModifiers(params ImmutableArray<string> modifiers)
+        => _modifiers = modifiers.NullToEmpty();


modifiers.NullToEmpty();

Is a default IA possible here due to it being a params list?

Sure. Any caller can specifically pass "default" with no compiler warning.

ToddGrun · 2025-07-08T21:07:09Z

...er/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/CodeGeneration/CodeWriterExtensions.cs

-            for (var i = 0; i < typeParameters.Count; i++)
+            var first = true;
+
+            foreach (var typeParameter in typeParameters)


foreach (var typeParameter in typeParameters)

Could this use the has* technique used later in the method to not need the first local or check per loop iteration?

redacted. Ignore me. :)

Absolutely not! 😄

ToddGrun · 2025-07-08T21:08:29Z

...er/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/CodeGeneration/CodeWriterExtensions.cs

+                }
+                else
+                {
+                    writer.Write((string)typeParameter.ParameterName);


typeParameter.ParameterName

Dumb question: why is the cast needed?

It's definitely not. Good catch!

ToddGrun · 2025-07-08T21:23:11Z

...er/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/CodeGeneration/CodeWriterExtensions.cs

-        }
-
-        for (var i = 0; i < suppressWarnings.Count; i++)
+        for (var i = 0; i < suppressWarnings.Length; i++)


for

nit: could use foreach

ToddGrun · 2025-07-08T21:54:28Z

src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/RazorProjectEngine.cs

-            method.Modifiers.Add("public");
-            method.Modifiers.Add("async");
-            method.Modifiers.Add("override");
+            method.UpdateModifiers("public", "async", "override");


method.UpdateModifiers("public", "async", "override");

I think this is right, but most of the other places that called the Update* methods were replacing code that did a Clear potentially followed by some Adds. I'm not seeing a Clear in the replaced code here, but I assume it was going to be empty anyways,.

Yup, that's right.

ToddGrun

333fred · 2025-07-09T23:42:46Z

...ft.CodeAnalysis.Razor.Compiler/src/Language/Intermediate/ClassDeclarationIntermediateNode.cs

-    public string ClassName { get; set; }
+    public ImmutableArray<string> Modifiers
+    {
+        get => _modifiers;


Is field available here? If so, maybe we could use it for these properties?

333fred · 2025-07-09T23:44:15Z

...ft.CodeAnalysis.Razor.Compiler/src/Language/Intermediate/ClassDeclarationIntermediateNode.cs


-        visitor.VisitClassDeclaration(this);
-    }
+    public void UpdateModifiers(params ImmutableArray<string> modifiers)


What's the benefit to having these methods over just having a set instead of an init on the property? I lean towards just having the setter, even if that means not being able to use the params, simply to avoid the duplication of calling NullToEmpty multiple times.

Generally, I was trying to get away from the get; set; pattern used for intermediate nodes because the state of the data is super unclear. It's very hard to determine what data is or isn't supposed to be set. I'm happy to use field and do validation and NullToEmpty in the setters (and the initializer?), but I was hoping to make setters super rare in the intermediate nodes and use "Update" methods for properties that are supposed to be update.

- Enable nullability - Add constructor - Make `IsPrimaryNamespace` property read-only - Remove unnecessary argument null check - Lazily create `Children` collection - General clean up

- Enable nullability - Add constructor - Make `IsPrimaryClass` property read-only - Remove unnecessary argument null check - Lazily create `Children` collection - Change `Modifiers` property to return an `ImmutableArray<string>` rather than an `IList<string>` (backed by a `List<string>`). - Change `Interfaces` property to return an `ImmutableArray<IntermediateToken>` rather than an `IList<IntermediateToken>` (backed by a `List<IntermediateToken>`). - Change `TypeParameters` property to return an `ImmutableArray<TypeParameter>` rather than an `IList<TypeParameter>` (backed by a `List<TypeParameter>`). - General clean up

- Enable nullability - Add constructor - Make properties readonly

Because `CodeRenderingContext` owns the `CodeWriter` instance used for code gen, and the `BuildNamespace` extension method targets `CodeWriter` and also takes a `CodeRenderingContext`, we can change it can target `CodeRenderingContext`.

Because `CodeRenderingContext` owns the `CodeWriter` instance used for code gen, and the `BuildClassDeclaration` extension method targets `CodeWriter` and also takes a `CodeRenderingContext`, we can change it can target `CodeRenderingContext`.

- Enable nullability - Add constructor - Remove unnecessary argument null check - Change `Modifiers` property to return an `ImmutableArray<string>` rather than an `IList<string>` (backed by a `List<string>`). - Change `SupressWarnings` property to return an `ImmutableArray<string>` rather than an `IList<string>` (backed by a `List<string>`). - Make `FieldName`, `FieldType`, and `IsTagHelperField` properties read-only.

- Enable nullability - Add constructor - Remove unnecessary argument null check - Change `Modifiers` property to return an `ImmutableArray<string>` rather than an `IList<string>` (backed by a `List<string>`). - Make `PropertyName`, `ProeprtyType`, and `PropertyExpression` properties read-only.

…ngContext Because `CodeRenderingContext` owns the `CodeWriter` instance used for code gen, and the `WritePropertyDeclaration` and `WriteAutoPropertyDeclaration` extension methods target `CodeWriter` and also take a `CodeRenderingContext`, we can change them can target `CodeRenderingContext`.

…ntext Because `CodeRenderingContext` owns the `CodeWriter` instance used for code gen, and the `BuildLinePragma` and `BuildEnhancedLinePragma` extension methods target `CodeWriter` and also take a `CodeRenderingContext`, we can change them can target `CodeRenderingContext`. In addition, this change changes the type that's returned to readonly ref struct. This requires a bit of adjustment to some of the calling code.

Because `CodeRenderingContext` owns the `CodeWriter` instance used for code gen, and the `WritePadding` extension method targets `CodeWriter` and also takes a `CodeRenderingContext`, we can change it can target `CodeRenderingContext`.

- Enable nullability - Add constructors - Make `IsPrimaryMethod` property read-only - Remove unnecessary argument null check - Lazily create `Children` collection - Change `Modifiers` property to return an `ImmutableArray<string>` rather than an `IList<string>` (backed by a `List<string>`). - Change `Parmeters` property to return an `ImmutableArray<MethodParameter>` rather than an `IList<MethodParameter>` (backed by a `List<MethodParameter>`). - General clean up

- Enable nullability - Add constructors - Make properties read-only

DustinCampbell requested review from a team as code owners July 8, 2025 20:30

ToddGrun reviewed Jul 8, 2025

View reviewed changes

ToddGrun approved these changes Jul 8, 2025

View reviewed changes

333fred reviewed Jul 9, 2025

View reviewed changes

DustinCampbell changed the base branch from dev/dustinca/intermediate-node-visitors to main July 10, 2025 00:56

DustinCampbell added 12 commits July 9, 2025 17:58

Clean up NamespaceDeclarationIntermediateNode

9277fd6

- Enable nullability - Add constructor - Make `IsPrimaryNamespace` property read-only - Remove unnecessary argument null check - Lazily create `Children` collection - General clean up

Clean up TypeParameter

79d0edb

- Enable nullability - Add constructor - Make properties readonly

Clean up MethodParameter

7dc5c99

- Enable nullability - Add constructors - Make properties read-only

DustinCampbell force-pushed the declaration-intermediate-nodes branch from f20d86f to 7dc5c99 Compare July 10, 2025 00:59

DustinCampbell changed the title ~~Clean up and reduce allocations in declaration intermediate nodes~~ Compiler: Clean up and reduce allocations in declaration intermediate nodes Jul 10, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Compiler: Clean up and reduce allocations in declaration intermediate nodes #12024

Compiler: Clean up and reduce allocations in declaration intermediate nodes #12024

Uh oh!

DustinCampbell commented Jul 8, 2025 •

edited

Loading

Uh oh!

ToddGrun Jul 8, 2025

Uh oh!

DustinCampbell Jul 8, 2025 •

edited

Loading

Uh oh!

ToddGrun Jul 8, 2025

Uh oh!

ToddGrun Jul 8, 2025

Uh oh!

DustinCampbell Jul 9, 2025

Uh oh!

ToddGrun Jul 8, 2025

Uh oh!

DustinCampbell Jul 8, 2025

Uh oh!

ToddGrun Jul 8, 2025

Uh oh!

ToddGrun Jul 8, 2025

Uh oh!

DustinCampbell Jul 9, 2025

Uh oh!

ToddGrun left a comment

Uh oh!

333fred Jul 9, 2025

Uh oh!

333fred Jul 9, 2025

Uh oh!

DustinCampbell Jul 10, 2025 •

edited

Loading

Uh oh!

Uh oh!

Compiler: Clean up and reduce allocations in declaration intermediate nodes #12024

Are you sure you want to change the base?

Compiler: Clean up and reduce allocations in declaration intermediate nodes #12024

Uh oh!

Conversation

DustinCampbell commented Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DustinCampbell Jul 8, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ToddGrun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DustinCampbell Jul 10, 2025 • 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!

DustinCampbell commented Jul 8, 2025 •

edited

Loading

DustinCampbell Jul 8, 2025 •

edited

Loading

DustinCampbell Jul 10, 2025 •

edited

Loading