Inefficient codegen when a lambda references an instance member

[smoogipoo]

Description

We've recently encountered a method allocating even though there was no allocate-y code running. We've since tracked this down to the following:

IL shows the capturing lambda's DisplayClass always seems to be "allocated" as soon as the method is entered, but the JIT has the ability to control when the object is initialised. When the lambda captures both a local and this, the JIT is suddenly unable to forego this initialisation even if the method terminates execution early.

Here's a gist that shows the issue: https://gist.github.com/smoogipoo/810c033962b8a95289dbdabd9d84801b
And a SharpLab repro.

Is this a valid and/or known issue, or something we should be wary of going forward?

Configuration + Data

BenchmarkDotNet=v0.12.1, OS=manjaro 
AMD Ryzen 9 3950X, 1 CPU, 32 logical and 16 physical cores
.NET Core SDK=3.1.103
  [Host]        : .NET Core 2.2.8 (CoreCLR 4.6.28207.03, CoreFX 4.6.28208.02), X64 RyuJIT
  .NET Core 2.2 : .NET Core 2.2.8 (CoreCLR 4.6.28207.03, CoreFX 4.6.28208.02), X64 RyuJIT
  .NET Core 3.1 : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT

Method	Job	Runtime	Mean	Error	StdDev	Ratio	RatioSD	Gen 0	Gen 1	Gen 2	Allocated
SafeMethod	.NET Core 2.2	.NET Core 2.2	545.8 ns	0.47 ns	0.41 ns	1.00	0.00	-	-	-	-
AllocatingMethod	.NET Core 2.2	.NET Core 2.2	7,151.4 ns	135.02 ns	112.75 ns	13.10	0.20	24.4064	-	-	32000 B
SafeMethod	.NET Core 3.1	.NET Core 3.1	276.8 ns	0.26 ns	0.23 ns	1.00	0.00	-	-	-	-
AllocatingMethod	.NET Core 3.1	.NET Core 3.1	9,072.8 ns	179.96 ns	438.05 ns	32.64	1.40	0.9460	-	-	32000 B

I've also run this on mono, which seems to always initialise the object (see Gen 0, Benchmark.Net doesn't seem to have allocation tracking on mono).

BenchmarkDotNet=v0.12.1, OS=manjaro
AMD Ryzen 9 3950X, 1 CPU, 32 logical and 16 physical cores
  [Host]     : Mono 6.4.0 (makepkg/fe64a4765e6 Sat), X64
  Job-CKTNGU : Mono 6.4.0 (makepkg/fe64a4765e6 Sat), X64

Runtime=Mono

Method	Mean	Error	StdDev	Ratio	Gen 0	Gen 1	Gen 2	Allocated
SafeMethod	4.491 us	0.0069 us	0.0054 us	1.00	5.7449	-	-	-
AllocatingMethod	5.031 us	0.0076 us	0.0064 us	1.12	7.6904	-	-	-

category:cq
theme:dead-code
skill-level:expert
cost:medium

[jaredpar]

It's unclear what you're asking for here. Do you believe there is a bug in C# compiler, the JIT, etc ...

[smoogipoo]

It seems to be a JIT issue, though I don't think I'm fully qualified to be able to answer that.

IL-wise they generate very similar code:

    .method private hidebysig 
        instance int32 safeMethod () cil managed 
    {
        // Method begins at RVA 0x2050
        // Code size 8 (0x8)
        .maxstack 3
        .locals init (
            [0] class C/'<>c__DisplayClass2_0' 'CS$<>8__locals0'
        )

        // sequence point: hidden
        IL_0000: newobj instance void C/'<>c__DisplayClass2_0'::.ctor()
        IL_0005: stloc.0
        IL_0006: ldc.i4.0
        IL_0007: ret
    } // end of method C::safeMethod

    .method private hidebysig 
        instance int32 allocatingMethod () cil managed 
    {
        // Method begins at RVA 0x2064
        // Code size 15 (0xf)
        .maxstack 3
        .locals init (
            [0] class C/'<>c__DisplayClass3_0' 'CS$<>8__locals0'
        )

        // sequence point: hidden
        IL_0000: newobj instance void C/'<>c__DisplayClass3_0'::.ctor()
        IL_0005: stloc.0
        IL_0006: ldloc.0
        IL_0007: ldarg.0
        IL_0008: stfld class C C/'<>c__DisplayClass3_0'::'<>4__this'
        IL_000d: ldc.i4.0
        IL_000e: ret
    } // end of method C::allocatingMethod

Whereas the JIT ASM is very different:

C.safeMethod()
    L0000: xor eax, eax
    L0002: ret

C.allocatingMethod()
    L0000: push esi
    L0001: mov esi, ecx
    L0003: mov ecx, 0x9c0cadc
    L0008: call 0x05c230cc
    L000d: lea edx, [eax+4]
    L0010: call 0x0f7fa3b0
    L0015: xor eax, eax
    L0017: pop esi
    L0018: ret

With the only difference being that an instance field is referenced in the allocatingMethod path.

[jaredpar]

Moving back to runtime as the issue is about the JIT generation for the pattern emitted by Roslyn, not the Roslyn pattern itself. At least that is my reading at this point. If the discussion comes back to the pattern emitted here we can move back to Roslyn.

[stephentoub]

Isn't the concern here that the display class is new'd up at the beginning of the method? That's entirely due to the C# compiler's IL code gen and is not something the JIT would change. I don't see how this is a runtime or JIT issue.

(The asm is different because in one case the JIT can actually see that the method is a nop and makes it such.)

[jaredpar]

@stephentoub I've been reading it as they want the JIT to special case and optimize this pattern. @smoogipoo is acknowledging the IL difference but in the face of that still wants the JIT ASM to be the same.

[AndyAyersMS]

Seems like incorrect odd IL to me -- for this source

    private int allocatingMethod()
    {
        return 0;

        int local = 2;
        return list.Count(_ => field > local);
    }

CSC produces this IL:

IL_0000  73 08 00 00 06    newobj       0x6000008
IL_0005  0a                stloc.0     
IL_0006  06                ldloc.0     
IL_0007  02                ldarg.0     
IL_0008  7d 04 00 00 04    stfld        0x4000004
IL_000d  16                ldc.i4.0    
IL_000e  2a                ret         

Now, given that IL, the jit should arguably be able to optimize away the newobj, but this is blocked because the jit doesn't propagate non-nullness for local 0 fast enough (it could/should happen during local AP in morph), and so the jit thinks the stfld might cause an exception, and that keeps the newobj live long enough that it can't be optimized away.

[stephentoub]

I've been reading it as they want the JIT to special case and optimize this pattern

Why couldn't Roslyn similarly avoid emitting the dead code?

[AndyAyersMS]

@smoogipoo I take it this is distilled from some more complex example? If so, do you have examples where Roslyn hoists the closure construction up above more interesting constructs than return 0?

Eager construction is likely deemed safe as the only observable effect is extra heap allocation, but as you note this allocation is not expected and seemingly should be deferred until the point of plausible use.

[jaredpar]

Why couldn't Roslyn similarly avoid emitting the dead code?

Roslyn certainly could. Assuming this wasn't done initially because it can complicate code gen a bit and it's a scenario for which the user has received a warning. Generally that gets prioritized fairly low.

Eager construction is likely deemed safe as the only observable effect is extra heap allocation, but as you note this allocation is not expected and seemingly should be deferred until the point of plausible use.

Agree Roslyn would ideally delay the closure allocation until it was actually used. This has come up before and is tracked by issue 20777. There have been a few discussions over the years but historically there has been concern the extra complexity will outweigh the wins. May be worth taking another look at once we get past C# 9.

In terms of resolving this issue. If we want to look at it as a Roslyn request we can transfer back to roslyn and I can dupe it against 20777 + add parts of the scenario. If want to have it track the potential JIT optimization discussed then we can leave it here. Either way is good by me.

[smoogipoo]

@AndyAyersMS The example I posted was hoisted from https://github.com/ppy/osu-framework/blob/c857318bdf8d3bb06b2d806bc957be571bded0ed/osu.Framework/Graphics/Cursor/TooltipContainer.cs#L195-L253.

The code which allocates (L235-252) runs <1% of the time, with majority of exits being done through L214.

The resultant IL: https://gist.github.com/smoogipoo/0ae07bebe7478c6960da22b16983a67b

[AndyAyersMS]

@jaredpar since you already have an issue for this we can keep this one to cover the codegen aspect. Marking as future.