-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Reduce allocations due to resizing dictionaries #12159
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR aims to reduce allocations by pre-sizing dictionaries based on known usage patterns, cutting down on repeated resizes.
- Introduces
ItemTypesCountinItemDictionaryfor better initial capacity guesses. - Adds
InitializeCapacityinLookupto optionally pre-allocate the primary dictionary. - Calls
InitializeCapacityinTaskExecutionHostandBatchingEnginebefore batch item additions.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Build/Collections/ItemDictionary.cs | Added ItemTypesCount property to expose the count of item-type buckets for capacity hints. |
| src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs | Invoke InitializeCapacity(outputs.Length) on the lookup before adding task outputs. |
| src/Build/BackEnd/Components/RequestBuilder/Lookup.cs | Added InitializeCapacity(int capacity) method and optional capacity-based constructor overload. |
| src/Build/BackEnd/Components/RequestBuilder/BatchingEngine.cs | Use InitializeCapacity(items.Count) when inserting into existing buckets to pre-size them. |
Comments suppressed due to low confidence (3)
src/Build/Collections/ItemDictionary.cs:87
- Add XML documentation for the
ItemTypesCountproperty to describe what it represents and why it's needed.
public int ItemTypesCount
src/Build/BackEnd/Components/RequestBuilder/Lookup.cs:712
- [nitpick] The method name
InitializeCapacityis misleading since it only initializes when the table is null. Consider renaming toInitializeIfNullor changing its behavior to truly ensure a minimum capacity (e.g., viaDictionary.EnsureCapacity).
internal void InitializeCapacity(int capacity) => PrimaryAddTable ??= new ItemDictionary<ProjectItemInstance>(capacity);
src/Build/BackEnd/Components/RequestBuilder/Lookup.cs:712
- Add unit tests for
InitializeCapacityto verify it sets the initial capacity correctly and doesn't override existing table contents.
internal void InitializeCapacity(int capacity) => PrimaryAddTable ??= new ItemDictionary<ProjectItemInstance>(capacity);
surayya-MS
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thanks for the PR!
AR-May
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thank you. Just couple of questions.
This reverts commit 723c74e.
Fixes # ### Context The usage pattern of `Dictionary<TKey, TValue>` in `Lookup` can cause a large number of allocations due to resizing. The dictionaries are often created in paths that add items one at a time. We can use available context to make better guesses about the desired size to reduce the number of allocations. Before: <img width="1318" height="120" alt="image" src="https://github.com/user-attachments/assets/7656b202-7439-4e32-9c64-28d0bf738740" /> After: <img width="1166" height="294" alt="image" src="https://github.com/user-attachments/assets/6935a568-4837-4412-8662-0149b19f5fc1" /> Where the allocations happen shift around a bit, but the total allocations drop roughly 100MB ### Changes Made ### Testing ### Notes ---------
This reverts commit 723c74e.
Fixes #
Context
The usage pattern of
Dictionary<TKey, TValue>inLookupcan cause a large number of allocations due to resizing. The dictionaries are often created in paths that add items one at a time. We can use available context to make better guesses about the desired size to reduce the number of allocations.Before:

After:
Where the allocations happen shift around a bit, but the total allocations drop roughly 100MB
Changes Made
Testing
Notes