perf: lazily initialize optional MockEngine collections

[thomhurst]

Summary

• Lazily initialize 4 ConcurrentDictionaries + 1 ConcurrentQueue in MockEngine<T> that support optional features (auto-track properties, event subscriptions/callbacks, auto-mock cache)
• These collections are now only allocated on first use, not at mock creation time
• Read paths use null checks on the backing field to avoid triggering allocation

Benchmark Results

Metric	Before	After	Change
TUnit.Mocks (Calculator)	2,654 ns / 4.47 KB	893 ns / 1.13 KB	3x faster, 4x less memory
TUnit.Mocks (Repository)	1,862 ns / 4.47 KB	839 ns / 1.13 KB	2.2x faster, 4x less memory

TUnit.Mocks now has the lowest allocation of all benchmarked frameworks (1.13 KB vs Moq 2 KB, FakeItEasy 2.66 KB, NSubstitute 4.88 KB).

Test plan

• All 672 TUnit.Mocks.Tests pass
• MockCreation benchmarks verified locally
• CI passes

[claude]

Code Review

This is a well-targeted performance PR with impressive benchmark results (3x faster, 4x less memory). The approach is clean and consistent. One meaningful concern worth addressing before merge:

---

Thread-Safety Risk with ??= Lazy Initialization

The lazy property accessors use the ??= operator:

private ConcurrentDictionary<string, object?> AutoTrackValues
    => _autoTrackValues ??= new();

This compiles to a non-atomic read-check-write, roughly:

get {
    if (_autoTrackValues == null) _autoTrackValues = new();
    return _autoTrackValues;
}

Under concurrent access, two threads can both see null, both allocate a new instance, and one thread's allocation gets overwritten — any data written to the losing instance is silently dropped. This matters here because ConcurrentDictionary was deliberately chosen for thread-safe concurrent access, but the lazy initialization pattern breaks that guarantee on the initial write race.

Better approach — use LazyInitializer.EnsureInitialized:

private ConcurrentDictionary<string, object?> AutoTrackValues
    => LazyInitializer.EnsureInitialized(ref _autoTrackValues);

LazyInitializer.EnsureInitialized uses Interlocked.CompareExchange internally, ensuring only one instance is ever published. It has zero overhead on the hot (already-initialized) read path — same as a null check — and no wrapper allocation at all, so the benchmark gains are fully preserved.

This applies to all five lazily-initialized fields:

• _autoTrackValues
• _eventSubscriptions
• _onSubscribeCallbacks
• _onUnsubscribeCallbacks
• _autoMockCache

---

Everything Else Looks Good

• Asymmetric read/write paths are intentional and correct: write paths use the property (allocating on first use), read-only paths check the backing field directly (e.g. HandleCallWithReturn, RecordEventSubscription, GetEventSubscriberCount, WasEventSubscribed) — this is the right pattern.
• Reset() correctly uses null-conditional operators — no spurious allocations.
• TryGetAutoMock null guard is correctly added alongside the lazy property.
• _callHistory staying eager is the right call — call history is always used.
• The benchmark data is solid and the commit message explains the motivation clearly.

The thread-safety fix is a one-line change per property and doesn't affect the performance gains — worth doing before merge.

[claude]

Code Review — perf: lazily initialize optional MockEngine collections

The previous review flagged the ??= thread-safety risk. The author has since addressed it with Interlocked.CompareExchange + volatile. That fix is correct — this is a follow-up pass on the updated code.

---

The Lazy Initialization Pattern Is Correct

The current pattern:

private ConcurrentDictionary<string, object?> AutoTrackValues
    => _autoTrackValues ?? Interlocked.CompareExchange(ref _autoTrackValues, new(), null) ?? _autoTrackValues;

This is a well-known lock-free lazy init idiom and it is correct:

• Fast path: _autoTrackValues ?? ... — volatile ensures a fresh read, short-circuits immediately if already initialized.
• Slow path: ICE atomically publishes the new instance only if the field is still null; it returns the previous (old) value.
  • If we won the race: ICE returns null → second ?? reads _autoTrackValues, which is now our newly written instance.
  • If we lost the race: ICE returns the other thread's instance (non-null) → second ?? short-circuits on that.

The volatile qualifier is also appropriate: it prevents the JIT from caching the field in a register between the two reads, which would be incorrect in the "won the race" branch.

No correctness concern with this pattern.

---

One Remaining Observation — RecordEventSubscription Write-Then-Null-Check Asymmetry

public void RecordEventSubscription(string eventName, bool isSubscribe)
{
    EventSubscriptions.Enqueue((eventName, isSubscribe));   // allocates via property

    if (isSubscribe)
    {
        if (_onSubscribeCallbacks is not null && _onSubscribeCallbacks.TryGetValue(...))  // direct field check

EventSubscriptions (the property) is called unconditionally — so it allocates the queue on the first event subscription even if no OnSubscribe callback was ever registered. The intent of the PR is to only allocate when a feature is actively used. For most test scenarios that use events but have no OnSubscribe callbacks, this still allocates _eventSubscriptions eagerly.

This is a pre-existing design decision, not a regression introduced by this PR, so it's not a blocker. But if _eventSubscriptions were guarded the same way as the callbacks — only allocated when explicitly needed — it would be fully consistent with the PR's lazy strategy. Worth a comment explaining why the queue is allocated on subscription recording vs. only on GetEventSubscriberCount/WasEventSubscribed calls.

---

Minor — Use Range Syntax Over Substring(4)

These two lines use the older API:

AutoTrackValues[memberName.Substring(4)] = args[0];
// ...
if (_autoTrackValues.TryGetValue(memberName.Substring(4), out var trackedValue))

With LangVersion set to preview, these should be memberName[4..]. Pre-existing, but these lines are touched in this diff — good opportunity to modernize.

---

Reset() — Nulling vs. Clearing Is the Right Call

_autoTrackValues = null;
_eventSubscriptions = null;
// etc.

This is strictly better than the old .Clear() approach: it reclaims the actual backing collection objects (GC can collect them), whereas .Clear() just empties them and leaves the empty ConcurrentDictionary/ConcurrentQueue alive. It also keeps Reset() consistent with the lazy allocation model — post-reset, features are back to the unallocated state.

---

TryGetAutoMock Null Guard Is Correct and Necessary

public bool TryGetAutoMock(string cacheKey, out IMock? mock)
{
    if (_autoMockCache is null) { mock = null; return false; }
    return _autoMockCache.TryGetValue(cacheKey, out mock);
}

This is the correct asymmetric read path — check the backing field directly (no allocation) on a read-only query. All five "read-only query" sites follow this pattern consistently.

---

Summary

The thread-safety concern from the previous review has been correctly resolved. The implementation is sound. Two minor observations:

1. RecordEventSubscription allocates _eventSubscriptions on first event use — not a bug, but worth a comment if the laziness invariant is intended to extend to the subscription queue.
2. memberName.Substring(4) → memberName[4..] — pre-existing, but these lines are touched in this diff.

The benchmark gains are real and the implementation is correct. LGTM with those minor observations noted.

[claude]

Code Review

The lazy-init approach is a solid optimization — 4x memory reduction is meaningful for a mocking framework where many tests create multiple mocks. The volatile + Interlocked.CompareExchange pattern for lazy initialization is correct and well-understood.

Bug: TOCTOU race condition on volatile field reads

There are three places where the same volatile field is read twice without capturing it in a local variable. Between the null check and the .TryGetValue() call, another thread can call Reset() and null out the field, causing a NullReferenceException.

HandleCallWithReturn (line ~230):

// UNSAFE: _autoTrackValues can be nulled by Reset() between these two reads
if (AutoTrackProperties && _autoTrackValues is not null && memberName.StartsWith("get_", StringComparison.Ordinal))
{
    if (_autoTrackValues.TryGetValue(memberName[4..], out var trackedValue))

RecordEventSubscription (lines ~542, ~549):

if (_onSubscribeCallbacks is not null && _onSubscribeCallbacks.TryGetValue(eventName, out var callback))
// ...
if (_onUnsubscribeCallbacks is not null && _onUnsubscribeCallbacks.TryGetValue(eventName, out var callback))

TryGetAutoMock (lines ~484-489):

if (_autoMockCache is null) { ... }
return _autoMockCache.TryGetValue(cacheKey, out mock);  // can throw if Reset() races here

Fix: Capture into a local variable before the null check. This is the standard pattern when working with volatile nullable references:

// HandleCallWithReturn
var autoTrackValues = _autoTrackValues;
if (AutoTrackProperties && autoTrackValues is not null && memberName.StartsWith("get_", StringComparison.Ordinal))
{
    if (autoTrackValues.TryGetValue(memberName[4..], out var trackedValue))

// RecordEventSubscription
var onSubscribe = _onSubscribeCallbacks;
if (onSubscribe is not null && onSubscribe.TryGetValue(eventName, out var callback))

var onUnsubscribe = _onUnsubscribeCallbacks;
if (onUnsubscribe is not null && onUnsubscribe.TryGetValue(eventName, out var callback))

// TryGetAutoMock
var cache = _autoMockCache;
if (cache is null) { mock = null; return false; }
return cache.TryGetValue(cacheKey, out mock);

Why this matters: volatile guarantees visibility (no stale caches), but it doesn't prevent a second thread from modifying the field between your two reads. A local snapshot gives you a stable reference for the duration of the operation.

Notably, GetEventSubscriberCount and WasEventSubscribed already do this correctly — they read _eventSubscriptions once and early-return if null, never touching the field again. The three cases above need the same treatment.

---

The lazy-init property pattern is correct

private ConcurrentDictionary<string, object?> AutoTrackValues
    => _autoTrackValues ?? Interlocked.CompareExchange(ref _autoTrackValues, new(), null) ?? _autoTrackValues;

This is a well-known, correct pattern: if CompareExchange returns null (we won the race), read back the field we just wrote; if it returns non-null (another thread won), use that value. The volatile ensures the final _autoTrackValues read sees the most recent write. ✓

---

Minor: Reset() nulling vs. clearing

Setting fields to null in Reset() is correct and consistent with the lazy approach. It also slightly improves Reset performance by avoiding the lock-per-operation on each collection. One subtle implication: any thread that captured a collection reference before Reset() fires will still operate on the old (now-abandoned) collection rather than the newly reset one — this is generally acceptable behavior for a Reset, but worth being aware of.

---

Summary: The optimization is well-executed and the performance results are impressive. The one concrete issue is the TOCTOU pattern on three volatile field reads; the fix is a one-liner in each case (capture to local). Everything else looks good.

[claude]

Code Review — Follow-up Pass

All three concerns from the previous reviews have been correctly addressed in this iteration:

1. Thread-safety () — replaced with LazyInitializer.EnsureInitialized, which uses Interlocked.CompareExchange internally. Correct and zero-overhead on the hot (already-initialized) path. ✓
2. TOCTOU race — all read-only paths now capture to a local via Volatile.Read(ref _field) is { } local before the null check and subsequent use. Pattern is consistent across all five fields and matches what GetEventSubscriberCount/WasEventSubscribed already did correctly. ✓
3. Range syntax — memberName[4..] replaces memberName.Substring(4) in the touched lines. ✓

One Remaining Subtle Concern — Reset() Uses Plain Writes

_autoTrackValues = null;    // plain write
_eventSubscriptions = null;
// etc.

Read paths use Volatile.Read (acquire fence), but Reset() uses plain assignment (no release fence). On weakly-ordered architectures (ARM), a thread doing Volatile.Read might not see a null that was written with a plain store. The formally correct pairing is Volatile.Write:

Volatile.Write(ref _autoTrackValues, null);
Volatile.Write(ref _eventSubscriptions, null);
// etc.

In practice on x64/.NET runtime this works due to the stronger-than-ECMA implementation guarantees, and Reset() is typically called in single-threaded teardown anyway — so this is not a blocker. But it's worth noting for completeness given the careful use of Volatile.Read elsewhere.

Summary

The implementation is correct and the benchmark results are compelling (4x less memory, lowest allocation among benchmarked frameworks). The LazyInitializer.EnsureInitialized + Volatile.Read combination is the right idiom for this pattern. LGTM — the Volatile.Write in Reset() is the only remaining observation and it's minor.

[claude]

Code Review — Follow-up Pass

All three concerns from the previous reviews have been correctly addressed in this iteration:

1. Thread-safety (??=) — replaced with LazyInitializer.EnsureInitialized, which uses Interlocked.CompareExchange internally. Correct and zero-overhead on the hot (already-initialized) path. ✓
2. TOCTOU race — all read-only paths now capture to a local via Volatile.Read(ref _field) is { } local before the null check and subsequent use. Pattern is consistent across all five fields and matches what GetEventSubscriberCount/WasEventSubscribed already did correctly. ✓
3. Range syntax — memberName[4..] replaces memberName.Substring(4) in the touched lines. ✓

One Remaining Subtle Concern — Reset() Uses Plain Writes

_autoTrackValues = null;    // plain write
_eventSubscriptions = null;
// etc.

Read paths use Volatile.Read (acquire fence), but Reset() uses plain assignment (no release fence). On weakly-ordered architectures (ARM), a thread doing Volatile.Read might not see a null that was written with a plain store. The formally correct pairing is Volatile.Write:

Volatile.Write(ref _autoTrackValues, null);
Volatile.Write(ref _eventSubscriptions, null);
// etc.

In practice on x64/.NET runtime this works due to the stronger-than-ECMA implementation guarantees, and Reset() is typically called in single-threaded teardown anyway — so this is not a blocker. But it's worth noting for completeness given the careful use of Volatile.Read elsewhere.

Summary

The implementation is correct and the benchmark results are compelling (4x less memory, lowest allocation among benchmarked frameworks). The LazyInitializer.EnsureInitialized + Volatile.Read combination is the right idiom for this pattern. LGTM — the Volatile.Write in Reset() is the only remaining observation and it's minor.

[claude]

Code Review — Final Pass

This is the final review following four rounds of iteration. All previously identified concerns have been resolved.

All Previous Concerns Addressed ✓

Round	Issue	Status
1	??= non-atomic lazy init	Fixed with LazyInitializer.EnsureInitialized
2	TOCTOU double-read race	Fixed with Volatile.Read(ref _field) is { } local captures
2	Range syntax on touched lines	Fixed: memberName[4..]
3–4	Reset() plain writes unpaired with Volatile.Read	Fixed with Volatile.Write

Final Implementation Is Correct

The complete pattern is now internally consistent:

• Write paths (AutoTrackValues[key] = ..., EventSubscriptions.Enqueue(...), etc.): use properties backed by LazyInitializer.EnsureInitialized — lock-free, safe under concurrent allocation races.
• Read-only paths (HandleCallWithReturn, RecordEventSubscription, TryGetAutoMock): capture via Volatile.Read(ref _field) is { } local then use the local exclusively — no TOCTOU window.
• Reset path: Volatile.Write(ref _field, null) provides a proper release fence, pairing correctly with the acquire fence in Volatile.Read on all read paths. This is the right idiom for weakly-ordered architectures (ARM).

Why This Final State Is Right

The Volatile.Read / Volatile.Write pairing matters for memory model correctness: Volatile.Read emits an acquire barrier (no reads/writes after this can be reordered before it) and Volatile.Write emits a release barrier (no reads/writes before this can be reordered after it). Pairing them ensures the null set by Reset() is visible to subsequent Volatile.Read calls on other threads, even on ARM64.

Result

The optimization delivers on its goals — 3x faster construction, 4x less memory, now the lowest allocation of any benchmarked framework. The threading model is sound throughout. LGTM.