perf(mocks): shrink MethodSetup + cache stateless matchers

[thomhurst]

Summary

Reduces allocations in the mock setup/verify/callback hot paths with no API changes.

• Hoist rare MethodSetup fields (event raises, out/ref assignments, state-machine strings, behavior lock) into a lazy RareState side-class. Saves ~40B per setup on the common path (single behavior, no events, no out/ref, no state machine).
• CAS-based lazy init in generated EnsureSetup: replaces LazyInitializer.EnsureInitialized(…, Func<T>) with an Interlocked.CompareExchange fast path. Drops the per-wrapper Func closure and two scratch fields (_builderInitialized, _builderLock).
• Cache stateless matchers as singletons: AnyMatcher<T>, NullMatcher<T>, NotNullMatcher<T>, and EmptyMatcher now expose static readonly Instance. Arg.Any<T>() / IsNull<T>() / IsNotNull<T>() / IsEmpty<T>() reuse these — one instance per closed T instead of one per call.

Estimated per-iteration allocation savings on mock benchmarks (rough):

• Callback (2 matchers): ~80B
• Setup (6 matchers across 3 methods): ~180B
• Verification (6 matchers): ~150B

No hot-path changes; no public API changes.

Test plan

• TUnit.Mocks.Tests full suite passes (904/904 on net10.0)
• Nightly mock benchmark workflow confirms alloc reduction with no time regression

[codacy-production]

Not up to standards ⛔

🔴 Issues 3 minor

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

Results:
3 new issues

Category	Results
CodeStyle	3 minor

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes. Give us feedback}

[claude]

Code Review — perf(mocks): shrink MethodSetup + cache stateless matchers

This is a well-structured allocation-reduction PR with three distinct, independent optimizations. The approach is sound overall. Here are the key observations:

---

Singleton matchers ✅

AnyMatcher<T>.Instance, NullMatcher<T>.Instance, NotNullMatcher<T>.Instance, and EmptyMatcher.Instance are textbook correct: all four are sealed, stateless, and internal. One instance per closed type (or singleton for the non-generic EmptyMatcher) is exactly right. The Arg<T> implicit operator for AnyArg is also updated consistently. No issues here.

---

RareState lazy allocation ✅ (with one note)

The nested class design cleanly separates hot-path fields (behaviors, invoke count) from rarely-used fields (events, out/ref, state machine strings, behavior lock). Saving ~40 bytes per MethodSetup on the common path is meaningful at scale.

Minor: non-volatile inner field writes on RequiredState/TransitionTarget

The getters use Volatile.Read(ref _rareState)?.RequiredState — the outer pointer is read with acquire semantics, but the inner field read is a plain load. The setters (EnsureRareState().RequiredState = value) have no corresponding Volatile.Write. On a weakly-ordered CPU (ARM), a thread reading via the getter could see a non-null _rareState but a stale RequiredState value.

In practice this is safe because state machine setup runs sequentially before tests execute. But given that the rest of the class is carefully volatile-annotated, adding Volatile.Write in the setter and reading the inner field with Volatile.Read would be consistent:

// setter
set
{
    var rare = EnsureRareState();
    Volatile.Write(ref rare.RequiredState, value);
}

// getter
get => Volatile.Read(ref _rareState) is { } r ? Volatile.Read(ref r.RequiredState) : null;

---

CAS-based lazy init in EnsureSetupSlow ⚠️ — subtle post-CAS ordering window

The generated code:

private {builderType} EnsureSetupSlow()
{
    var setup = new MethodSetup(_memberId, _matchers, _memberName);
    var fresh = new {builderType}(setup);
    var prev = Interlocked.CompareExchange(ref _builder, fresh, null);
    if (prev is not null) return prev;   // ← Thread B exits here...
    _engine.AddSetup(setup);             // ← ...before Thread A reaches this
    return fresh;
}

When Thread B loses the CAS, it returns prev (Thread A's builder) immediately — before Thread A has called _engine.AddSetup(setup). The old LazyInitializer.EnsureInitialized with a Func<T> and its internal double-checked lock would have blocked Thread B until Thread A's factory completed, ensuring the setup was registered before any caller could use the builder.

The new CAS approach changes this contract: EnsureSetupSlow now returns a builder whose underlying MethodSetup may not yet be registered with the engine. If Thread B immediately invokes EnsureSetup().Returns(...) to add behaviors and Thread A hasn't finished AddSetup yet, the engine won't know about the setup at verify time.

Practical risk: Mock setup is typically sequential per method, so this race likely never manifests. But it's a behavioral change that deserves explicit documentation.

Suggested fix: Add a comment to EnsureSetupSlow acknowledging the assumption and the reason it's safe:

// _engine.AddSetup is called only by the CAS winner; the loser returns the winner's
// builder. This is safe because mock setup is sequential — the same method-wrapper
// is not configured concurrently in normal usage.

Alternatively, register setup with the engine before the CAS (accepting that a losing thread may have briefly registered a setup that gets discarded — which requires AddSetup to be idempotent or for the engine to de-register on CAS loss).

---

Stale comment in MockMembersBuilder.cs 🔧

// EnsureSetup method — uses LazyInitializer to guarantee exactly-once AddSetup
EmitEnsureSetup(writer, builderType);

This comment appears twice and is now incorrect — LazyInitializer was replaced by CAS. Should be updated to reflect the actual approach.

---

BehaviorLock — inner field not read with volatile 📝

private Lock BehaviorLock => Volatile.Read(ref _rareState)?.BehaviorLock ?? EnsureBehaviorLock();

The outer _rareState is read with acquire semantics, but .BehaviorLock is a plain field read on the resulting object. On ARM, a thread could observe _rareState as non-null (from someone calling EnsureRareState for a different reason, e.g., RequiredState) while BehaviorLock is still null. This causes EnsureBehaviorLock() to be called, allocating and discarding a Lock object — counter to the PR's goal.

EnsureBehaviorLock itself correctly handles this via CAS, so correctness isn't broken, but it does generate extra allocations on ARM in multi-threaded scenarios. Using Volatile.Read(ref rare.BehaviorLock) in the fast path of EnsureBehaviorLock (or marking the field volatile) would close this.

---

Summary

Change	Assessment
Singleton matchers	✅ Clean, correct
RareState hoisting	✅ Good design with minor volatile consistency gap
CAS-based lazy init	⚠️ Correct for sequential setup; stale comment needs fixing; behavioral difference from LazyInitializer should be documented
BehaviorLock inner field	📝 Non-volatile inner read can cause extra allocations on ARM

The two must-fix items before merge are: update the stale comment and document the post-CAS ordering assumption in EnsureSetupSlow. The volatile gaps are lower priority but worth addressing for correctness at the stated level of care.

[claude]

Follow-up review — second commit addresses all prior feedback

The second commit (d23edf58) cleanly resolves every item from the earlier review. Here's a final pass:

All issues addressed ✅

Stale comments — both MockMembersBuilder.cs call sites now say // EnsureSetup — CAS-based lazy init (see EmitEnsureSetup). Fixed.

RequiredState / TransitionTarget volatile inner reads — getters now use Volatile.Read(ref rare.RequiredState) / Volatile.Read(ref rare.TransitionTarget); setters use Volatile.Write. The acquire/release pairing is consistent across the two levels of indirection. Fixed.

BehaviorLock inner field — fast path now reads Volatile.Read(ref rare.BehaviorLock) before falling through to EnsureBehaviorLock, which also issues a Volatile.Read after the CAS to guarantee the winner's write is visible to the loser on ARM. Fixed.

CAS ordering assumption — two comments in EmitEnsureSetup (one in the generator source, one emitted into generated code) document the sequential-setup assumption and point future maintainers toward re-introducing synchronization if the invariant ever breaks. Fixed.

One small nit (non-blocking)

EnsureRareState itself still uses a plain return _rareState! after the CAS rather than Volatile.Read(ref _rareState)!:

private RareState EnsureRareState()
{
    var existing = Volatile.Read(ref _rareState);
    if (existing is not null) return existing;
    Interlocked.CompareExchange(ref _rareState, new RareState(), null);
    return _rareState!;   // ← plain read; could be Volatile.Read for symmetry
}

In practice the CAS provides the necessary barrier so the winner's write is visible to itself, but replacing the last line with return Volatile.Read(ref _rareState)! would keep the pattern consistent with how EnsureBehaviorLock was updated in this very commit. Not a correctness issue — just a consistency nit.

Summary

The PR is in good shape. All correctness concerns from the first review have been addressed with precise fixes, and the code is now internally consistent in its volatile discipline. The allocation savings are real and the approach is sound. Ready to merge.

[claude]

Code Review — Third Pass

Following up on the two prior rounds of review. This pass checks that all previous feedback was addressed and gives a fresh look at the current diff.

Previous Issues — All Resolved ✅

Every item from the first two reviews has been addressed:

Prior concern	Status
Stale LazyInitializer comments in MockMembersBuilder.cs	✅ Fixed (// EnsureSetup — CAS-based lazy init)
RequiredState/TransitionTarget missing Volatile.Write through both indirection levels	✅ Fixed (getter uses Volatile.Read(ref rare.X), setter uses Volatile.Write)
BehaviorLock inner field not read with volatile	✅ Fixed (fast path uses Volatile.Read(ref rare.BehaviorLock))
CAS ordering assumption undocumented	✅ Fixed (comments in both generator source and emitted code)
Nit: EnsureRareState returning plain _rareState!	✅ Fixed — current diff shows return Volatile.Read(ref _rareState)!

Fresh Look at Current State

Singleton matchers — Textbook correct. Stateless sealed classes with static readonly Instance = new() are CLR-guaranteed thread-safe, zero-cost after first access. All four call sites updated.

RareState hoisting — The nested private sealed class RareState design is clean. Making fields internal to RareState rather than private is appropriate given the class is private itself. All field accesses through EnsureRareState() / Volatile.Read(ref _rareState) are consistent.

One subtle but correct pattern worth calling out: Volatile.Write(ref EnsureRareState().RequiredState, value) takes a ref directly from a method-return field access. This is valid in C# 7+ because the target is a heap-allocated class field (stable address), and the compiler accepts it. It's slightly unusual stylistically but not a problem.

EnsureRareState CAS pattern — Now follows the same canonical form as EnsureBehaviorLock:

var existing = Volatile.Read(ref _rareState);   // fast path acquire-load
if (existing is not null) return existing;
Interlocked.CompareExchange(ref _rareState, new RareState(), null);
return Volatile.Read(ref _rareState)!;          // winner sees own write; loser sees winner's write

The final Volatile.Read after the CAS is the correct pattern — it ensures the CAS loser sees the winner's write on ARM (where CAS alone doesn't imply a full barrier for subsequent plain reads). This is now consistent across both EnsureRareState and EnsureBehaviorLock.

Generated EnsureSetupSlow — The post-CAS ordering assumption is now documented in two places (generator source comment + emitted code comment), which is the right level of documentation for a subtle invariant. The sequential-setup assumption is reasonable and consistent with how mocking frameworks are used.

GetEventRaises double-check pattern — Correctly reads EventRaises via Volatile.Read before the lock, then uses EventRaises!.ToArray() inside the lock. Safe because EventRaises is only ever set from null → non-null (never back to null), so the non-null observation at the volatile read persists inside the lock.

No Outstanding Issues

The PR is clean. All correctness concerns from prior reviews are resolved, the volatile discipline is consistent throughout, and the performance rationale is sound. The estimated allocation savings (~80–180B per hot-path call) are real and the implementation is correct.

Ready to merge. ✅

[claude]

Code Review — Fourth Pass (perf(mocks): shrink MethodSetup + cache stateless matchers)

All issues from the previous three review rounds have been resolved. This pass is a fresh look at the final state.

---

What's changed (summary)

Area	Change
AnyMatcher<T>, NullMatcher<T>, NotNullMatcher<T>, EmptyMatcher	static readonly Instance singleton added
Arg.cs, ArgOfT.cs	All four call sites replaced with Instance
MethodSetup.cs	Rare fields hoisted into a lazily-allocated RareState inner class
Generated *MockCall classes (all snapshot files)	LazyInitializer replaced with Volatile.Read fast path + EnsureSetupSlow CAS slow path

---

Singleton matchers — Correct ✅

AnyMatcher<T>, NullMatcher<T>, NotNullMatcher<T> are generic — each closed type gets its own Instance via the CLR's per-instantiation static field guarantee. EmptyMatcher is non-generic, so Instance is a true global singleton. All four are stateless and sealed, making the singleton pattern safe and allocation-free.

---

RareState lazy allocation — Correct ✅

private RareState EnsureRareState()
{
    var existing = Volatile.Read(ref _rareState);
    if (existing is not null) return existing;
    Interlocked.CompareExchange(ref _rareState, new RareState(), null);
    return Volatile.Read(ref _rareState)!;  // canonical CAS pattern
}

The BehaviorLock fast path, RequiredState/TransitionTarget getters/setters, and EnsureRareState all follow the same canonical volatile-read + CAS pattern now. No memory-ordering gaps remain.

---

CAS-based EnsureSetupSlow in generated code — Correct ✅

private MethodSetupBuilder<T> EnsureSetupSlow()
{
    var setup = new MethodSetup(_memberId, _matchers, _memberName);
    var fresh = new MethodSetupBuilder<T>(setup);
    var prev = Interlocked.CompareExchange(ref _builder, fresh, null);
    if (prev is not null) return prev;
    // AddSetup runs only on the CAS winner. Setup is sequential in practice,
    // so a concurrent loser observing the builder before registration is benign.
    _engine.AddSetup(setup);
    return fresh;
}

The post-CAS ordering assumption is documented in both the emitted code and MockMembersBuilder.cs. [MethodImpl(MethodImplOptions.NoInlining)] correctly keeps the slow path out of the JIT's inlining budget for the hot fast-path method.

---

Minor observation (non-blocking)

OutRefAssignments getter takes the lock whenever rare != null, even to return a null value:

var rare = Volatile.Read(ref _rareState);
if (rare is null) return null;
lock (BehaviorLock)
{
    return rare.OutRefAssignments;  // often null
}

GetEventRaises uses a lock-free early-exit for its equivalent null case via Volatile.Read(ref rare.EventRaises). Applying the same pattern here would avoid acquiring BehaviorLock for mocks that have _rareState allocated (e.g. because RequiredState was set) but no out/ref parameters. That said, out/ref-heavy mocks are already a rare path, and this is consistent with the pre-PR behavior, so it doesn't warrant blocking the merge.

---

Summary

All prior review concerns are resolved. The three independent optimizations (singleton matchers, RareState hoisting, CAS-based lazy init) are each correct in isolation and compose correctly together. The snapshot test coverage gives high confidence in the generated-code changes.

Ready to merge. ✅