Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve performance of DefaultRazorTagHelperContextDiscoveryPhase #10602

Merged
merged 9 commits into from
Jul 16, 2024

Conversation

DustinCampbell
Copy link
Member

This is some follow-up work after a discussion with @ToddGrun. Previously, @ToddGrun made a change 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.

@DustinCampbell DustinCampbell requested review from a team as code owners July 9, 2024 23:28
@DustinCampbell
Copy link
Member Author

This is still in need of reviews from @dotnet/razor-compiler

@jjonescz jjonescz added the area-compiler Umbrella for all compiler issues label Jul 15, 2024
Copy link
Contributor

@chsienki chsienki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I don't love the nonComponentTagHelper prefixes in the visitor, as it confused me a bit at first that we were explicitly differentiating but not within the type, but I get where you're coming from.

Seems like another case of 'we need a better name than 'tag helper'' that we were talking about a while back :/

@jjonescz
Copy link
Member

we need a better name than 'tag helper'

legacyTagHelper, mvcTagHelper, cshtmlTagHelper...?

@DustinCampbell
Copy link
Member Author

we need a better name than 'tag helper'

legacyTagHelper, mvcTagHelper, cshtmlTagHelper...?

Y'all are free to consider this discussion outside of my pull request. I'm not going to name this here. 😄

@DustinCampbell
Copy link
Member Author

DustinCampbell commented Jul 15, 2024

Nit: I don't love the nonComponentTagHelper prefixes in the visitor, as it confused me a bit at first that we were explicitly differentiating but not within the type, but I get where you're coming from.

There are two visitors -- one that applies to "components" and one that applies to "tag helpers". Since both are really built on "tag helpers", it seemed to me that the real distinction is whether a tag helper represents a component or not. If you have better names, I'm definitely happy to use them.

@DustinCampbell DustinCampbell merged commit 8a97a5f into dotnet:main Jul 16, 2024
12 checks passed
@DustinCampbell DustinCampbell deleted the tag-helper-discovery-perf branch July 16, 2024 02:47
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Jul 16, 2024
@RikkiGibson RikkiGibson modified the milestones: Next, 17.12 Preview 1 Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-compiler Umbrella for all compiler issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants