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.

[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]

@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();
}

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "72ac01bc6cd0c96b30522ba0fdb359af4d71ff99",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "cb301bc23e682da023cf3c51985d45fbb528e7c3",
  "last_reviewed_commit": "72ac01bc6cd0c96b30522ba0fdb359af4d71ff99",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "cb301bc23e682da023cf3c51985d45fbb528e7c3",
  "last_recorded_worker_run_id": "29695232837",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "d73c1d8f1f5665678b727c172c113cb33b996a55",
      "review_id": 4730548759
    },
    {
      "commit": "72ac01bc6cd0c96b30522ba0fdb359af4d71ff99",
      "review_id": 4731129068
    }
  ]
}

[github-actions]

Holistic Review

Motivation: Legitimate perf improvement. The multicast invocation stubs (both the VM-generated IL in comdelegate.cpp and the AOT-generated IL in DelegateThunks.cs) iterate the invocation list by index, which incurs a bounds check and array covariance/type-check helper on every delegate call. Rewriting the loop to walk raw byref pointers from start to end removes both per-iteration costs on a hot path.

Approach: Reasonable and internally consistent across the CoreCLR VM stub, the NativeAOT thunk, and the shared MulticastDebuggerTraceHelper. The byref start/end loop, MemoryMarshal.GetArrayDataReference over the Wrapper element type, and Wrapper.GetElementSize()/th.GetSize() stride all line up, and the size-of-Wrapper-equals-size-of-ref assumption is sound since Wrapper is a single-field struct wrapping a Delegate reference. The debugger-trace refactor moving the cold call out-of-line is fine.

Summary: ⚠️ Needs Changes. The core loop rewrite looks correct, but the rewritten MulticastDebuggerTraceHelper has a reversed Unsafe.ByteOffset argument order that produces a negative/garbage step index for every delegate after the first (see inline comment). This path only runs under an attached debugger with multicast tracing active, so it would escape normal CI. Recommend fixing and, if feasible, adding coverage for the debugger multicast-trace scenario. Other aspects (loop bounds, blt/blt.un signedness on byrefs, stride math, internal exposure of _invocationList) reviewed and look acceptable.

---

Detailed Findings

❌ Correctness — Reversed Unsafe.ByteOffset operands in MulticastDebuggerTraceHelper

See the inline comment on StubHelpers.cs. ByteOffset(origin, target) returns target - origin, so passing (ref position, ref list[0]) computes &list[0] - position, which is negative for all but the first iteration and yields a bogus count. Operands must be swapped so count = (position - &list[0]) / sizeof(object).

✅ Loop bounds and stride — VM and AOT stubs

Both stubs compute end = start + count * elementSize and loop while current < end, incrementing current by one element per step. Multicast stubs are only used when count ≥ 1 (do/while semantics preserved via unconditional first iteration then blt/blt.un back-branch), so the empty-list case is not a concern here. The AOT path uses blt.un (unsigned) and the VM path uses EmitBLT (signed); for forward-walking heap addresses within a single array both behave identically in practice, so this is not flagged as a defect, though blt.un is the more conventional choice for pointer comparison.

✅ Wrapper size assumption

Wrapper is a single-field struct over a Delegate reference, so sizeof(Wrapper) == sizeof(object) and using GetArrayDataReference<Wrapper> / object-sized stride is valid. The PR description explicitly raises this question to the VM maintainers; confirming with them is advisable, but the assumption holds for the current layout.

💡 Observation — _invocationList visibility widened to internal

Changing _invocationList from private to internal is required so StubHelpers can read it. This is acceptable within CoreLib but slightly widens the field's reach; no action needed.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 149.6 AIC · ⌖ 11.1 AIC · ⊞ 10K

[MichalPetryka]

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

using BenchmarkDotNet.Attributes;

public class MyBenchmarks
{
    public Action a10;
    public Action a100;
    public Action a1000;
    public Action a10000;

    [GlobalSetup]
    public void Setup()
    {
        for (int i = 0; i < 10; i++)
            a10 += new Action(() => {});
        for (int i = 0; i < 100; i++)
            a100 += new Action(() => {});
        for (int i = 0; i < 1000; i++)
            a1000 += new Action(() => {});
        for (int i = 0; i < 10000; i++)
            a10000 += new Action(() => {});
    }

    [Benchmark]
    public void Bench10() => a10();
    [Benchmark]
    public void Bench100() => a100();
    [Benchmark]
    public void Bench1000() => a1000();
    [Benchmark]
    public void Bench10000() => a10000();
}

[github-actions]

Holistic Review

Motivation: Unchanged from the prior review. This is a legitimate perf improvement to the multicast invocation stubs (VM-generated IL in comdelegate.cpp and AOT-generated IL in DelegateThunks.cs): walking the invocation list by raw byref pointers from start to end removes the per-iteration bounds check and array covariance/type-check helper on a hot delegate-invocation path.

Approach: Unchanged in spirit. The new commit adapts the stubs to the _helperObject / _extraData / Delegate.Wrapper[] field layout that landed in main (a merge-conflict resolution) and, importantly, fixes the Unsafe.ByteOffset bug called out in the prior review. The byref start/end loop, GetArrayDataReference<Wrapper> origin, and Wrapper element-size stride all line up across the VM stub, the NativeAOT thunk, and MulticastDebuggerTraceHelper. Wrapper remains a single-field struct over a Delegate reference, so sizeof(Wrapper) == sizeof(object) and the object-sized stride assumption still holds.

Summary: ✅ LGTM. The incremental "Clenaup merge" commit resolves the correctness defect from the previous review and re-bases the stubs onto the current field layout without introducing new issues. No actionable findings in the incremental scope.

Assessment History

• review 4730548759 reviewed commit d73c1d8 with verdict ⚠️ Needs Changes; current verdict is ✅ LGTM. Changed. The incremental commit 72ac01b rewrites MulticastDebuggerTraceHelper so Unsafe.ByteOffset now takes (origin = &helperObject[0], target = position), yielding count = (position - &list[0]) / sizeof(Wrapper) — the previously reversed operands are corrected, producing a non-negative index for every iteration. The same commit also migrates the stubs from _invocationList/InvocationCount to _helperObject/_extraData with the Wrapper element type (merge resolution), and switches the VM's trace-count read to CONV_U/LDIND_U4 (the counter g_multicastDelegateTraceActiveCount is a DWORD). Motivation and overall approach are unchanged; the risk is materially reduced, hence the verdict change.

---

Detailed Findings

✅ Correctness — reversed Unsafe.ByteOffset fixed

The defect flagged previously in StubHelpers.cs is resolved. ByteOffset(ref list[0], ref position) returns position - &list[0], a non-negative byte delta, and dividing by sizeof(Delegate.Wrapper) yields the correct current index. The cast of _helperObject to Delegate.Wrapper[] is safe here because this helper is only reached for multicast delegates.

✅ Field-layout migration and references

FIELD__DELEGATE__HELPER_OBJECT, FIELD__DELEGATE__EXTRA_DATA, CLASS__DELEGATEWRAPPER, and FIELD__DELEGATEWRAPPER__VALUE all resolve in corelib.h, and the AOT thunk reads the Wrapper.Value field via GetKnownField("Value"), matching the struct's internal Delegate? Value. _helperObject is internal, so StubHelpers can read it.

✅ VM trace-count read CONV_U/LDIND_U4

g_multicastDelegateTraceActiveCount is a DWORD; reading it as unsigned 4-byte is the correct/conventional choice. Since it is only compared for equality against 0 (EmitBEQ), the signedness change is behavior-preserving.

✅ Loop bounds and stride — unchanged from prior scope

Both stubs compute end = start + count * sizeof(Wrapper) and walk one element per iteration with a do/while back-branch, so the semantics reviewed previously are preserved. The remaining renames (fReturnVal→fReturns, startLocal→currentRef, etc.) are purely cosmetic.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 99.4 AIC · ⌖ 16.6 AIC · ⊞ 10K