Prevent large number of dictionary resizes #8796
Conversation
davidwengier
left a comment
davidwengier
left a comment
There was a problem hiding this comment.
Some context for reviewers: Avi and I talked offline about this, and looked at a few razor.project.json files to determine what heuristic to use for the initial size. Even though the Register method can add N dictionary entries per tag helper descriptor, we settled on this being a good medium between avoiding an egregious number of resizes, and not pre-allocating too big a dictionary.
| { | ||
| // 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(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@DustinCampbell, would this change interfere with any of your existing refactorings in this area?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Agreed. I took a look and came to the same conclusion. This change is better than doing nothing.
There was a problem hiding this comment.
@333fred, I changed to use IReadOnlyList as requested. It touches several files, but doesn't seem too scary.
| { | ||
| // 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(); |
There was a problem hiding this comment.
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.
|
@dotnet/razor-compiler for a second review. |
5 participants
Summary of the changes
GCPauseWatson reports high GC pressure caused by frequent dictionary resizes in this code:
https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1824275
We attempt to reduce these by giving the dictionary a reasonable size to start with.
Fixes: