-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Reduce allocations in ProjectItemInstance.CommonConstructor by settin… #12978
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
base: main
Are you sure you want to change the base?
Reduce allocations in ProjectItemInstance.CommonConstructor by settin… #12978
Conversation
…g right capacity for list
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 optimizes memory allocation in ProjectItemInstance.CommonConstructor by pre-sizing a list to avoid potential resizing operations. The issue identified through PRISM telemetry showed that when building the inheritedItemDefinitions list, the original code would create a list with exact capacity matching the input collection size, then potentially trigger a resize when adding one more item.
Key changes:
- Pre-allocate list capacity with
Count + 1to accommodate the potential addition of a project-level item definition - Use
AddRangeto populate the list instead of the collection constructor - Expand the ternary operator into an explicit if-else structure for clarity
| } | ||
| else | ||
| { | ||
| List<ProjectItemDefinitionInstance> list = new List<ProjectItemDefinitionInstance>(itemDefinitions.Count + 1); // account for possible addition below to avoid resizing |
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.
There is no need to have this list variable.
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.
I think AI agent used a list variable to be able to call AddRange(), which is not available on inheritedItemDefinitions which is IList.
I agree it can be done without the variable by casting since we just created it as a List. Were you suggesting something like this?
inheritedItemDefinitions = (itemDefinitions == null) ? null : new List<ProjectItemDefinitionInstance>(itemDefinitions.Count + 1);
((List<ProjectItemDefinitionInstance>)inheritedItemDefinitions)?.AddRange(itemDefinitions);
This pull request was generated by the VS Perf Rel AI Agent. Please review this AI-generated PR with extra care! For more information, visit our wiki. Please share feedback with TIP Insights
Microsoft.Build.Execution.ProjectItemInstance.CommonConstructor. InCommonConstructor,inheritedItemDefinitionsis built asnew List<ProjectItemDefinitionInstance>(itemDefinitions)(capacity ==itemDefinitions.Count) and theninheritedItemDefinitions.Add(itemDefinition)is called when the project-level item definition exists. This could lead to a list resize as seen in stack trace.List.Add -> EnsureCapacity -> set_Capacityframes seen in the stack trace that lead toTypeAllocated!Microsoft.Build.Execution.ProjectItemDefinitionInstance[].Best practices wiki
See related failure in PRISM
ADO work item