Reduce size of LibraryDependency class

[drewnoakes]

Bug

Fixes: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1826369 Fixes: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1835763

Regression? Last working version: N/A

Description

This PR is @melytc's work as part of the perfathon. I put together the PR as she's OOF the coming week.

The LibraryDependency class has been identified as contributing to high GC on customer machines, allocating up to 37MB/s. This change reduces the memory footprint of that type in two ways:

1. Six integral fields are packed into a single 32-bit integer, reducing 12 bytes down to 4 bytes.
2. The backing collection for the NoWarn property is now lazily allocated, as it will usually be empty.

For the packing of fields, there are three Boolean and three enum-based fields. With some masking and shifting, these can all be efficiently stored within a single 32-bit integer space. Unit tests have been added to ensure the correctness of those fields now and in future. Comments have been added to the enums being packed to ensure any future modifications to those enums take this packing strategy into account. The packing of enums does leave some headroom, so a few more members could be added to each without requiring any code changes in LibraryDependency.

For the NoWarn collection, an additional NoWarnCount property was added that allows callers to test whether the collection is empty before calling the NoWarn getter (which would lazily allocate a backing collection). This seems to be a pragmatic approach to this issue given the backwards compatibility requirements. Several callers have been updated to check the count first.

This PR also null annotates LibraryDependency and some other types that needed annotations in the process of doing so.

PR Checklist

• [x] PR has a meaningful title

• [x] PR has a linked issue.

• [x] Described changes

• Tests

  • [x] Automated tests added
  • OR
  • [ ] Test exception
  • OR
  • [ ] N/A

• Documentation

  • [ ] Documentation PR or issue filled
  • OR
  • [x] N/A

[nkolev92]

@drewnoakes There are bunch of test failures that look like true failures. Can you please take a look?

[drewnoakes]

I can't see any failures that seem related to this PR. Can you point to an example of what you consider a true failure?

EDIT I assume you're talking about the functional tests.

[zivkan]

Every test I look at in Azure DevOps, and go to the history tab for the failed test, shows it only fails in this PR's branch, not in any other branch. So, while NuGet does have flakey tests, all the tests I looked at for this PR's build appear not to be flakey, it appears related to the change in this PR. Given the same test fails on Mac, Linux, Windows .NET Framework and Windows .NET (Core) also suggests it's not flakey, and related to this PR.

[drewnoakes]

Thanks. I saw an earlier build number as the first instance of the failure, but that seems to have been an earlier build of this PR.

That failure is very high level. It's not quickly obvious where the failure could be l, or how to narrow it down from such a high level test.

I won't have time to look at this for a few days.

[drewnoakes]

@zivkan @nkolev92 I found and fixed an issue. The tests are running now.

To be clear this bug was in my code, not @melytc's! I made a change to avoid materializing the NoWarn collection in Equals and messed it up. The last commit fixes it, and adds a test for that case.

[zivkan]

source build retargets everything to build for net8.0, and this has caused a compile error: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=8059454&view=logs&j=cb98efd6-7062-56ef-f642-0176540bbe0a&t=d42c45d2-f6c8-5ae2-d854-abf6f8d78853&l=88

/home/vsts/work/1/s/src/NuGet.Core/NuGet.LibraryModel/LibraryDependency.cs(264,72): error CS8600: Converting null literal or possible null value to non-nullable type. [/home/vsts/work/1/s/src/NuGet.Core/NuGet.LibraryModel/NuGet.LibraryModel.csproj::TargetFramework=net8.0]

[drewnoakes]

I like the lazy initialization changes.

It helps, but it makes all calling code more ugly and requires callers to know that they need to check this property before reading. Future code will easily break this invariant and undo any benefit from making it lazy. the Freeze approach in a recent PR could avoid that.

The packing seems like a lot.

Prism shows a lot of memory devoted to instances of this type. Packing these fields cuts the size of the type significantly. By contrast to the lazy change however, it's completely enclosed within this type, doesn't require any change to the users of these types, and cannot be broken by external changes.

We're talking about a few booleans here. The longer name for the dependency would have a bigger impact to the overall size of the type.

It's three booleans and three enums. Altogether that saves ten bytes per instance. For this to appear in Prism, there must be a lot of instances of this type in the real world.

[ghost]

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]

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]

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.

[davkean]

@nkolev92 @drewnoakes @zivkan

Please reconsider this PR; in dumps taken from a large partner's internal restore, I can see that there are almost half a million of these objects in memory.

This is just a snapshot at a point in time so doesn't include all of its allocations over the lifetime of the restore. The memory at its peak, quadrupled than what the above dump showed, so there's likely more than 2 million of these objects in memory at any point throughout its restore.

When you have that many instances of these objects in memory, every byte counts.

[davkean]

I'm reopening for consideration.

[nkolev92]

The interesting thing that @zivkan's tests show that the difference was really small: https://github.com/NuGet/NuGet.Client/pull/5303#discussion_r1274806397. So this PR might not be enough to really make a dent.

Another idea would be to intern this type (within the context of restore).

A more breaking approach would be to do https://github.com/NuGet/Home/issues/13077, and then we can make this type immutable, and change all the initialization code around it.

I think there's probably parts of this PR that are less controversial than others. For example, I think eliminating the empty list creation would make a bigger dent than the bool packing, and is shorter. Maybe we can start with that too.

[dotnet-policy-service[bot]]

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.

[aortiz-msft]

Closing as per last comment.