perf(mocks): optimize MockEngine for lower allocation and faster verification

[thomhurst]

Summary

• Flat array-based setup storage — replaces Dictionary<int, MethodSetup[]> with MethodSetup[]?[] indexed by member ID (dense sequential ints 0..N), giving O(1) lookup instead of dictionary hashing
• Per-member call index and counters — replaces linear scan of ConcurrentQueue<CallRecord> with List<CallRecord>?[] per-member index and int[] counters, enabling O(1) call count verification
• Lazy initialization — Lock, call history, and per-member arrays are only allocated when first needed, saving ~64B per mock that is created but never invoked
• O(1) verification fast-path — CallVerificationBuilder skips argument matching entirely when _matchers.Length == 0, using the per-member counter directly
• Deduplicated OutRef+Events pattern — extracted ApplyMatchedSetup helper, removing 8 duplicate code blocks across the four Handle methods

Test plan

• All 701 existing + new tests pass (dotnet run --project TUnit.Mocks.Tests --framework net10.0)
• 22 new tests in PerformanceOptimizationTests.cs covering:
  • Per-member call counting and indexing
  • Flat array setup storage with multiple members
  • Fast-path vs matcher-path verification
  • Thread safety with concurrent calls to multiple members
  • Reset clearing new data structures
  • Edge cases (no setup, no calls, multiple reset cycles)
• Benchmarks project builds successfully
• Source generator tests pass (22/22)

[claude]

Code Review: perf(mocks) - MockEngine Optimizations

Overall this is a solid performance improvement — the move from dictionary-based setup storage to flat arrays, per-member counters for O(1) verification, and the ApplyMatchedSetup refactor are all well-motivated. 22 new tests provide good coverage. A few issues worth addressing:

---

Bug: ApplyMatchedSetup silently drops OutRefContext.Set(null)

File: TUnit.Mocks/MockEngine.cs

The original code always called OutRefContext.Set(matchedSetup?.OutRefAssignments) — even when matchedSetup was null. This matters in reentrant scenarios: if a behavior's Execute() triggers another mock call that sets OutRefContext, the outer call's post-Execute Set was responsible for restoring or clearing it.

The new helper:

private void ApplyMatchedSetup(MethodSetup? matchedSetup)
{
    if (matchedSetup is null) return;  // ← skips Set(null)
    if (matchedSetup.OutRefAssignments is { } outRef)
        OutRefContext.Set(outRef);
    RaiseEventsForSetup(matchedSetup);
}

If matchedSetup is null (or its OutRefAssignments is null), the thread-local is no longer cleared. In the typical non-reentrant flow this is fine because Consume() always clears it. But if a callback in Execute() makes a reentrant mock call that sets OutRefContext, the outer call's post-Execute ApplyMatchedSetup no longer overwrites that. The fix is minimal:

private void ApplyMatchedSetup(MethodSetup? matchedSetup)
{
    OutRefContext.Set(matchedSetup?.OutRefAssignments);  // always clear or set
    if (matchedSetup is null) return;
    RaiseEventsForSetup(matchedSetup);
}

Why this matters: The original comment "Set out/ref assignments after Execute to avoid reentrancy overwrite from callbacks" documents exactly this invariant, and the refactor quietly removed it.

---

Design concern: WasCalled fast-path has a TOCTOU gap

File: TUnit.Mocks/Verification/CallVerificationBuilder.cs

The fast-path reads the count lock-free, then acquires the lock to mark calls verified:

var totalCount = _engine.GetCallCountFor(_memberId);   // 1. no lock
if (times.Matches(totalCount))
{
    var allCallsForMember = _engine.GetCallsFor(_memberId); // 2. acquires lock
    for (int i = 0; i < allCallsForMember.Count; i++)
        allCallsForMember[i].IsVerified = true;
}

Between steps 1 and 2, additional calls can be recorded. The count used to validate Times and the list used to mark verified are inconsistent snapshots. For VerifyNoOtherCalls() this could cause a spurious failure: extra calls recorded after step 1 won't be marked verified and will appear as "other calls".

In normal single-threaded test flow this is benign, but the PR explicitly tests concurrent verification (Concurrent_Verification_With_Per_Member_Counters), so thread safety matters. Consider taking the lock for the entire fast-path read+mark, or documenting that verification is only safe after all calls have completed.

---

Misleading test comments about "fast path"

File: TUnit.Mocks.Tests/PerformanceOptimizationTests.cs

Several tests claim to cover the fast path but won't actually exercise it:

// Assert — no-matcher verification (fast path)
mock.Add(Any(), Any()).WasCalled(Times.Exactly(3));  // _matchers.Length == 2, NOT 0

The fast path fires when _matchers.Length == 0, which only applies to zero-parameter methods. mock.GetName().WasCalled(...) would genuinely hit it; mock.Add(Any(), Any()).WasCalled(...) does not. This could mislead future developers into thinking the fast path is being covered when it isn't. Adding a dedicated test for a zero-parameter method, and fixing the comments, would make the intent clear.

---

Minor: EnsureCallArrayCapacity growth starts at 8 and doesn't double

var newSize = Math.Max(required, 8);  // same in both EnsureSetupArrayCapacity and EnsureCallArrayCapacity

If an interface has 9 members and all are used, the first expansion goes to 9. The second call with memberId=9 would copy again to 10, etc. For large interfaces, consider exponential growth (Math.Max(required, _callsByMemberId.Length * 2)) to avoid repeated O(n) copies. This mirrors the standard List<T> growth policy.

---

Positive observations

• ApplyMatchedSetup refactoring is excellent — removing 8 duplicated OutRef+Events blocks is a meaningful improvement to maintainability.
• Lazy Lock initialization in both MockEngine and MethodSetup is a nice touch, saving real allocations for mocks that are created but never set up concurrently.
• volatile MethodSetup[]?[]? for the setup snapshot (lock-free reads in FindMatchingSetup) paired with guarded mutable lists is a clean two-layer design.
• Test coverage is comprehensive — concurrent scenarios, reset cycles, and edge cases are all represented.

[claude]

Follow-up Review: Second Commit (61d09d6)

The second commit adds MarkCallsVerified to avoid the ToArray() allocation in the fast-path. Good improvement. Comparing against my previous review, here is the status of each issue:

---

Previously raised: ApplyMatchedSetup drops OutRefContext.Set(null) — not addressed

The second commit did not touch this. The original code unconditionally called OutRefContext.Set(matchedSetup?.OutRefAssignments) (passing null when there was no setup), which would clear the thread-local. The refactored helper still short-circuits on null before calling Set:

private void ApplyMatchedSetup(MethodSetup? matchedSetup)
{
    if (matchedSetup is null) return;  // still skips Set(null)
    if (matchedSetup.OutRefAssignments is { } outRef)
        OutRefContext.Set(outRef);
    RaiseEventsForSetup(matchedSetup);
}

The invariant documented in the original comments ("Set out/ref assignments after Execute to avoid reentrancy overwrite from callbacks") is still broken. The minimal fix:

private void ApplyMatchedSetup(MethodSetup? matchedSetup)
{
    OutRefContext.Set(matchedSetup?.OutRefAssignments);  // always clear or set, even for null
    if (matchedSetup is null) return;
    RaiseEventsForSetup(matchedSetup);
}

---

Previously raised: TOCTOU in fast-path — partially addressed

MarkCallsVerified now holds the lock while iterating, which is better. But the count read is still lock-free:

var totalCount = _engine.GetCallCountFor(_memberId);   // lock-free Volatile.Read
if (times.Matches(totalCount))
    _engine.MarkCallsVerified(_memberId);               // acquires Lock internally

Between the count read and the MarkCallsVerified call, new calls can be recorded. The count used for Times validation and the calls actually marked verified are still inconsistent snapshots. For a strict Times.Exactly(N) check followed by VerifyNoOtherCalls(), an extra concurrent call slipping in could cause a spurious failure.

This is acceptable if verification is documented as "only safe after all concurrent callers have completed" — but the existing test Concurrent_Verification_With_Per_Member_Counters calls WasCalled while no new calls are being made, so it does not actually stress this gap. Consider either: (a) holding the lock for the full count+mark, or (b) adding a comment acknowledging the intentional trade-off.

---

Previously raised: misleading "fast path" test comments — not addressed

mock.Add(Any(), Any()).WasCalled(...) creates a CallVerificationBuilder with _matchers.Length == 2, so it always takes the slow path. The tests labelled "fast path" do not exercise it. mock.GetName().WasCalled(Times.Once) (zero parameters) would genuinely hit the fast path. This is a test-accuracy issue rather than a correctness bug.

---

Previously raised: array growth strategy — not addressed

EnsureCallArrayCapacity and EnsureSetupArrayCapacity both grow to Math.Max(required, 8) rather than doubling. For interfaces with many members accessed sequentially, each new high-water-mark triggers a full copy. Consider Math.Max(required, existing * 2) to match List<T> amortized behavior.

---

What the second commit does well

MarkCallsVerified eliminates the ToArray() allocation in the fast-path success case — a clean targeted fix that avoids the double-fetch pattern from the first commit. The lock acquisition inside MarkCallsVerified is the right place for it.

---

Summary: The OutRefContext.Set(null) regression from the first commit is the only behavioral correctness risk. The rest are design/documentation concerns. Would recommend addressing that one before merge.

[claude]

Code Review: perf(mocks): optimize MockEngine for lower allocation and faster verification

The overall direction here is solid — replacing Dictionary + ConcurrentQueue with dense arrays and per-member counters is a well-motivated optimization for a mocking library where mock objects are frequently created/destroyed. The ApplyMatchedSetup refactor is a clear improvement in readability.

That said, I found a few issues worth addressing.

---

Bug: TOCTOU race in GetAllCalls, GetUnverifiedCalls, and GetDiagnostics

The null-guard for _callHistory is performed outside the lock, but Reset() can null it inside the lock. This creates a window where _callHistory passes the null check, Reset() runs and nulls it, then the code inside the lock dereferences it and throws NullReferenceException.

// MockEngine.cs — current (racy)
public IReadOnlyList<CallRecord> GetAllCalls()
{
    if (_callHistory is null) return [];   // ← passes null check
    lock (Lock)
    {
        // Reset() may have run here — _callHistory is now null
        return _callHistory.ToArray();     // ← NullReferenceException
    }
}

The same pattern appears in GetUnverifiedCalls and the _callHistory block in GetDiagnostics.

Fix: Move the null check inside the lock:

public IReadOnlyList<CallRecord> GetAllCalls()
{
    lock (Lock)
    {
        return _callHistory is null ? [] : _callHistory.ToArray();
    }
}

---

Bug: ApplyMatchedSetup skips OutRefContext.Set(null) for unmatched calls

The original code always called OutRefContext.Set(matchedSetup?.OutRefAssignments) — which means it called OutRefContext.Set(null) even when no setup matched. The new ApplyMatchedSetup early-returns on null, so OutRefContext.Set is never called for unmatched calls.

If OutRefContext is thread-static (or an ambient context), a previous mock call on the same thread that set out/ref values will leave stale values that could bleed into subsequent unmatched calls. Test isolation depends on the context being cleared between calls.

Why this matters: In tight test loops or when ThreadPool threads are reused, the same thread may handle multiple mock invocations. Stale out/ref context from a previous setup could silently produce wrong out/ref values.

Suggested fix: Preserve the original clearing behaviour for null setups:

private void ApplyMatchedSetup(MethodSetup? matchedSetup)
{
    OutRefContext.Set(matchedSetup?.OutRefAssignments); // always clear/set
    if (matchedSetup is null) return;
    RaiseEventsForSetup(matchedSetup);
}

---

Design note: Consolidating setup lock and call-recording lock

Merging _setupLock and the previous lock-free ConcurrentQueue under a single Lock is fine for the common case (setup before run, calls during run), but the comment says contention is "negligible" — the new test Concurrent_Setup_Then_Calls_Across_Members intentionally exercises concurrent setup and call recording, which is the exact contention scenario. For most users this won't matter, but the claim in the comment is slightly misleading. Consider wording it as "acceptable" rather than "negligible."

---

Minor: Test comments hardcode assumed internal member ID ordering

// Arrange — ICalculator has 3 members: Add (0), GetName (1), Log (2)

This documents an internal implementation detail (member ID assignment order) that is not part of the public contract. If the interface gains members or the source generator changes ordering, the comment becomes a trap. Since the tests verify observable behaviour (call counts, return values) without actually depending on the numeric IDs, consider removing these comments — the tests are self-explanatory without them.

---

Nit: Sentinel await Assert.That(true).IsTrue() assertions

Several tests end with:

await Assert.That(true).IsTrue();

This is a no-op assertion used to satisfy the async/await requirement, but it obscures the actual intent. If the test is "no exception was thrown," consider using Assert.That(mock.Invocations).HasCount().EqualTo(n) or similar meaningful assertion, or just remove the sentinel if a preceding WasCalled assertion already covers it.

---

What's working well

• The flat-array approach is idiomatic and the (uint)memberId < (uint)array.Length bounds trick for combined null/range check is a clean pattern.
• GetCallCountFor using Volatile.Read without a lock for the fast-path verification is correct and avoids contention on the common verification case.
• The EnsureLock lazy-init via Interlocked.CompareExchange is correct and the [MethodImpl(MethodImplOptions.NoInlining)] placement on the cold path is good practice.
• Test coverage for reset cycles, thread safety, and edge cases is thorough.

The TOCTOU race and the OutRefContext clearing change are the two items I'd want addressed before merge.

[claude]

Code Review

Good progress on this PR — the correctness regression raised in previous reviews has been fixed in the latest commit. now unconditionally calls OutRefContext.Set(matchedSetup?.OutRefAssignments) before the null guard, which correctly preserves the original clearing behaviour. That was the most important correctness concern, so nice work getting it addressed.

---

Still open: TOCTOU race in GetAllCalls, GetUnverifiedCalls, and GetDiagnostics

This was flagged in the previous review and remains unaddressed in the latest commit. The null-guard on _callHistory is performed outside the lock, but Reset() nulls it inside the lock:

// GetAllCalls — current code (racy)
if (_callHistory is null) return [];   // ← passes null check on Thread A
lock (Lock)
{
    // Thread B calls Reset() here — _callHistory is now null
    return _callHistory.ToArray();     // ← NullReferenceException
}

The same pattern is in GetUnverifiedCalls and GetDiagnostics. The fix is straightforward — move the null check inside the lock:

public IReadOnlyList<CallRecord> GetAllCalls()
{
    lock (Lock)
    {
        return _callHistory is null ? [] : _callHistory.ToArray();
    }
}

Why this matters: Reset() is explicitly tested in concurrent scenarios (e.g., Invocations_Per_Member_Independent_After_Reset), and diagnostic/verification calls can race with it. The three-instruction window is narrow but not theoretical.

---

Minor (still open): Sentinel await Assert.That(true).IsTrue() assertions

Several tests end with:

mock.GetName().WasCalled(Times.Exactly(3));
await Assert.That(true).IsTrue(); // ← no-op

The WasCalled call is the real assertion — the sentinel adds noise without adding meaning. Either use a genuinely meaningful final assertion (e.g., mock.Invocations count) or remove it.

---

Minor (still open): Internal member ID ordering in test comments

// Arrange — ICalculator has 3 members: Add (0), GetName (1), Log (2)

This documents an internal implementation detail (member ID assignment order) that the tests don't actually depend on. If the interface grows or the source generator changes ordering, these comments become misleading. Since the observable behaviour under test doesn't rely on numeric IDs, these comments can be removed without loss of clarity.

---

Summary

Issue	Status
OutRefContext.Set(null) skipped on unmatched calls	✅ Fixed in latest commit
Array growth not doubling	✅ Fixed in earlier commit
TOCTOU: null-guard outside lock in GetAllCalls / GetUnverifiedCalls / GetDiagnostics	⚠️ Still present
Sentinel Assert.That(true).IsTrue()	Minor — still present
Internal member ID comments in tests	Minor — still present

The flat-array approach, lazy lock init, Volatile.Read fast-path, and ApplyMatchedSetup refactor are all solid. The TOCTOU fix is the remaining blocker I'd want before merge.

[claude]

Code Review

Good progress on this PR — the OutRefContext.Set(null) correctness regression raised in previous reviews has been fixed in the latest commit. ApplyMatchedSetup now unconditionally calls OutRefContext.Set(matchedSetup?.OutRefAssignments) before the null guard, which correctly preserves the original clearing behaviour. That was the most important correctness concern, so nice work getting it addressed.

---

Still open: TOCTOU race in GetAllCalls, GetUnverifiedCalls, and GetDiagnostics

This was flagged in the previous review and remains unaddressed in the latest commit. The null-guard on _callHistory is performed outside the lock, but Reset() nulls it inside the lock:

// GetAllCalls — current code (racy)
if (_callHistory is null) return [];   // passes null check on Thread A
lock (Lock)
{
    // Thread B calls Reset() here — _callHistory is now null
    return _callHistory.ToArray();     // NullReferenceException
}

The same pattern appears in GetUnverifiedCalls and GetDiagnostics. The fix is straightforward — move the null check inside the lock:

public IReadOnlyList<CallRecord> GetAllCalls()
{
    lock (Lock)
    {
        return _callHistory is null ? [] : _callHistory.ToArray();
    }
}

Why this matters: Reset() is explicitly tested in concurrent scenarios, and diagnostic/verification calls can race with it. The three-instruction window is narrow but not theoretical.

---

Minor (still open): Sentinel await Assert.That(true).IsTrue() assertions

Several tests end with:

mock.GetName().WasCalled(Times.Exactly(3));
await Assert.That(true).IsTrue(); // no-op

The WasCalled call is the real assertion — the sentinel adds noise without meaning. Either use a genuinely meaningful final assertion (e.g., mock.Invocations count) or remove it.

---

Minor (still open): Internal member ID ordering in test comments

// Arrange — ICalculator has 3 members: Add (0), GetName (1), Log (2)

This documents an internal implementation detail (member ID assignment order) that the tests don't actually depend on. If the interface grows or the source generator changes ordering, these comments become misleading. They can be removed without loss of clarity.

---

Summary

Issue	Status
OutRefContext.Set(null) skipped on unmatched calls	Fixed in latest commit
Array growth not doubling	Fixed in earlier commit
TOCTOU: null-guard outside lock in GetAllCalls / GetUnverifiedCalls / GetDiagnostics	Still present
Sentinel Assert.That(true).IsTrue()	Minor — still present
Internal member ID comments in tests	Minor — still present

The flat-array approach, lazy lock init, Volatile.Read fast-path, and ApplyMatchedSetup refactor are all solid. The TOCTOU fix is the remaining blocker I'd want before merge.

[claude]

Code Review — Final Pass

The last commit () moves the _callHistory null checks inside the lock in GetAllCalls, GetUnverifiedCalls, and GetDiagnostics, which correctly resolves the TOCTOU race raised in every prior review. All three correctness issues flagged across the review cycle are now addressed:

Issue	Status
OutRefContext.Set(null) skipped on unmatched calls	Fixed in 5dbe7005
Array growth not doubling	Fixed in 61d09d68 / initial commit
TOCTOU: null-guard outside lock in GetAllCalls / GetUnverifiedCalls / GetDiagnostics	Fixed in 3d3b4c9a

---

Two minor items still open (non-blocking)

Sentinel await Assert.That(true).IsTrue() assertions

Still present at lines 169, 189, 207, 217, 228, 308, 329, 364 of PerformanceOptimizationTests.cs. These are no-ops — the preceding WasCalled or WasNeverCalled calls are the real assertions. They can be removed or replaced with a meaningful assertion (e.g. await Assert.That(mock.Invocations).HasCount().EqualTo(n)) without changing test semantics.

Internal member ID ordering in test comments

Still present (e.g. // ICalculator has 3 members: Add (0), GetName (1), Log (2)). These document source-generator internals the tests don't actually depend on. They're harmless today but will quietly become wrong if the interface or generator changes.

---

Overall

The PR is in good shape. The flat-array setup storage, per-member call counters, lazy Lock init, Volatile.Read fast-path, and ApplyMatchedSetup consolidation are all solid improvements. The two minor items above can be addressed in a follow-up or cleaned up at merge time — neither affects correctness.

[claude]

Code Review — Final Pass

The last commit (3d3b4c9a) moves the _callHistory null checks inside the lock in GetAllCalls, GetUnverifiedCalls, and GetDiagnostics, which correctly resolves the TOCTOU race raised in every prior review. All three correctness issues flagged across the review cycle are now addressed:

Issue	Status
OutRefContext.Set(null) skipped on unmatched calls	Fixed in 5dbe7005
Array growth not doubling	Fixed earlier
TOCTOU: null-guard outside lock in GetAllCalls / GetUnverifiedCalls / GetDiagnostics	Fixed in 3d3b4c9a

---

Two minor items still open (non-blocking)

Sentinel await Assert.That(true).IsTrue() assertions

Still present at lines 169, 189, 207, 217, 228, 308, 329, 364 of PerformanceOptimizationTests.cs. These are no-ops — the preceding WasCalled or WasNeverCalled calls are the real assertions. They can be removed or replaced with a meaningful assertion (e.g. await Assert.That(mock.Invocations).HasCount().EqualTo(n)) without changing test semantics.

Internal member ID ordering in test comments

Still present (e.g. // ICalculator has 3 members: Add (0), GetName (1), Log (2)). These document source-generator internals the tests don't actually depend on. They're harmless today but will quietly become wrong if the interface or generator changes.

---

Overall

The PR is in good shape. The flat-array setup storage, per-member call counters, lazy Lock init, Volatile.Read fast-path, and ApplyMatchedSetup consolidation are all solid improvements. The two minor items above can be addressed in a follow-up or cleaned up at merge time — neither affects correctness.