Reduce allocations in Razor's DirectiveVisitor #10537
Conversation
Noticed in customer trace, ~35% of allocations over a nearly two minute period are in razor's sourcegenerator's call to the TagHelperDirectiveVisitor. As the DirectiveVisitor construction and usage are scoped to the calling method (DefaultRazorTagHelperContextDiscoveryPhase.ExecuteCore), it's easy enough to make the DirectiveVisitor disposable and to make the DirectiveVisitor use a pooled array for it's TagHelperDescriptor array.
|
@ToddGrun since this is a compiler change it will need 2 sign offs from that team prior to merging. Mine does not count towards that |
| @@ -69,6 +69,8 @@ protected override void ExecuteCore(RazorCodeDocument codeDocument) | |||
| var context = TagHelperDocumentContext.Create(tagHelperPrefix, matches.ToImmutableArray()); | |||
| codeDocument.SetTagHelperContext(context); | |||
| codeDocument.SetPreTagHelperSyntaxTree(syntaxTree); | |||
|
|
|||
| visitor.Dispose(); | |||
There was a problem hiding this comment.
Consider using (visitor) { on line 55-ish, and ending it here, so its clearer to everyone that its being disposed, and so that there is a try..finally in place
There was a problem hiding this comment.
I can change to that, I didn't go that route initially as it didn't fall directly into the using pattern and it's really not a big deal if the disposal doesn't end up happening.
There was a problem hiding this comment.
and it's really not a big deal if the disposal doesn't end up happening
Is it not? I would have thought that would mean the pool allocates lots of new builders, defeating the purpose of the PR.
There was a problem hiding this comment.
Let me rephrase, it's not a big deal if it's an exceptional case where this occurs, not if it's the normal code path (which would probably indicate bigger problems than avoiding these allocations)
| } | ||
|
|
||
| internal sealed class TagHelperDirectiveVisitor : DirectiveVisitor | ||
| { | ||
| private readonly List<TagHelperDescriptor> _tagHelpers; | ||
| private readonly PooledObject<ImmutableArray<TagHelperDescriptor>.Builder> _tagHelpers; |
There was a problem hiding this comment.
Random thought: Is ImmutableArray the best choice here? Since this builder is never realized, a pooled list might be just as suitable, but a bit simpler, especially if an initial capacity can be set?
Looks like ImmutableArray<>.Builder just resizes an array anyway, so I can't imagine List would be any worse, but I don't really have any knowledge here.
There was a problem hiding this comment.
Or even just a PooledArrayBuilder<T>?
There was a problem hiding this comment.
I tried PooledArrayBuilder initially, and it didn't seem to work because I was trying to store that into a field, and that type is defined as a NonCopyable struct.
There was a problem hiding this comment.
I didn't go with PooledList as I talked to Andrew and we weren't sure of it's benefit. It's a struct, but not marked as non-copyable, so it could be used, but it doesn't seem to be used a lot throughout the codebase. I'm flexible, and can go with that (or whatever else works) if you'd prefer.
There was a problem hiding this comment.
Yeah, PooledList can't be used, but we also have ListPool which can. Isn't coding fun?!
(sorry, my bad when i said "pooled list" and wasn't specific. I knew we had one that was a ref struct, but couldn't remember which)
There was a problem hiding this comment.
Yeah, this feels cleaner. Thanks for the suggestions!
…10602) This is some follow-up work after a discussion with @ToddGrun. Previously, @ToddGrun made [a change](#10537) to use pooled lists within the `DirectiveVisistors` used by `DefaultRazorTagHelperContextDiscoveryPhase`. However, looking at further traces, there is still a ton of CPU-bound work. Notably, there are loads of string comparisons that happen over and over to compare strings for assembly names. To address this, I've made the following changes: 1. Instead of using pooled lists, the `DirectiveVisitors` themselves are pooled and they own their data structures. 2. `TagHelperDirectiveVisitor` has been updated to store a `Dictionary<string, List<TagHelperDescriptor>>` rather than a flat `List<TagHelperDescriptor>`. This is used to index tag helpers by assembly name, which greatly reduces the number of string comparisons. The `List<TagHelperDescriptor>` held stored in the dictionary are pooled to avoid extra allocations of tag helpers per assembly.
Noticed in customer trace, ~35% of allocations over a nearly two minute period are in razor's sourcegenerator's call to the TagHelperDirectiveVisitor.
As the DirectiveVisitor construction and usage are scoped to the calling method (DefaultRazorTagHelperContextDiscoveryPhase.ExecuteCore), it's easy enough to make the DirectiveVisitor disposable and to make the DirectiveVisitor use a pooled array for it's TagHelperDescriptor array.
*** from the customer trace ***
