Skip to content

Prevent large number of dictionary resizes #8796

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jun 7, 2023
Merged

Prevent large number of dictionary resizes #8796

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fb866b4
Preallocate a sufficiently large dictionary.
apilosofms Jun 2, 2023
5f600dd
Converted to IReadOnlyList throughout.
apilosofms Jun 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/Compiler/Microsoft.AspNetCore.Razor.Language/src/TagHelperBinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,12 @@ internal sealed class TagHelperBinder
/// <param name="descriptors">The descriptors that the <see cref="TagHelperBinder"/> will pull from.</param>
public TagHelperBinder(string tagHelperPrefix, IEnumerable<TagHelperDescriptor> descriptors)
{
// To prevent creating a dictionary that gets resized many times, we attempt to guess at a
// suitable capacity.
var descriptorCount = (descriptors is ICollection<TagHelperDescriptor> bounded) ? bounded.Count : descriptors.Count();
Copy link
Member

@davidwengier davidwengier Jun 2, 2023

Choose a reason for hiding this comment

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

I'll let the compiler team comment since they own the code, but I would probably just update the descriptors parameter to be IReadOnlyList<TagHelperDescriptor>, and roll that through wherever it needs to go in the codebase. This is an internal API, so I would be surprised if the change affected any public APIs.

From a quick look, I wouldn't expect the .Count() to actually be called outside of test code anyway, so maybe it doesn't matter. The only non-test code that calls this seems to always pass an array.

Copy link
Member

@davkean davkean Jun 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If you end up changing this, I would recommend changing to the underlying type (List<ItagHelperDescriptor>?) to avoid boxing the struct enumerator it provides when you foreach.

Copy link
Member

@DustinCampbell DustinCampbell Jun 2, 2023

Choose a reason for hiding this comment

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

+💯

Copy link
Member

@333fred 333fred Jun 2, 2023

Choose a reason for hiding this comment

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

@DustinCampbell, would this change interfere with any of your existing refactorings in this area?

Copy link
Member

@DustinCampbell DustinCampbell Jun 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Nope. Should be fine.

Copy link
Member

@333fred 333fred Jun 2, 2023

Choose a reason for hiding this comment

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

Ok, so I looked through the call stack here, and changing the type to something more concrete than IReadOnlyList would require breaking the public API (or adding an internal API version, preferably returning ImmutableArray). This is something we can do in the 17.7 timeframe, but to avoid dragging this PR on, I'd suggest simply going to IReadOnlyList for now and filing a followup issue to move this to ImmutableArray at a later point.

Copy link
Member

@DustinCampbell DustinCampbell Jun 7, 2023

Choose a reason for hiding this comment

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

Agreed. I took a look and came to the same conclusion. This change is better than doing nothing.

Copy link
Contributor Author

@apilosofms apilosofms Jun 7, 2023

Choose a reason for hiding this comment

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

@333fred, I changed to use IReadOnlyList as requested. It touches several files, but doesn't seem too scary.


_tagHelperPrefix = tagHelperPrefix;
_registrations = new Dictionary<string, HashSet<TagHelperDescriptor>>(StringComparer.OrdinalIgnoreCase);
_registrations = new Dictionary<string, HashSet<TagHelperDescriptor>>(descriptorCount, StringComparer.OrdinalIgnoreCase);

// Populate our registrations
foreach (var descriptor in descriptors)
Expand Down