Skip to content
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

Modify ContentItemCollection.PopulateItemGroups to use pooling for highly allocated temporary data structures #6098

Merged
merged 3 commits into from
Oct 30, 2024

Conversation

ToddGrun
Copy link
Contributor

@ToddGrun ToddGrun commented Oct 12, 2024

Bug

Fixes: NuGet/Home#13851

Description

Reduce allocations in ContentItemCollection.PopulateItemGroups by using pooling

PR Checklist

  • Meaningful title, helpful description and a linked NuGet/Home issue
  • Added tests
  • Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc.

@ToddGrun ToddGrun requested a review from a team as a code owner October 12, 2024 01:49
@microsoft-github-policy-service microsoft-github-policy-service bot added the Community PRs created by someone not in the NuGet team label Oct 12, 2024
@ToddGrun
Copy link
Contributor Author

ToddGrun commented Oct 12, 2024

Highlighted allocations the ones that are removed due to this change. Took two profiles of the before to verify it wasn't just a bad run:

image

image

@ToddGrun
Copy link
Contributor Author

A couple thoughts:

  1. If this codepath is going to soon be removed, let's just abandon this. Not executing code is the best optimization possible.
  2. If the lock usage in SimplePool is a concern, it might be good to consider using something like Roslyn's ObjectPool.
  3. If the possible increase in total memory footprint due to pooling is a concern (it hasn't been flagged as such in other VS codebases I've played in), then an option would be to clear out the pools once the work needing to use these objects is completed. Not a path I would like to go down, but thought I would call it out when listing potential concerns I could think of with this PR.

@jeffkl jeffkl self-assigned this Oct 22, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Oct 30, 2024

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 90 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

@jeffkl jeffkl removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Oct 30, 2024
@jeffkl jeffkl merged commit 587f4d5 into NuGet:dev Oct 30, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community PRs created by someone not in the NuGet team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ContentItemCollection.PopulateItemGroups unnecessarily allocates
3 participants