Switch ItemBucket to be a struct #12148
Open
+74
−17
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
Switches ItemBucket from a heap-allocated class to a value-type struct (with IComparable<T>) to eliminate boxing and reduce allocations, adds a .NET-Framework List<T>.EnsureCapacity extension to minimize list resizes, and optimizes BucketConsumedItems to preallocate and reorder in place.
- Added
EnsureCapacity<T>extension inUtilities.csfor .NET Framework. - Converted
ItemBucketto a struct and updated its comparison API. - Updated
BatchingEngineto useEnsureCapacityand perform in-place bucket ordering.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Build/Utilities/Utilities.cs | Introduce EnsureCapacity<T> extension method for lists on non-.NET platforms. |
| src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs | Removed null check for batchBucket now that it's a struct. |
| src/Build/BackEnd/Components/RequestBuilder/ItemBucket.cs | Change ItemBucket from class to struct and implement IComparable<ItemBucket>. |
| src/Build/BackEnd/Components/RequestBuilder/BatchingEngine.cs | Use EnsureCapacity, remove temporary list allocation, and implement in-place bucket swap. |
Comments suppressed due to low confidence (3)
src/Build/Utilities/Utilities.cs:606
- The summary suggests multiple doublings, but the implementation only doubles once then falls back to the requested capacity. Please clarify the doc to match the code behavior. For example, "the capacity is doubled once (or set to
capacityif still smaller)".
/// the capacity is increased by continuously twice current capacity until it is at least the specified <paramref name="capacity"/>.
src/Build/BackEnd/Components/RequestBuilder/BatchingEngine.cs:372
- [nitpick] This in-place swapping algorithm is nontrivial; consider adding a unit test to verify that buckets end up correctly ordered for various initial sequences and edge cases.
for (int i = 0; i < buckets.Count;)
src/Build/BackEnd/Components/RequestBuilder/BatchingEngine.cs:11
- To bring the
EnsureCapacity<T>extension into scope, you may need to add the namespace that containsUtilities(e.g.using Microsoft.Build.Utilities;). Otherwise the extension method won't be discovered.
using Microsoft.Build.Internal;
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #
Context
The
ItemBuckettype is currently a class that gets allocated fairly frequently, added to collections, and passed around. Otherwise, the class is fairly a straightforward container of a handful of items. Switching it to a struct would avoid the heap allocations here.Before:
After:
Some things to note about the changes:
I needed to switch to having
ItemBucketimplement the genericIComparable<T>to avoid boxing the struct.A class is the size of a pointer and structs can vary in size depending on how many fields are in the struct. This is why there is an increase in the
ItemBucket[]allocations that back theList<ItemBucket>.To reduce the number of
ItemBucket[]allocations, I reduced the number of times we need to resize by utilizingEnsureCapacity(). While newer .NET code has this, I had to create an extension function for Framework.There was an additional
List<ItemBucket>that was being allocated just to sort the buckets inBucketSequenceNumber. We can do this in place to avoid the allocation.Changes Made
Testing
Notes