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

Clean up TagHelperFactsService and friends #9199

Merged

Conversation

DustinCampbell
Copy link
Member

This change cleans up the tooling layer's TagHelperFactsService to consume and return ImmutableArray<T> rather than IReadOnlyList<T>. I've gone through all code using this service to ensure that any boxing is removed or eliminated. I also eliminated several other allocations (e.g. .ToList().AsReadOnly()).

@dotnet/razor-compiler: I made a couple of changes to compiler code to facilitate the shift over to ImmutableArray<T>:

  • The key change is in TagHelperParseTreeRewriter which previously held onto a List<KeyValuePair<string, string>>, which it handed out during tag helper binding before reusing the same collection later. In this change, it holds onto an ImmutableArray<KeyValuePair<string, string>>.Builder and returns a ImmutableArray<KeyValuePair<string, string>>. So, this represents a new allocation (for the underlying array) and copy that wasn't there before. However, I don't expect this will be significant and the code is simpler and a bit less surprising now.
  • I unrolled the lambdas in TagHelperMatchingConventions.SatsifiesAttributes(...). IMO, this makes the code a bit more readable and takes advantage of the fact that ImmutableArray<T> is now in play, so it can foreach.

In Shared.Utilities, I made a few tweaks that I hope will be useful:

  • PooledArrayBuilder<T> now has an enumerator.
  • Added a KeyValuePair.Create(...) helper for NetFx and netstandard2.0, similar to the one on core.
  • Added a HashSet<T>.AddRange(...) extension method to avoid boxing when passing an ImmutableArray<T>. Note: This is named differently that than HashSet<T>.UnionWith(...), which would normally be used to add multiple items to a hash set. Since UnionWith already exists as an instance method, the compiler won't consider similar extension methods of the same name when performing overload resolution.

@DustinCampbell DustinCampbell requested review from a team as code owners August 30, 2023 18:57
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

Compiler and Shared changes generally look to me. One question.

* Add Contains extension method for string that calls IndexOf.
* Don't mark builder and list parameters of enumerators as nullable.
@DustinCampbell DustinCampbell merged commit b0273c3 into dotnet:main Aug 31, 2023
@DustinCampbell DustinCampbell deleted the tag-helper-facts-service-clean-up branch August 31, 2023 18:29
@ghost ghost added this to the Next milestone Aug 31, 2023
@Cosifne Cosifne modified the milestones: Next, 17.8 P3 Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants