Pay for an allocation/assignments of a capture class regardless of whether it's used

[davkean]

Version: Csc.exe 2.3.0.61907 (ee1637c)

Given:

    class Program
    {
        private void Capture(bool value)
        {
            if (value)
            {
                Method(() => value.ToString());
            }
        }

        private void Method(Action action)
        {
        }
    }

The following is the code gen:

.method private hidebysig instance void  Capture(bool 'value') cil managed
{
  // Code size       46 (0x2e)
  .maxstack  3
  .locals init ([0] class ConsoleApp65.Program/'<>c__DisplayClass1_0' 'CS$<>8__locals0',
           [1] bool V_1)
  IL_0000:  newobj     instance void ConsoleApp65.Program/'<>c__DisplayClass1_0'::.ctor()
  IL_0005:  stloc.0
  IL_0006:  ldloc.0
  IL_0007:  ldarg.1
  IL_0008:  stfld      bool ConsoleApp65.Program/'<>c__DisplayClass1_0'::'value'
  IL_000d:  nop
  IL_000e:  ldloc.0
  IL_000f:  ldfld      bool ConsoleApp65.Program/'<>c__DisplayClass1_0'::'value'
  IL_0014:  stloc.1
  IL_0015:  ldloc.1
  IL_0016:  brfalse.s  IL_002d
  IL_0018:  nop
  IL_0019:  ldarg.0
  IL_001a:  ldloc.0
  IL_001b:  ldftn      instance void ConsoleApp65.Program/'<>c__DisplayClass1_0'::'<Capture>b__0'()
  IL_0021:  newobj     instance void [mscorlib]System.Action::.ctor(object,
                                                                    native int)
  IL_0026:  call       instance void ConsoleApp65.Program::Method(class [mscorlib]System.Action)
  IL_002b:  nop
  IL_002c:  nop
  IL_002d:  ret
} // end of method Program::Capture

Notice that you pay for the creation/assignment of the capture class regardless of the branch in the method. This results in unexpected allocations, such as: #20775

[davkean]

I noticed that the code gen basically treats the capture class's field as if it was the parameter - if I use parameter elsewhere, then the compiler gens that as a read of the field from the capture class. Based on that, this looks very deliberate code gen and probably not overlooked.

[jaredpar]

Based on that, this looks very deliberate code gen and probably not overlooked.

Correct. This behavior is deliberate. The capture logic is entirely scope based and as an optimization it merges empty capture scopes into scopes which capture actual variables. In this case the scope which allocations the delegate has no captured variables and is hence "empty". The compiler optimizes the situation by merging the captures into a single scope, the one where value lives.

That being said, this is a frequent source of confusion amongst users. Additionally the logic around merging scopes was also done when we had a very different pattern for allocating closure types. It made a lot more sense in that context, possibly less in the current one.

@agocke is looking at this as a possible source of optimization in 15.5 when we are revisiting our capture code whole sale. Will let him dupe this against any bugs he may already have filed here.

[jaredpar]

CC @khyperia as an FYI as well

[Suchiman]

Not only that it causes possible unnecessary allocations, it also causes errors during debugging which caused me more confusion than i'd like to admit, up to the point thinking that the compiler has an emit bug and dissecting everything, pulling my hair out, before realizing, it's my debugging technique coupled to this unfortunate codegen.

For example, take this code

class Program
{
    private void Capture(bool value)
    {
        if (value)
        {
            var value2 = 12;
            Method(() => value ? value2 : value2 + 12);
        }
    }

    private void Method(Func<int> action)
    {
        action();
    }
}

If you're debugging and suppose value is false but you'd like to see what would happen if the branch was taken, so with the current instruction being on if (value) you move the instruction pointer to the { as if value were true and now you step and once you step over var value2 = 12; you receive a mind boggling NullReferenceException because moving the instruction pointer skipped initialization of the closure which causes value2 = 12; that actually tries to set a field on the uninitialized closure to produce a NullReferenceException

[myblindy]

Just ran into this earlier (the closed issue right above this) and was pointed to this as the original issue regarding closures allocating memory too early, so I figured I'd throw in my vote to have this looked at.

The worst part isn't even the allocations, though they add up very fast if you're not expecting them, it's that the problem is completely hidden. The C# code looks innocuous, with the allocation hidden behind a branch off the hot path, while in reality it's contributing to almost every single allocation in your program.

[tmat]

@Suchiman The suboptimal debugging experience is caused by #32352.

[CyrusNajmabadi]

There is no good solution here as moving things around may cause other problems. The best thing to do is to potentially write an analyzer and have strict rules about separating your functions. A common pattern is to internally have a static local function which contains the code causing the closure. Being static means it itself doesn't accidentally cause a closure by capturing anything. And the compiler def won't create any closure anywhere in that case.

For most people this doesn't matter and the defaults are good. For people where allocations are that impactful, the best thing to do is just be explicit and use only the patterns certain to be safe. The middle ground is likely to very much cause problems when perf is that critical.

[myblindy]

@CyrusNajmabadi

There is no good solution here

I'm curious why you say that, from the outside it seems that the solution is conceptually simple: don't allocate the closure type until it's actually used, ie inside the if branch. This isn't about loops or recursion or anything weird, it's a straight up condition around calling a lambda function, the dumbest approach will work flawlessly.

A common pattern is to internally have a static local function which contains the code causing the closure

That may be true, but it's only true because of the broken code generation. In fact that's how I solved my problem as well, but it's a work around only, it's not something I'd ever choose to write first.

The middle ground is likely to very much cause problems when perf is that critical.

I'd also be curious of what you mean here. I can tell you that the reason why I raised this issue earlier was because this generated IL was allocating over 2MB of 28 byte objects every single second (hot path in rendering code) with absolutely no indication that this could even be possible by just looking at the C# code. What could be worse, short of just plain incorrect code being generated?