Optimize multicast stubs

[MichalPetryka]

Rewrites multicast stubs to byref loops to remove bounds checks and covarianc helpers from every iteration.

This assumes the wrapper struct is the same size as a ref, is such assumption fine for the VM? @jkotas @MichalStrehovsky

[MichalPetryka]

@EgorBot -arm -amd --envvars DOTNET_JitDisasm:IL_STUB_MulticastDelegate_Invoke

using BenchmarkDotNet.Attributes;

public class MyBenchmarks
{
    public Action a;

    [GlobalSetup]
    public void Setup()
    {
        for (int i = 0; i < 10000; i++)
            a += new Action(() => {});
    }

    [Benchmark]
    public void Bench() => a();
}

[MichalPetryka]

Hmm the asm seems better but the arm perf is much worse, I assume it's cause the JIT is ordering blocks wrong which causes branch mispredictions.

EDIT: the branch order is also different than what I get locally, is the bot using any weird settings? @EgorBo

[jkotas]

This assumes the wrapper struct is the same size as a ref, is such assumption fine for the VM

These sort of Unsafe.As UB casts can confuse optimizations that depend on accurate type information. I think we should be rather fixing the few places where we still do them in CoreLib instead of introducing new ones.

the arm perf is much worse,

We have seen number of cases where unsafe code "optimizations" result in worse performance. It sounds like this is another one of those.

covariant helpers from every iteration.

What are the covariant helpers that this is eliminating?

for (int i = 0; i < 10000; i++)
a += new Action(() => {})

The typical multicast delegate has very few targets, and the targets are typically different. This is not very representative microbenchmark.

[MichalPetryka]

@EgorBot -arm -amd --envvars DOTNET_JitDisasm:IL_STUB_MulticastDelegate_Invoke

using BenchmarkDotNet.Attributes;

public class MyBenchmarks
{
    public Action a;

    [GlobalSetup]
    public void Setup()
    {
        for (int i = 0; i < 10000; i++)
            a += new Action(() => {});
    }

    [Benchmark]
    public void Bench() => a();
}

[MichalPetryka]

These sort of Unsafe.As UB casts can confuse optimizations that depend on accurate type information. I think we should be rather fixing the few places where we still do them in CoreLib instead of introducing new ones.

It's not UB here since the As is on the ref, not on the object. It's just layout reliant. If desired, I can swithc the code to fetch the sizeof of thee struct and use ldfld to avoid assuming the field is at offset 0.

We have seen number of cases where unsafe code "optimizations" result in worse performance. It sounds like this is another one of those.

I assume the bad perf is from the horrible layout from the JIT caused by the debugging helper check which is predicted to be hot. Does the code need to recheck this on every iteration?

What are the covariant helpers that this is eliminating?

I misread the loop as doing ldelema, it's just a bounds check, not a covariance helper.

The typical multicast delegate has very few targets, and the targets are typically different. This is not very representative microbenchmark.

The targets being different shouldnt matter much outside of CPU branch prediction here and it makes results more stable. This should also not scale badly with size, I assume this'd be faster even with just 2 delegates if the JIT used a proper layout.

The actual diff of the loop, excluding the debugging call is this:

[huoyaoyuan]

This doesn't look optimal for branch predicting (forward conditional jump as hot path). The original logic was constructed for optimizing this.

[MichalPetryka]

This doesn't look optimal for branch predicting (forward conditional jump as hot path). The original logic was constructed for optimizing this.

The original IL was unusual and confusing the JIT causing even worse performance. Additionally, it was treated as hot there too. cc @tannergooding since we discussed this on Discord.

It seems the only way to solve this here would be an internal Assume.Likely/Unlikely.

[tannergooding]

Like I suggested on discord, if we don't actually need multicast to lightup the debugger support at any point in the loop; then cloning is likely your best bet here.

You can rewrite the IL to be better and not the asm default to likely and still be understandable as normal control flow, but its quite a bit harder.

I'm not sure the extra churn is worth it though; the current stuff seems plenty good enough.