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

Remove unneeded allocations parsing assets file #4483

Merged
merged 2 commits into from Mar 7, 2022

Conversation

davkean
Copy link
Contributor

@davkean davkean commented Mar 4, 2022

Bug

Fixes: NuGet/Home#11648

Regression? Last working version:

Description

Remove unneeded allocations parsing assets file

PR Checklist

  • PR has a meaningful title

  • PR has a linked issue.

  • Described changes

  • Tests

    • Automated tests added
    • OR
    • Test exception - No functional changes.
    • OR
    • N/A
  • Documentation

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

@davkean davkean requested a review from a team as a code owner March 4, 2022 03:22
@nkolev92
Copy link
Member

nkolev92 commented Mar 4, 2022

If you name your branch dev-*, something like dev-davkean-removeUnneededAllocations it will auto trigger the build.

Copy link
Contributor

@kartheekp-ms kartheekp-ms left a comment

Choose a reason for hiding this comment

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

I found another code path where a similar fix can be applied. The fix is to replace the line no:712 with return new List<IAssetsLogMessage>(0); and replace line no: 715 with var items = new List<IAssetsLogMessage>(json.Count);

internal static IList<IAssetsLogMessage> ReadLogMessageArray(JArray json, string projectPath)
{
if (json == null)
{
return new List<IAssetsLogMessage>();
}
var items = new List<IAssetsLogMessage>();
foreach (var child in json)
{
var logMessage = ReadLogMessage(child as JObject, projectPath);
if (logMessage != null)
{
items.Add(logMessage);
}
}
return items;
}

@jeffkl
Copy link
Contributor

jeffkl commented Mar 7, 2022

I restarted the failing test job

These List<T> where unnecessarily being resized despite being aware of how many items are being added.
@davkean
Copy link
Contributor Author

davkean commented Mar 7, 2022

@kartheekp-ms I fixed that one too.

@davkean davkean merged commit 7917ca8 into dev Mar 7, 2022
@davkean davkean deleted the RemoveUnneededAllocations branch March 7, 2022 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants