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

Further reduce allocations in CreateGraphNodeAsync #5593

Merged
merged 2 commits into from
Feb 1, 2024

Conversation

Erarndt
Copy link
Contributor

@Erarndt Erarndt commented Jan 12, 2024

Bug

Related: NuGet/Home#12748

Regression? Last working version:

Description

This PR makes additional changes to reduce allocations in CreateGraphNodeAsync() by addressing three issues highlighted in this trace:

image

A LightweightList was introduced in a previous PR to reduce the need to allocate a List<T> in the cases where the number of items was 4 or fewer. This value was chosen because it was seen as a safe starting point since it's the default size for List<T> as well as the 80th percentile in telemetry data

50 - 2,
70 - 3,
80 - 4
90 - 7
95 - 9

This trace indicates that there are still significant allocations occurring and this change allows LightweightList to contain up to 10 items before needing to allocate a List<T>.

The second change introduces a helper method on GraphNode that enables callers to ensure that InnerNodes has sufficient capacity to add additional items to minimize resizing.

Finally, FindRuntimeDependencies() often returns a List<T> wrapped by the IEnumerable<T> interface which was resulting in boxing of the List<T>.Enumerator. The existing NoAllocEnumerate() helper can be used here to avoid the boxing.

PR Checklist

  • PR has a meaningful title

  • PR has a linked issue.

  • Described changes

  • Tests

    • Automated tests added
    • OR
    • Test exception
    • OR
    • N/A
  • Documentation

    • Documentation PR or issue filled
    • OR
    • N/A

@Erarndt Erarndt requested a review from a team as a code owner January 12, 2024 20:22
@ghost ghost added the Community PRs created by someone not in the NuGet team label Jan 12, 2024
nkolev92
nkolev92 previously approved these changes Jan 17, 2024
@ghost ghost 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 Jan 24, 2024
@ghost
Copy link

ghost commented Jan 24, 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.

@Erarndt Erarndt requested a review from nkolev92 January 29, 2024 21:16
@ghost ghost 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 Jan 29, 2024
jeffkl
jeffkl previously approved these changes Jan 30, 2024
@jeffkl jeffkl self-assigned this Jan 30, 2024
@jeffkl jeffkl enabled auto-merge (squash) February 1, 2024 16:39
@jeffkl jeffkl merged commit 40ff51f into NuGet:dev Feb 1, 2024
15 of 16 checks passed
@Erarndt Erarndt deleted the dev-erarndt-lightweightList branch February 1, 2024 18:25
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
4 participants