switch from newing up a ICollection ot a IReadOnlyCollection #12008
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
This PR aims to reduce unnecessary allocations by switching internal usages from returning mutable ICollection instances to returning IReadOnlyCollection where applicable, while preserving public API behavior. Key changes include updating interface signatures, modifying return types in various collections, and wrapping public API returns with ReadOnlyCollection where needed.
- Changed interface and method signatures from ICollection to IReadOnlyCollection where possible.
- Removed redundant ReadOnlyCollection wrappers in internal code.
- Adjusted unit tests and collection implementations to reflect new collection types.
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Build/Instance/ProjectInstance.cs | Updated explicit interface implementations to return IReadOnlyCollection and wrapped public API returns. |
| src/Build/Instance/ImmutableProjectCollections/ImmutableItemDictionary.cs | Changed indexer and method return types to IReadOnlyCollection. |
| src/Build/Evaluation/PropertyTrackingEvaluatorDataWrapper.cs | Modified GetItems signature for consistency with internal usage. |
| src/Build/Evaluation/LazyItemEvaluator.EvaluatorData.cs | Updated GetItems return type to IReadOnlyCollection. |
| src/Build/Evaluation/IItemProvider.cs | Adjusted interface to return IReadOnlyCollection. |
| src/Build/Definition/Project.cs | Wrapped internal IReadOnlyCollection with ReadOnlyCollection for public API compliance. |
| src/Build/Collections/ItemDictionary.cs | Changed indexer and GetItems method return types accordingly. |
| src/Build/Collections/IItemDictionary.cs | Updated interface to use IReadOnlyCollection. |
| src/Build/BackEnd/Components/RequestBuilder/Lookup.cs | Systematically updated GetItems and related methods to return IReadOnlyCollection. |
| src/Build/BackEnd/Components/RequestBuilder/ItemBucket.cs | Changed parameter types to reflect IReadOnlyCollection usage. |
| src/Build/BackEnd/Components/RequestBuilder/IntrinsicTasks/ItemGroupIntrinsicTask.cs | Updated GetItems and ModifyItems calls to use the new types, with one call using a spread operator. |
| src/Build/BackEnd/Components/RequestBuilder/BatchingEngine.cs | Modified dictionary and loop types to use IReadOnlyCollection. |
| Various Unit Tests | Updated collection references to use IReadOnlyCollection to match the method updates. |
| @@ -342,7 +342,7 @@ private void ExecuteModify(ProjectItemGroupTaskItemInstance child, ItemBucket bu | |||
| } | |||
|
|
|||
| // Now apply the changes. This must be done after filtering, since explicitly set metadata overrides filters. | |||
| bucket.Lookup.ModifyItems(child.ItemType, group, metadataToSet); | |||
| bucket.Lookup.ModifyItems(child.ItemType, [..group], metadataToSet); | |||
There was a problem hiding this comment.
The use of the spread operator '[..group]' here differs from other similar calls where 'group' is passed directly. Please clarify if a defensive copy is intended and consider making this usage consistent with the other ModifyItems calls.
Suggested change
| bucket.Lookup.ModifyItems(child.ItemType, [..group], metadataToSet); | |
| bucket.Lookup.ModifyItems(child.ItemType, group, metadataToSet); |
Copilot uses AI. Check for mistakes.
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.
Context
There were lots of allocations due to newing up a ReadOnlyCollection just to fufill a ICollection contract even if it was internal.
Changes Made
Testing
Took traces of build before my changes and after my changes.
Before (ReadOnlyCollection 33 Megs of allocations)
After (ReadOnlyCollection 5 Megs of allocations)
Notes