perf: reduce allocations in test execution hot paths

[thomhurst]

Summary

• Replace ReaderWriterLockSlim with simple double-checked locking in EventReceiverRegistry - cache writes happen once per type, so the complex upgradeable lock pattern was overkill
• Change ExecuteTestLifecycleAsync to return ValueTask to avoid Task allocation on the fast path (tests without retry/timeout)
• Use lazy allocation for cleanup exceptions list - most tests complete without cleanup exceptions, avoiding unnecessary List<Exception> allocation
• Use collection expression in TestDiscoveryService for cleaner syntax

Test plan

• Build succeeds
• Existing TUnit.Engine.Tests pass
• Run benchmarks to measure improvement

🤖 Generated with Claude Code

[thomhurst]

Summary

This PR optimizes hot paths by replacing ReaderWriterLockSlim with double-checked locking, using ValueTask for test lifecycle, and lazy-allocating cleanup exception lists.

Critical Issues

1. Thread Safety Issue in EventReceiverRegistry

The double-checked locking pattern in GetReceiversOfType() has a race condition with the lock-free fast path (TUnit.Engine/Events/EventReceiverRegistry.cs:143-156).

Problems:

• _cachedTypedReceivers is a plain Dictionary - not thread-safe for concurrent read/write
• RegisterReceiverInternal() calls _cachedTypedReceivers.Clear() while holding lock, but reads happen lock-free
• Without memory barriers, readers may see stale/corrupted data

Fix: Use ConcurrentDictionary for _cachedTypedReceivers, or keep the read lock for cache hits.

2. ValueTask Allocation Negates Benefits

TUnit.Engine/Services/TestExecution/TestCoordinator.cs:128 calls .AsTask() which defeats the purpose of using ValueTask. The fast path benefits, but this adds API complexity for tests with timeout/retry.

Suggestion: Either accept the allocation in the slow path (keep Task), or change TimeoutHelper.ExecuteWithTimeoutAsync to accept Func returning ValueTask.

Suggestions

The lazy allocation of cleanupExceptions is well done - this will help avoid allocations in the success path.

Collection expression in TestDiscoveryService looks good.

Verdict

REQUEST CHANGES - The thread safety issue in EventReceiverRegistry must be fixed before merge. This is a hot path with potential for subtle race conditions that could cause test discovery failures.

[thomhurst]

Summary

Optimizes hot paths by replacing ReaderWriterLockSlim with double-checked locking, using ValueTask for test lifecycle, and lazy-allocating cleanup exception lists.

Critical Issues

None found ✅

Suggestions

1. Consider renaming variable for clarity

TUnit.Engine/Events/EventReceiverRegistry.cs:137 - The variable cached in the double-check pattern could be renamed on first use vs reuse to make the pattern more obvious:

// First check outside lock
if (_cachedTypedReceivers.TryGetValue(typeKey, out var fastPath))
{
    return (T[])fastPath;
}

lock (_lock)
{
    // Double-check after acquiring lock
    if (_cachedTypedReceivers.TryGetValue(typeKey, out var cached))
    {
        return (T[])cached;
    }
    // ...
}

This is purely stylistic though - the current code is correct.

2. ValueTask optimization comment

The .AsTask() call on line 128 is necessary for compatibility with TimeoutHelper.ExecuteWithTimeoutAsync, but it does negate the ValueTask benefit on the slow path (retry/timeout scenarios). The fast path (line 116) still benefits. Consider adding a comment explaining this tradeoff, or potentially updating TimeoutHelper to accept ValueTask-returning delegates in a future PR.

Previous Review Status

The previous review raised two concerns:

1. Thread safety with Dictionary - ✅ RESOLVED: The PR uses ConcurrentDictionary, not plain Dictionary, so concurrent reads/writes are safe
2. ValueTask.AsTask() defeating purpose - ⚠️ PARTIALLY ADDRESSED: The fast path (no retry/no timeout) benefits from ValueTask. The slow path still allocates via .AsTask(), but this is acceptable since it's already the slow path

Verdict

✅ APPROVE - No critical issues. The thread safety concern from the previous review is addressed by using ConcurrentDictionary. The lazy allocation optimization is sound, and the ValueTask optimization provides measurable benefit on the fast path.