Memory Usage - Lambda closures in hot paths cause excessive allocations

[habbes]

We have more cases of closures causing excessive allocation in AGS. These are cases where we use a lambda expression that references a variable from the enclosing scope. This causes the compiler to create a new object (DisplayClass) that will store the referenced variables so that the lambda can still access them when it's called. In addition, the generated Func or Action is not cached, so a new Func or Action is created each time.

We can optimize these lambdas refactoring them such that all the variables that they need be passed as arguments. In the case of LINQ methods like IEnumerable.Any() we could consider using plain for loops.

There's also another subtle source of "surprise" allocations when passing a static method as an argument to method expecting a func (or action).

Consider the following scenario:

using System;
public class C {
    public void M() {
        Helpers.CallFilter(Helpers.DoStuff);
    }
}

public static class Helpers {
    public static bool DoStuff() { return true; }
   
    public static void CallFilter(Func<bool> filter)
    {
        filter();
    }
}

Passing Helpers.DoStuff directly to Helpers.CallFilter causes a new Func<bool>(Helpers.DoStuff) to be created each time as you can see here

But when you wrap the static method into a lambda as follows:

using System;
public class C {
    public void M() {
        Helpers.CallFilter(() => Helpers.DoStuff());
    }
}

public static class Helpers {
    public static bool DoStuff() { return true; }
   
    public static void CallFilter(Func<bool> filter)
    {
        filter();
    }
}

Then a new Func is created only once and cached as demonstrated here. I find this to be quite intriguing, I'm not sure why the compiler doesn't cache the Func when the static method is passed directly.

Here are some of the "hot" spots for closure allocations:

Method	Cause of allocation	Potential remedy	Cost in AGS (% of allocation size)
WriterValidationUtils.ValidatePropertyDerivedTypeConstraint(PropertySerializationInfo)	The call DerivedTypeConstraints.Any(d => d == fullTypeName) references fullTypeName which is defined in the outer scope	Consider using a plain for loop	0.31% (DisplayClass)
Edm.ExtensionMethods.ReplaceAlias	The lambda passed to FirstOrDefault references typeAlias which is defined outside the lambda.	Consider using a for loop	0.51% (Func) + 0.26% (DisplayClass)
WriterValidationUtils.ValidateDerivedTypeConstraint	The lambda passed to derivedTypeConstraints.Any references fullTypeName defined in outer scope	Consider using for loop	20k DisplayClass allocations on my local profiling session
SelectedPropertiesNode.GetSelectedPropertiesForNavigationProperty	The lambda passed to this.children.Any() references navigationPropertyName defined in the outer scope	Consider using for loop	10k DisplayClass allocations on my local profiling session
Edm.ExtensionMethods.FindType	The static method RegistrationHelper.CreateAmbiguousTypeBinding passed as the last argument to FindAcrossModels causes a new Func to be created each time	Replace the last argument with an actual lambda (first, second) => RegistrationHelper.CreateAmbiguousTypeBinding(first, second)	0.49 % (Func)
ODataWriterCore.CheckForNestedResourceInfoWithContent	The lambda passed to this.InterceptException references currentNestedResourceInfo which is defined in the outer scope	Define the variable locally within the lambda since it can be obtained from thisParam.currentScope.Item	Not sure about AGS. On my local profile it accounts for 10k allocations (for serializing an entity set with 5k entities).

About that last issue, such allocations were fixed in this PR but I think this was inadvertently missed, since it's a quick fix. I don't have the figures for this in AGS since the PR that addressed most of such issues has not yet been deployed.

AGS:

Local profiler:

Assemblies affected

Microsoft.OData.Core 7.9.1

Reproduce steps

Create an app with an endpoint that uses the ODataWriter to write a resource set response with a large number of entities (eg. this experiment). Run a memory profiler against your app (e.g. the VS .NET Object Allocator Tracking Tool) then execute the endpoint with the profiler running, and check the allocation data compiled by the profiler during the request.

Expected result

Less number of allocations from closures

Actual result

A lot of allocations from closures

@joaocpaiva @Sreejithpin @mikepizzo @gathogojr

[chrisspre]

Thanks Clement, and thanks for adding these details.
I was able to verify that the compiler actually creates some sort of internal lazy initialization if the code uses CallFilter(() => Helpers.DoStuff()) which doesn't happen when using CallFilter(Helpers.DoStuff).
I am ok with this change but think we need to be careful to not over-generalize it to other uses of lambda's

[joaocpaiva]

I was also intrigued why compiler is not optimizing this, the same way when we explicitly use the lambda. Obviously, I was expecting they had a reason to not have done it yet.

dotnet/roslyn#5835

Kevin is correct. This is an optimization that was added to lambdas but not to method groups because it was a breaking change. The C# spec explicitly stated that method group to delegate conversions created a new delegate every time and there was evidence that customers depended on that.

Fast forward 10 years and the breaking change aspect is no longer seen as a blocker. We are willing to take the break here. The issue at this point is just cost / benefit. The optimization is well known here and perf sensitive code paths have pretty much all migrated to the lambda approach. The change to add caching for method groups is non-trivial and in an area of the compiler that is subject to unexpected regressions.

It's something we generally consider at the start of every release but usually more pressing optimization cases end up getting taken instead. This one usually falls off because a) there is a trivial work around b) there are lots of analyzers out there that recommend this exact optimization c) introducing it will likely lead to a few regressions that will take a few iterations to resolve d) it will break a few customers

I think they will fix this eventually at the compiler, but perhaps not very soon because it is low priority for the reasons they state above,. Until then their recommendation is to use static code analyzers to detect and apply the workaround, which is what we are doing in AGS.