Fix IsSubSetOfWithNullCheck (#11222)

[mjolka]

Bug

Fixes: NuGet/Home#11222

Regression? Last working version:

Description

IsSubSetOfWithNullCheck is supposed to return true when other is
a subset of parent.

This fixes an issue where restore can take a long time to run on
solutions with many references and packages.

A test solution was created with 42 projects, with project n referencing
projects 1..(n-1). Each project also had a couple of package references
and a NoWarn element.

Before this change, restore ran for 5 minutes, used ~14GB of memory,
and failed with an OutOfMemoryException ("Array dimensions exceeded
supported range.")

After this change, restore completed in under 5 seconds.

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

[erdembayar]

@jeffkl
Could please take a look at this PR? I believe you have context on this.

[nkolev92]

Can we please add tests for this?

[mjolka]

I've added unit tests and in the process found and fixed an issue with NodeWarningProperties.IsSubSetOf.

I've also created a repo with a solution I described in the original comment, where a restore runs for several minutes and dies with a OOM exception. You can find it here: https://github.com/mjolka/LotsOfDependencies

If you have any suggestions on how to turn that into a test case, please let me know.

[nkolev92]

LGTM.
Thanks for this change!

Just some tweaks to the tests before approving.

[erdembayar]

I've added unit tests and in the process found and fixed an issue with NodeWarningProperties.IsSubSetOf.

I've also created a repo with a solution I described in the original comment, where a restore runs for several minutes and dies with a OOM exception. You can find it here: https://github.com/mjolka/LotsOfDependencies

If you have any suggestions on how to turn that into a test case, please let me know.

Thank you. Is performance improvement still holding after your recent changes?
Could you include before and after output windows log?

[mjolka]

Sorry, pushed those changes by accident.

[nkolev92]

Awesome fix.

Thanks @mjolka

[nkolev92]

@NuGet/nuget-client Last call for reviews.
I'll try to merge Monday afternoon if there's no additional feedback.

[mjolka]

@nkolev92 Thanks for your feedback. I was going to make some further changes to the test cases today but if you're happy with the state of the PR I'll leave it alone.

Would you like me to squash those "wip" commits and force push?

@erdembayar The performance improvement still holds after the recent changes.

I've attached the output of running NuGet.exe restore .\src\LotsOfDependencies.sln -Force -ForceEvaluate before and after the changes, is that what you were asking for?

before.txt

after.txt

[nkolev92]

@mjolka We squash and merge, no need to squash in your branch. I sometimes do it on the developing branch as it makes rebasing easier.

What kind of test changes were you thinking of?

[mjolka]

@nkolev92 I was thinking of replacing the PackageSpecificNoWarn helper enum with a string based format for the test cases. Here's an example: mjolka@41f8928

[nkolev92]

That's a good change. No changes to adding/removing test cases right?

[mjolka]

@nkolev92 Right, no changes to adding/removing test cases. This would be the change: mjolka@d4bd3e3

[nkolev92]

feel free to push if you'd like to.
I'll rerun the tests when you do.

[mjolka]

Thanks @nkolev92 .

I just noticed what might be a bug in NodeWarningProperties.Intersect(HashSet<NuGetLogCode> first, HashSet<NuGetLogCode> second) and wanted to get your thoughts on it as it's closely related to this change.

In the method above Intersect, IsSubSetOfWithNullCheck, null is considered to be the empty set:

NuGet.Client/src/NuGet.Core/NuGet.Commands/RestoreCommand/Logging/TransitiveNoWarnUtils.cs

Lines 870 to 890 in aebfb49

	private static bool IsSubSetOfWithNullCheck(HashSet<NuGetLogCode> parent, HashSet<NuGetLogCode> other)
	{
	// Null is empty and always a subset.
	if (other == null || other.Count == 0)
	{
	return true;
	}
	// A null or empty parent cannot be a superset.
	if (parent == null || parent.Count == 0)
	{
	return false;
	}
	if (other.Count <= parent.Count)
	{
	return parent.IsSubsetOf(other);
	}
	return false;
	}

Following that logic, Intersect(first, null) and Intersect(null, second) should both return null (or an empty HashSet). However, Intersect(first, null) returns first and Intersect(null, second) returns second.

NuGet.Client/src/NuGet.Core/NuGet.Commands/RestoreCommand/Logging/TransitiveNoWarnUtils.cs

Lines 892 to 913 in aebfb49

	private static HashSet<NuGetLogCode> Intersect(HashSet<NuGetLogCode> first, HashSet<NuGetLogCode> second)
	{
	if (ReferenceEquals(first, second))
	{
	return first;
	}
	if (first == null)
	{
	return second;
	}
	if (second == null)
	{
	return first;
	}
	var result = new HashSet<NuGetLogCode>(first);
	result.IntersectWith(second);
	return result;
	}

[nkolev92]

I think it's ok if null and empty are considered the same for practical purposes.
If you do want to make changes there, can we please do that in a different PR? :)