perf: eliminate store.ToArray() allocation on mock behavior execution hot path

[thomhurst]

Summary

• Add IArgumentFreeBehavior interface for behaviors that don't use method arguments (ReturnBehavior, CallbackBehavior, VoidReturnBehavior, ThrowBehavior, ComputedReturnBehavior, RawReturnBehavior, ComputedRawReturnBehavior). The typed HandleCall<T1,...> methods check for this interface first and skip the store.ToArray() allocation entirely.
• Add typed callback behaviors (TypedCallbackBehavior<T1,...T8>) with matching ITypedBehavior<T1,...T8> interfaces, so typed callbacks execute without boxing arguments into object?[].
• Update source generator to emit direct typed callback registration (EnsureSetup().Callback(callback)) instead of wrapping in an object?[] closure (EnsureSetup().Callback(args => callback(...))).
• Add ExecuteBehavior<T1,...> helper methods that implement a three-tier dispatch: IArgumentFreeBehavior → ITypedBehavior<T...> → fallback store.ToArray().

Benchmark results

Benchmark	Before	After	Change
Invocation (void)	317 ns / 208B	190 ns / 120B	40% faster, 42% less alloc
Invocation (string)	271 ns / 144B	119 ns / 88B	56% faster, 39% less alloc
Invocation (100 calls)	31,365 ns / 21.2KB	19,356 ns / 11.9KB	38% faster, 44% less alloc
Callback (no-args)	765 ns / 3.14KB	461 ns / 3.06KB	40% faster
Callback (with-args)	906 ns / 3.35KB	585 ns / 3.15KB	35% faster
Setup (single)	494 ns / 2.03KB	368 ns / 2.03KB	25% faster
Setup (multiple)	696 ns / 2.7KB	519 ns / 2.7KB	25% faster

TUnit.Mocks now beats Imposter on all invocation benchmarks (previously behind) and beats Mockolate on setup (previously behind).

Test plan

• All 756 TUnit.Mocks.Tests pass
• All 30 source generator snapshot tests pass (snapshots updated)
• Benchmarks show improvements across all categories
• CI pipeline passes

[claude]

Performance Optimization Review: Eliminate store.ToArray() Allocation on Mock Hot Path

This is a well-structured performance PR with clear benchmarks and a logical layered dispatch strategy. The changes are consistent across the codebase and the snapshot tests confirm the source generator output is correct. Overall the approach is sound — here are my observations:

---

What's Working Well

Three-tier dispatch design is clean. The IArgumentFreeBehavior → ITypedBehavior<T...> → ToArray() fallback chain in ExecuteBehavior is the right pattern for this kind of incremental optimization. The priority is correct: argument-free behaviors (the majority in typical usage) are the fastest path.

Dual-interface on TypedCallbackBehavior — implementing both IBehavior and ITypedBehavior<T...> means the typed path works when available and the generic Execute(object?[]) path still works for any consumer calling through IBehavior, including the zero-arity HandleCall(object?[]) path in MockEngine.cs. This is the right approach.

Source generator condition is correct. The allNonOutParams is null && nonOutParams.Count <= 8 guard in MockMembersBuilder.cs ensures out/ref parameter overloads still use the args => closure (correct, since their types may differ from the typed store), and methods with more than 8 parameters fall back to the legacy path.

---

Issues and Observations

1. IBehavior.Execute(object?[]) is redundant on IArgumentFreeBehavior implementations but still mandatory (minor code smell)

Every class that now implements IArgumentFreeBehavior still has the full Execute(object?[] arguments) implementation that forwards internally. This is technically correct but creates a maintenance obligation: if someone adds a new IArgumentFreeBehavior class, they must not forget to implement the Execute(object?[]) signature even though it is never called by the optimized path.

A safer design going forward would be to provide a default implementation on IBehavior that routes through IArgumentFreeBehavior when possible — but that would require making IBehavior non-public or adding a default interface method, which may conflict with AOT constraints. As-is this is acceptable, but worth noting as a future refactor target.

2. ExecuteBehavior helpers do not use the store parameter for IArgumentFreeBehavior — the store argument is load-bearing only for the fallback path, yet is always passed

private static object? ExecuteBehavior<T1>(IBehavior b, ArgumentStore<T1> store, T1 a1)
    => b is IArgumentFreeBehavior af ? af.Execute() : b is ITypedBehavior<T1> tb ? tb.Execute(a1) : b.Execute(store.ToArray());

The store parameter is only consumed in the b.Execute(store.ToArray()) fallback. For the fast paths (the two is checks) the struct is passed in but never used. Since ArgumentStore<T> is a readonly struct, this is already on the stack and incurs no heap allocation, so this is not a performance concern. But it is slightly surprising: callers always construct the store anyway (e.g. var store = new ArgumentStore<T1>(arg1)), so there is no wasted work. This is fine as-is.

3. TypedCallbackBehavior unchecked cast on the Execute(object?[]) path

public object? Execute(object?[] arguments) { callback((T1)arguments[0]!); return null; }

The ! suppresses the nullable warning but there is still an unchecked direct cast (not as). If HandleCall is ever invoked through the object?[] path (e.g. via MockEngine.HandleCall(int, string, object?[])) with the wrong argument type, this will throw InvalidCastException rather than a descriptive mock error. This was the behavior before the PR too (the old closure args => callback((int)args[0]!) had the same issue), so this is not a regression. However, since TypedCallbackBehavior is a new type, it would be worth documenting that the Execute(object?[]) override assumes the caller guarantees type correctness — or at minimum, wrapping the cast in a try/catch with a better error message. This is a low-priority concern given these are [EditorBrowsable(Never)] internal types.

4. The allNonOutParams is null condition in MockMembersBuilder is implicit and fragile

if (allNonOutParams is null && nonOutParams.Count <= 8)
{
    writer.AppendLine("EnsureSetup().Callback(callback);");
}

The allNonOutParams parameter being null is used as a sentinel meaning "this is the primary overload, not a generic-subset overload for a method with more parameters". This is an implicit contract between the caller and GenerateTypedCallbackOverload. A named boolean parameter (bool isDirectRegistration or a dedicated enum) would make the intent explicit. Alternatively, a brief comment explaining why null means "primary overload" would significantly reduce future confusion. The current behavior is correct — the logic holds — but the convention is easy to misread.

5. Missing Func<T1, TReturn> typed return behaviors (opportunity gap)

The PR eliminates ToArray() allocations for all behaviors that don't use arguments (IArgumentFreeBehavior) and for callbacks that use arguments (ITypedBehavior<T...>). The ComputedReturnWithArgsBehavior (which holds a Func<object?[], TReturn>) is not covered by this PR. If a user sets up .Returns(args => (string)args[0]), the old store.ToArray() allocation still occurs. This is a natural extension of the same pattern — a TypedReturnBehavior<T1, TReturn> + ITypedReturnBehavior<T1, TReturn> approach — but is out of scope for this PR. Worth tracking as a follow-up.

---

Summary

The implementation is correct, well-tested (756 tests + 30 snapshot tests), and the benchmark numbers are compelling. The three-tier dispatch is the right architectural choice. The main code quality concern is the implicit null sentinel in MockMembersBuilder (#4 above), which a comment would resolve. Everything else is either pre-existing or a minor observation.

Verdict: Looks good to merge after addressing the source generator readability note.

[codacy-production]

Not up to standards ⛔

🔴 Issues 32 minor

Alerts:
⚠ 32 issues (≤ 0 issues of at least minor severity)

Results:
32 new issues

Category	Results
BestPractice	8 minor
CodeStyle	8 minor
Comprehensibility	16 minor

View in Codacy

🟢 Metrics 40 complexity

Metric	Results
Complexity	40

View in Codacy

_{TIP This summary will be updated as you push new changes. Give us feedback}

[claude]

Code Review: PR #5409 — Eliminate store.ToArray() Allocation on Mock Behavior Execution Hot Path

The benchmark numbers are impressive and the approach is architecturally sound. This is a well-executed micro-optimization PR. Below are my findings, ordered from most to least impactful.

---

1. [EditorBrowsable(Never)] on public typed Callback<T...> methods is the wrong tool here

The new Callback<T1>, Callback<T1,T2>, etc. overloads on MethodSetupBuilder and VoidMethodSetupBuilder are marked [EditorBrowsable(EditorBrowsableState.Never)]. The intent is to hide them from IntelliSense because they are only meant to be called by source-generated code. But [EditorBrowsable(Never)] does not prevent user code from calling them — it only suppresses the IntelliSense suggestion.

If the intent is "don't show this in IDE autocomplete but keep it callable from generated code", the attribute is correct. However, the summary XML doc is missing on these methods. Any user who does discover them will see no documentation. Either add a brief <summary> explaining the intended caller, or add a <remarks> tag noting these are emitted by the source generator.

---

2. ITypedBehavior<T...> interfaces are only implemented by TypedCallbackBehavior<T...> — the interface indirection may be unnecessary (but is worth keeping)

The ExecuteBehavior dispatch chain is:

b is IArgumentFreeBehavior af ? af.Execute()
  : b is ITypedBehavior<T1> tb ? tb.Execute(a1)
  : b.Execute(store.ToArray())

Currently ITypedBehavior<T1> is only ever implemented by TypedCallbackBehavior<T1>. The interface approach is forward-looking (e.g., for the ComputedReturnWithArgsBehavior follow-up mentioned in the PR), which is the right design. However, this should be documented so a future developer doesn't simplify it away.

Recommendation: Add a comment in TypedCallbackBehavior.cs explaining the interface exists to allow future typed return behaviors to participate in the same dispatch.

---

3. ⚠️ allNonOutParams is null sentinel is undocumented — most important readability issue

In MockMembersBuilder.cs:

if (allNonOutParams is null && nonOutParams.Count <= 8)
{
    // Direct typed registration
    writer.AppendLine("EnsureSetup().Callback(callback);");
}
else
{
    var castArgs = BuildCastArgs(nonOutParams, allNonOutParams);
    writer.AppendLine($"EnsureSetup().Callback(args => callback({castArgs}));");
}

The null value of allNonOutParams is used as an implicit sentinel meaning "this method is not a subset overload of a wider method". This contract between the call site and GenerateTypedCallbackOverload is fragile — a future developer adding a new call path might pass null inadvertently and silently enable the direct-registration path for a method that actually has out parameters.

Fix: Add a comment: // null means this is the primary (non-subset) overload — safe for direct typed registration

This is the one change I'd ask for before merging.

---

4. Execute(object?[]) in TypedCallbackBehavior uses unchecked direct casts

// TypedCallbackBehavior<T1, T2>
public object? Execute(object?[] arguments) { callback((T1)arguments[0]!, (T2)arguments[1]!); return null; }

If arguments has fewer elements than expected, this throws IndexOutOfRangeException with no context. This is not a regression (the old lambda-based approach had the same issue), but since TypedCallbackBehavior is new in this PR, it's a good moment to add a Debug.Assert(arguments.Length >= N, ...) for arity-N behaviors. Zero cost in release builds, much clearer test failures.

---

5. ComputedRawReturnWithArgsBehavior asymmetry is worth a comment

• RawReturnBehavior — now implements IArgumentFreeBehavior ✓
• ComputedRawReturnBehavior — now implements IArgumentFreeBehavior ✓
• ComputedRawReturnWithArgsBehavior — unchanged ✗

Expected, and noted in the PR as a follow-up. A comment on ComputedRawReturnWithArgsBehavior marking it as a candidate for a future ITypedBehavior-based optimization would help track this.

---

Summary

Item	Severity	Type
allNonOutParams is null sentinel undocumented	Moderate	Readability
[EditorBrowsable(Never)] methods lack XML docs	Minor	Documentation
ITypedBehavior interface purpose not documented	Minor	Documentation
Execute(object?[]) arity not bounds-checked	Low	Correctness (debug)
ComputedRawReturnWithArgsBehavior asymmetry	Info	Follow-up tracking

The implementation is correct, the test coverage is good, and the performance improvements are real and well-measured. The only change I'd ask for before merging is a comment on the allNonOutParams is null condition in MockMembersBuilder.cs — everything else is optional quality-of-life.

Approved with minor note.

[claude]

Performance Optimization Review

This is a well-structured performance PR with clear benchmarks and a logical three-tier dispatch design. The approach is sound and the gains are impressive. Here are my observations:

---

Design: IArgumentFreeBehavior is internal but IBehavior is public

IBehavior is public (needed for generated code) but IArgumentFreeBehavior is internal. This means third-party custom behaviors that don't use arguments can never participate in the fast path — they'll always fall through to Execute(object?[]>) and incur the allocation. If a library consumer implements their own IBehavior, they have no way to opt-in.

Consider making IArgumentFreeBehavior public (possibly [EditorBrowsable(EditorBrowsableState.Never)] to hide it from casual discovery), for the same reason IBehavior is public. Alternatively, document clearly that IArgumentFreeBehavior is intentionally internal-only, so the design choice is visible to maintainers.

---

The store parameter in ExecuteBehavior helpers is unused in two of three branches

private static object? ExecuteBehavior<T1>(IBehavior b, ArgumentStore<T1> store, T1 a1)
    => b is IArgumentFreeBehavior af ? af.Execute() :
       b is ITypedBehavior<T1> tb ? tb.Execute(a1) :
       b.Execute(store.ToArray());

The store parameter is only used in the fallback branch. Since ArgumentStore<T> is a readonly struct, passing it by value is fine — but in the common cases (the fast paths), the compiler still has to copy the struct onto the call frame. For 8-arg overloads that struct is 8 fields wide. This is unlikely to matter in practice given AOT and inlining, but if you want to be maximally allocation-free you could pass by in:

private static object? ExecuteBehavior<T1>(IBehavior b, in ArgumentStore<T1> store, T1 a1)

This avoids any defensive copies on the caller side when the struct has no ref-returning members. It's a minor point but consistent with the allocation-free philosophy of the PR.

---

Source generator: the allNonOutParams is null && nonOutParams.Count <= 8 guard is a hidden invariant

In MockMembersBuilder.GenerateTypedCallbackOverload:

if (allNonOutParams is null && nonOutParams.Count <= 8)
{
    writer.AppendLine("EnsureSetup().Callback(callback);");
}
else
{
    var castArgs = BuildCastArgs(nonOutParams, allNonOutParams);
    writer.AppendLine($"EnsureSetup().Callback(args => callback({castArgs}));");
}

The <= 8 limit silently mirrors the typed overload cap in MethodSetupBuilder. If someone later raises the cap in MethodSetupBuilder.cs (adding Callback<T1...T9>), the generator won't automatically emit the fast path for 9-arg methods — it will silently fall back to the closure form, with no compiler error or test failure.

A safer approach would be to define a constant or static readonly field (e.g. MaxTypedCallbackArity = 8) shared between or referenced from the generator, so the two are kept in sync explicitly. At minimum, a comment like // Must match number of typed Callback<T...> overloads in MethodSetupBuilder would make the coupling visible.

---

[EditorBrowsable(EditorBrowsableState.Never)] on public typed Callback<T...> overloads

The new typed Callback<T1>, Callback<T1,T2>, etc. overloads on MethodSetupBuilder and VoidMethodSetupBuilder are all marked [EditorBrowsable(EditorBrowsableState.Never)]. This is intentional (they're implementation details meant to be called only by generated code), and matches how other infrastructure methods in this codebase are hidden. Good pattern — just confirming this is intentional and not accidentally suppressing genuinely useful public API.

---

Overall

The approach is clean and the benchmark results are compelling — 38–56% faster invocations and 39–44% allocation reduction across the board. The three-tier dispatch hierarchy (IArgumentFreeBehavior → ITypedBehavior<T...> → object?[] fallback) is the right design given the need to maintain backward compatibility via IBehavior. The ArgumentStore lazy-boxing pattern and the source generator changes to emit direct typed callbacks are both consistent with TUnit's allocation-first design philosophy.

The main actionable concern is the magic constant <= 8 in the source generator that creates invisible coupling to the runtime arity cap. Everything else is minor polish.

[claude]

Code Review: perf: eliminate store.ToArray() allocation on mock behavior execution hot path

Overall: This is a well-targeted, measurably beneficial performance PR. The benchmarks are credible, the approach is principled, and the test coverage is solid. The following focuses on design concerns and correctness edge cases.

---

1. IArgumentFreeBehavior is internal but IBehavior is public — asymmetric abstraction

IBehavior is public (required because generated code calls it). IArgumentFreeBehavior is internal. This means a third-party behavior implementation that correctly implements IBehavior will silently fall through to the store.ToArray() fallback forever, with no way to opt into the fast path.

If IArgumentFreeBehavior were made public (it has no problematic surface area — just object? Execute()), external behaviors could participate. Alternatively, XML docs on IBehavior should explicitly note that IArgumentFreeBehavior exists but is internal, so future API evolution is a conscious decision.

---

2. allNonOutParams is null as a signal is fragile in the source generator

File: TUnit.Mocks.SourceGenerator/Builders/MockMembersBuilder.cs ~line 442

The fast path is gated on allNonOutParams is null, where null signals "primary overload (no out/ref struct subset remapping)." However, the semantics of null are not enforced anywhere — they depend on every current and future caller of GenerateTypedCallbackOverload remembering this convention. If a future call site passes null inadvertently, the generated code silently registers the callback against the wrong typed overload — a correctness bug with no compile-time guard.

Suggested fix: Use a named boolean parameter (bool isPrimaryOverload) or an enum to make the intent and its safety invariant unmistakable. Also, the <= 8 arity guard would benefit from a named constant (const int MaxTypedCallbackArity = 8) mirroring the boundary in TypedCallbackBehavior.cs.

---

3. TypedCallbackBehavior<T...>.Execute(object?[]) uses Debug.Assert instead of real bounds checking

File: TUnit.Mocks/Setup/Behaviors/TypedCallbackBehavior.cs

The fallback Execute(object?[] arguments) path uses Debug.Assert(arguments.Length >= N), which is stripped in Release builds. If a mismatched arguments array ever reaches this path, the result is a bare IndexOutOfRangeException rather than a clear, diagnostic error.

Since this is the fallback (not the hot path), a real bounds check or a descriptive throw has negligible cost. The Debug.Assert adds no real safety and sets a misleading precedent.

// Instead of:
Debug.Assert(arguments.Length >= 2);

// Prefer:
if (arguments.Length < 2)
    throw new ArgumentException($"Expected at least 2 arguments, got {arguments.Length}.", nameof(arguments));

---

4. store struct is copied on every ExecuteBehavior call, even when unused

File: TUnit.Mocks/MockEngine.Typed.cs

The signatures ExecuteBehavior<T1>(IBehavior b, ArgumentStore<T1> store, T1 a1) pass store by value. For IArgumentFreeBehavior and ITypedBehavior<T1> — the common cases — store is copied but never accessed. For arity-8 this is 8 field copies on each hot-path invocation.

Consider passing store by in (readonly ref) or restructuring to only construct/pass the store when the fallback path is actually taken. This would reclaim the small but real cost on the fast path.

---

5. Duplicate boilerplate across MethodSetupBuilder and VoidMethodSetupBuilder

The 8 typed Callback<T1,...> overloads are copy-pasted identically into both builders, with the only difference being the return type. The existing Returns, Throws, etc. share this pattern, so this is not new debt, but 16 new overloads make it more acute.

At minimum, a code comment ("keep in sync with VoidMethodSetupBuilder") would prevent silent drift. A shared internal extension or abstract base method is the architectural fix if this pattern continues to grow.

---

6. ThrowBehavior reuses the same exception instance on every call (pre-existing, newly visible)

File: TUnit.Mocks/Setup/Behaviors/ThrowBehavior.cs

This was pre-existing behavior, but the IArgumentFreeBehavior fast path now routes all ThrowBehavior calls through Execute() more explicitly. When .Throws(new SomeException()) is used and the mocked method is called multiple times, the same exception instance (with the original stack trace from setup time) is thrown every time, which can make test failure messages confusing.

This is a good moment to decide whether a fresh exception should be created per-call (e.g. by accepting a factory Func<Exception> instead).

---

7. No test coverage for the TypedCallbackBehavior.Execute(object?[]) fallback path

The fallback Execute(object?[] arguments) on each arity variant is not directly tested. If someone constructs a TypedCallbackBehavior<string> and the arity/type at dispatch time doesn't match the typed fast path, the fallback runs — but its Debug.Assert + cast logic has no regression coverage. A direct unit test for this path would catch future breakage.

---

Summary

Concern	Severity
Fragile null signal for "primary overload" in source generator	Medium
Debug.Assert instead of real validation in fallback path	Medium
IArgumentFreeBehavior not extensible to third-party behaviors	Low/design
store struct copied on every ExecuteBehavior call	Low/perf
16 duplicate overloads across two builders	Low/maintenance
ThrowBehavior reuses same exception instance per call	Low/UX
No test for TypedCallbackBehavior.Execute(object?[]) fallback	Low

The most actionable items before merge are #2 (fragile null signal) and #3 (replace Debug.Assert with a real guard). The others could be addressed in follow-up work. The core optimization is sound and the approach is well-executed.

[claude]

Code Review

Excellent performance work. The three-tier dispatch (IArgumentFreeBehavior → ITypedBehavior<T...> → fallback ToArray) is a clean, extensible design, and the benchmark numbers speak for themselves — beating Imposter on all invocation benchmarks is a meaningful milestone.

Prior Review Items — Status

Issue	Status
allNonOutParams is null sentinel needs explanation	✅ Fixed — comment added
store copied on every ExecuteBehavior call	✅ Fixed — now passed in
Debug.Assert stripped in Release builds	❌ Still unaddressed
Missing unit tests for TypedCallbackBehavior.Execute(object?[]) fallback	❌ Still unaddressed

---

Remaining Issue: Debug.Assert in TypedCallbackBehavior.Execute(object?[])

This is the one item from prior reviews that still needs attention.

All 8 TypedCallbackBehavior<T...> classes use Debug.Assert(arguments.Length >= N) as the only guard in the Execute(object?[]) fallback path:

public object? Execute(object?[] arguments) { Debug.Assert(arguments.Length >= 1); callback((T1)arguments[0]!); return null; }

In Release builds, Debug.Assert is stripped entirely. A caller that reaches this path with the wrong arity gets an IndexOutOfRangeException with no context — no indication that the arity mismatch is the cause.

Why this matters: While TypedCallbackBehavior is internal and the source generator always emits the typed fast path, the Execute(object?[]) method is a required IBehavior contract member and can be called if anyone iterates the behavior store and calls it directly (e.g. a third-party extension or future engine code). The assertion gives a false sense of safety.

Suggested fix — use ArgumentOutOfRangeException.ThrowIfLessThan (available since .NET 8, which is TUnit's minimum target):

public object? Execute(object?[] arguments) 
{ 
    ArgumentOutOfRangeException.ThrowIfLessThan(arguments.Length, 1);
    callback((T1)arguments[0]!); 
    return null; 
}

This is a one-line change per class and provides a clear, actionable error in both Debug and Release.

---

Minor Observation: Duplicate Callback<T...> Overloads

The 8 typed Callback<T...> overloads are duplicated across MethodSetupBuilder and VoidMethodSetupBuilder (+57 lines each). A prior review suggested adding a sync comment — this would help the next person who touches one of the two builders remember to update the other. Not a blocker, but worth tracking.

---

Overall this is a well-executed, well-tested optimization with clear motivation. The only actionable item before merge is replacing the Debug.Assert guards with real release-mode guards.