Evaluator allocation optimization

[MichalPavlik]

Fixes #6307

Context

Evaluator sometimes allocates List<ProjectRootElement> instances when it's not necessary.

Changes Made

Collection allocation is postponed and empty Enumerable singleton is used to represent empty result.
@drewnoakes I tried to use ImmutableArray, but it produced more allocations - builder is copying underlying array when creating instance of the immutable collection.

Testing

I tracked List<ProjectRootElement> allocations while building simple solution with two empty C# projects (.NET Framework and Core). Number of allocated instances dropped from 264 to 178.

Notes

This optimization looked like good first issue to solve, but complexity of the Evaluator is IMHO high and using out parameters for these collections makes it more difficult to track their flow :)

[dnfadmin]

All CLA requirements met.

[dnfadmin]

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

❌ MichalPavlik sign now
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[Therzok]

Out of curiosity, instead of having the API allocate the list, why not pass a list to add to, to the API? That avoids all the intermediate collection allocations.

[ladipro]

Changing the type of the variable to IEnumerable has the unfortunate implication of regressing foreach performance. Compare the IL code here:

https://sharplab.io/#v2:EYLgtghglgdgNAFxFANgHwAICYCMBYAKAwAYACDHAFgG5CMBmcrUgYVIG9DTvzGNLSAWQAUAGSgBnBAB4KxAHykUkhAEoOXHloBmAewBOAUwgBjABalhc0hNKwlK1Zq3dOBFx/I4AnMImrad08AX2dSUKDuBnIBADkrHHpZHAUHKXU3T1I9I1MLBLJbe2V0sK1MrJ4KX39ArIitCOCgA

When typed as IEnumerable, List's enumerator is boxed (an extra allocation) and then accessed through the IEnumerator interface (tiny bit slower than direct calls).

Can this optimization be implemented without changing the type, by using a special value of null to represent an empty list for example?

[MichalPavlik]

I know about value type based enumerator for List<T> and I was thinking about using null to keep specific type. To be honest, I don't know why I rejected this idea so it worth to use it.
I will try to use ImmutableArray and MoveToImmutable as Drew mentioned. Maybe we will not need List at all.

[MichalPavlik]

@ladipro I changed the code to use nulls.

[drewnoakes]

@drewnoakes I tried to use ImmutableArray, but it produced more allocations - builder is copying underlying array when creating instance of the immutable collection.

There are several ways to construct ImmutableArray. If you use a builder with correct capacity, then MoveToImmutable there is no wasted allocation.

Changing the type of the variable to IEnumerable has the unfortunate implication of regressing foreach performance.

If you are able to use ImmutableArray then the foreach performance benefit will be maintained.

[MichalPavlik]

@Therzok It was one of the solutions I proposed in the issue. The change would be more impactful, because for example ExpandAndLoadImports is adding items to collection conditionally based on containsWildcards value. I wanted to avoid bigger changes and make the PR small.

[MichalPavlik]

@drewnoakes When I looked at the description for this method some time ago, I somehow fixed in my mind that it immediately creates new array for Builder. I checked the source and it seems I understood it wrong. I will update my branch with ImmutableArray and measure it again.

[MichalPavlik]

@drewnoakes I tried MoveToImmutable, but it requires to know the exact number of elements in advance. So I used null insead of empty enumerable in last commit.

[ladipro]

Why is this needed?

[ladipro]

nit:

Suggested change

	if (sdkResult.AdditionalPaths != null)
	{
	if (projects == null && sdkResult.AdditionalPaths.Count > 0)
	{
	projects = new List<ProjectRootElement>();
	}
	if (sdkResult.AdditionalPaths?.Count > 0)
	{
	projects ??= new List<ProjectRootElement>();

Or maybe even:

Suggested change

	if (sdkResult.AdditionalPaths != null)
	{
	if (projects == null && sdkResult.AdditionalPaths.Count > 0)
	{
	projects = new List<ProjectRootElement>();
	}
	if (sdkResult.AdditionalPaths?.Count > 0)
	{

and new up the List lazily when actually adding to it a few lines below.

[MichalPavlik]

Oh boy! That's an embarrassing refactoring mistake. I'm starting to have selective blindness in this part of code. It's scary that all tests passed... It should be fixed now.