Compiler: Make intermediate nodes a bit more efficient#11931
Merged
Conversation
- Remove DebuggerDisplayFormatter - Merge IntermediateNodeFormatter and IntermediateNodeFormatterBase - Seal IntermediateNodeFormatter - Replace TextWriter with StringBuilder - Use pooled StringBuilder to create debugger display strings in IntermediateNode - Cache the short name for each type in a static dictionary - Use Debug.Asserts rather than throwing ArgumentNullException - Avoid allocating extra intermediate strings when escaping '\r', '\n', and '\t'. - Avoid using LINQ - Don't use string.Join(...) to create intermediate string to append to StringBuilder - Add nullability annotations - Other small bits of clean up
- Add nullability annotations - Use expression-bodied members and compound assignment for Annotations and Diagnostics properties - Use nameof in [DebuggerDisplay] - Rename DebuggerToString() to GetDebuggerDisplay()
- Make IntermediateNode.Diagnostics return an ImmutableArray<RazorDiagnostic> rather than exposing a lazily created RazorDiagnosticCollection. - Add IntermediateNode.AddDiagnostic(...) method to add a diagnostic. - Add IntedmediateNode.AddDiagnosticsFromNode(...) to add all diagnostics from another IntermediateNode. - Cache the ImmutableArray<RazorDiagnostic> in a field rather than recomputing it each time from _diagnosticsBuilder.
This type is no longer used and can be removed.
ToddGrun
reviewed
Jun 10, 2025
ToddGrun
reviewed
Jun 10, 2025
|
This looks amazing to me |
- IntermediateNodeFormatter: Move const to the method where it is used. - ComponentBindLoweringPass: Add missing type pattern to switch expression. - ComponentMarkupDiagnosticPass: Replace expressions with existing local.
chsienki
approved these changes
Jun 11, 2025
333fred
approved these changes
Jun 11, 2025
DustinCampbell
added a commit
that referenced
this pull request
Jun 13, 2025
`ItemCollection` is essentially a property bag that results in dictionaries being used instead of plain ol' fields, and it's been overused in Razor. I've been trying to get rid of it for nearly a year. #10720, #10764, #11360, and #11931 each represent changes that remove a use of `ItemCollection`. This pull request finally gets rid of `ItemCollection` completely. The last use of `ItemCollection` is the `RazorCodeDocument.Items`, which is used by the compiler to store various objects during compilation, and tooling uses it to cache some expensive-to-create data. This change stores all of the compiler objects on `RazorCodeDocument` directly. The data cached by tooling is stored on the side in a `ConditionalWeakTable`. As part of this, I've refactoring the `RazorCodeDocument.TryComputeNamespace` extension method to be more understandable and maintainable. ---- CI Build: https://dev.azure.com/dnceng/internal/_build/results?buildId=2728842&view=results Test Insertion: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/643246 Toolset Run: https://dev.azure.com/dnceng/internal/_build/results?buildId=2728843&view=results
This was referenced Jun 14, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request focuses on two properties of
IntermediateNode: Diagnostics and Annotations.IntermediateNode.Diagnosticsis constructed most of the time, even when there aren't any diagnostics. I've reworked how diagnostics are stored and collected to avoid creating any storage until a diagnostic is actually added. With this change, theRazorDiagnosticCollectiontype can be removed entirely.IntermediateNode.Annotationsworks like a property bag that is backed by anItemCollection. This is pretty inefficient. First of all,ItemCollectionuses aConcurrentDictionaryunder the hood, which is completely unnecessary in the compiler's synchronous code paths. Plus, all of the data stored inAnnotations(some of which are effectively bools) can just be stored as properties on the strongly-typedIntermediateNodetypes themselves.I recommend reviewing commit-by-commit, as I tried to keep each separate and cohesive.
CI Build: https://dev.azure.com/dnceng/internal/_build/results?buildId=2728082&view=results
Test Insertion: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/642908
Toolset Run: https://dev.azure.com/dnceng/internal/_build/results?buildId=2728084&view=results