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.

Sign up for GitHub

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

Jump to bottom

Unroll Linq in GetFlags #5555

Merged
merged 2 commits into from
Jan 30, 2024
Merged

Unroll Linq in GetFlags #5555

merged 2 commits into from
Jan 30, 2024

Conversation

Erarndt
Copy link
Contributor

@Erarndt Erarndt commented Dec 20, 2023

Bug

Related: NuGet/Home#12748

Regression? Last working version:

Description

Avoids some allocations due to LINQ usage. Also creates a new list that is sorted in place rather than using OrderBy, which internally allocates a buffer for all the items, and then calling ToList() which allocates a list again.

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 December 20, 2023 21:55
@ghost ghost added the Community PRs created by someone not in the NuGet team label Dec 20, 2023
davkean
davkean reviewed Dec 21, 2023
View reviewed changes
src/NuGet.Core/NuGet.Packaging/NuspecReader.cs Outdated
// PERF: Avoid Linq on hot paths
var splitFlags = flags.Split(CommaArray, StringSplitOptions.RemoveEmptyEntries);
var set = new HashSet<string>(splitFlags.Length, StringComparer.OrdinalIgnoreCase);
for (int i = 0; i < splitFlags.Length; ++i)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can just be a foreach as C# will unroll under the covers to this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are compile errors, so maybe this can be addressed in the same push @Erarndt

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

@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 Dec 29, 2023
@ghost
Copy link

ghost commented Dec 29, 2023

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.

@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 Dec 30, 2023
zivkan
zivkan previously approved these changes Dec 30, 2023
View reviewed changes
nkolev92
nkolev92 reviewed Jan 2, 2024
View reviewed changes
Copy link
Member

@nkolev92 nkolev92 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's compile errors.

But the general change looks good to me.

Please create a matching issue for these changes.

src/NuGet.Core/NuGet.Packaging/NuspecReader.cs Outdated
// PERF: Avoid Linq on hot paths
var splitFlags = flags.Split(CommaArray, StringSplitOptions.RemoveEmptyEntries);
var set = new HashSet<string>(splitFlags.Length, StringComparer.OrdinalIgnoreCase);
for (int i = 0; i < splitFlags.Length; ++i)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are compile errors, so maybe this can be addressed in the same push @Erarndt

@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 16, 2024
@ghost
Copy link

ghost commented Jan 16, 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:08
@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
@Erarndt Erarndt requested a review from jeffkl January 29, 2024 21:08
@jeffkl jeffkl merged commit f93c401 into NuGet:dev Jan 30, 2024
16 checks passed
@Erarndt Erarndt deleted the dev-erarndt-GetFlags branch January 30, 2024 19:53
@Saibamen
Copy link

@Erarndt: Do you have benchmark results?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davkean davkean davkean left review comments

@jeffkl jeffkl jeffkl approved these changes

@zivkan zivkan zivkan left review comments

@nkolev92 nkolev92 Awaiting requested review from nkolev92

Assignees

@zivkan zivkan

Labels
Community PRs created by someone not in the NuGet team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Erarndt @Saibamen @davkean @nkolev92 @zivkan @jeffkl @martinrrm