perf: reduce allocations and improve hot-path performance

[thomhurst]

Summary

• Lazy-init Context output state — StringBuilder, ReaderWriterLockSlim, and ConsoleLineBuffer are now allocated only when a test actually captures output (thread-safe via LazyInitializer.EnsureInitialized). Most test contexts never use these, saving ~6 object allocations per test.
• Lock-free log sink dispatch — TUnitLoggerFactory.GetSinks() now returns a volatile cached array instead of taking a lock and calling ToArray() on every Console.Write during tests. Cache is rebuilt only when sinks are added/cleared.
• Replace ConcurrentBag with List for Timings and Artifacts collections that are written sequentially and read once after completion.
• O(1) duplicate detection in ClassHookContext via new generic ReferenceEqualityComparer<T> (replaces O(N) List.Contains on reference types).
• Parallel test registration — RegisterTestsAsync uses Parallel.ForEachAsync for 8+ tests, bounded by Environment.ProcessorCount.
• HashSet-based UID filter — TestNodeUidListFilter matching is now O(1) per test instead of O(N) list scan.
• CTS fast-path in HookTimeoutHelper — skip CancellationTokenSource allocation when no timeout is configured (Timeout.InfiniteTimeSpan or >= 1 day).
• Targeted EventReceiverRegistry invalidation — invalidate only the specific interface type cache entry when a receiver is registered, instead of clearing the entire cache.
• Deferred StateBag/Events allocation in TestBuilderContext and DataGeneratorMetadataCreator.
• LINQ elimination in hot paths — manual loops replace .Any(), .OfType().FirstOrDefault(), and yield return + .ToArray() patterns in TestBuilder, TestExtensions, and TestFilterService.
• Conditional exception list in test teardown — List<Exception> only allocated when exceptions actually exist.

Test plan

• All 167 unit tests pass
• All 144 engine tests pass (99 skipped as expected)
• Clean build with 0 warnings across all TFMs
• CI passes across net8.0/net9.0/net10.0

[codacy-production]

Not up to standards ⛔

🔴 Issues 1 medium · 3 minor

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

Results:
4 new issues

Category	Results
CodeStyle	3 minor
Performance	1 medium

View in Codacy

🟢 Metrics 19 complexity

Metric	Results
Complexity	19

View in Codacy

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

[claude]

Performance Optimization Review

This is a well-structured PR targeting real bottlenecks with measurable impact. Most changes are correct and use idiomatic patterns. A few items deserve scrutiny before merging.

---

Issues Found

1. TestFilterService._uidFilterSet — data race in lazy init

// TestFilterService.cs
private HashSet<string>? _uidFilterSet;

private HashSet<string> GetOrCreateUidFilterSet(TestNodeUidListFilter filter)
{
    if (_uidFilterSet != null)
        return _uidFilterSet;
    // ... build set ...
    _uidFilterSet = set;
    return set;
}

MatchesTest can be called from multiple threads during parallel test execution. Two threads can both observe _uidFilterSet == null, both construct the set, and one's result gets overwritten. While the outcome is functionally identical (same content), this is a genuine data race on a non-volatile field. The rest of this PR uses LazyInitializer.EnsureInitialized precisely for this scenario — apply it here too:

private HashSet<string>? _uidFilterSet;

private HashSet<string> GetOrCreateUidFilterSet(TestNodeUidListFilter filter) =>
    LazyInitializer.EnsureInitialized(ref _uidFilterSet, () =>
    {
        var set = new HashSet<string>(StringComparer.Ordinal);
        foreach (var uid in filter.TestNodeUids)
            set.Add(uid.Value);
        return set;
    })!;

Why this matters: The current pattern is a well-known anti-pattern (lazy init without synchronization). It's also inconsistent with the thread-safety discipline applied everywhere else in this PR.

---

2. RegisterTestsAsync — parallel execution may not be safe

await Parallel.ForEachAsync(
    testList,
    new ParallelOptions { MaxDegreeOfParallelism = Environment.ProcessorCount },
    async (test, _) => await RegisterTest(test).ConfigureAwait(false)
);

RegisterTest invokes:

• test.Context.GetTestRegisteredReceivers() — hits EventReceiverRegistry shared state
• User-defined ITestRegisteredEventReceiver.OnTestRegistered(...) — could have shared mutable state
• testArgumentRegistrationService.RegisterTestArgumentsAsync(test.Context) — shared service instance

The threshold of 8 tests is also arbitrary. If RegisterTest has any unsynchronized shared state (even inside user receivers), this will introduce flaky non-deterministic failures that are extremely hard to reproduce.

Suggested approach: Either document explicitly which internal methods are thread-safe and add a test for concurrent registration, or reconsider this change. Parallelizing I/O-bound work (e.g., async data source fetching) is well-motivated, but parallelizing what is effectively sequential state machine setup risks subtle correctness issues. The perf win here may not justify the correctness risk.

---

3. GitHubReporter.cs — SingleOrDefault is semantically stronger than the original

// Before (tolerant):
var state = testNodeUpdateMessage.TestNode.Properties.OfType<TestNodeStateProperty>().FirstOrDefault();

// After (throws if > 1 match):
var state = testNodeUpdateMessage.TestNode.Properties.SingleOrDefault<TestNodeStateProperty>();

SingleOrDefault throws InvalidOperationException if the sequence contains more than one matching element. If IPropertyBag can ever contain multiple TestNodeStateProperty instances (e.g., a duplicate update, a bug in the platform, or a future API change), the reporter will crash instead of silently handling it. The original FirstOrDefault was defensively resilient.

Also note: SingleOrDefault must scan the entire collection to verify uniqueness, whereas OfType<T>().FirstOrDefault() exits after the first match — so this change could be measurably slower on property bags with many entries.

Recommendation: Revert to OfType<TestNodeStateProperty>().FirstOrDefault() unless there's a specific reason uniqueness needs to be enforced.

---

4. ClassHookContext — dual collection doubles memory, not thread-safe

private readonly HashSet<TestContext> _testSet = new(ReferenceEqualityComparer<TestContext>.Instance);
private readonly List<TestContext> _tests = [];

public void AddTest(TestContext testContext)
{
    if (!_testSet.Add(testContext))
        return;
    _tests.Add(testContext);
}

This maintains two parallel data structures (HashSet for O(1) lookup + List for ordered iteration). The O(1) improvement is real, but:

• Memory doubles per class context.
• The two collections are not updated atomically — concurrent AddTest calls without external synchronization could corrupt both.
• The original _tests.Contains was also not thread-safe, so this doesn't regress on that front, but neither does it fix it.

If AddTest is called from a single thread (test discovery is typically single-threaded), a simpler, cleaner alternative is LinkedHashSet semantics via Dictionary<TestContext, int> keyed by reference, which avoids the dual-collection fragility:

// Or simply: if AddTest is single-threaded, document it and keep HashSet as primary
private readonly HashSet<TestContext> _tests = new(ReferenceEqualityComparer<TestContext>.Instance);
public IReadOnlyCollection<TestContext> Tests => _tests;

---

5. HookTimeoutHelper — TimeSpan.FromDays(1) is recomputed on every call

if (effectiveTimeout == Timeout.InfiniteTimeSpan || effectiveTimeout >= TimeSpan.FromDays(1))

TimeSpan.FromDays(1) is a static method that calculates a new TimeSpan value on every invocation. While it doesn't heap-allocate (it's a struct), it's unnecessarily recomputed in what's claimed to be a hot path. Extract to a static readonly field:

private static readonly TimeSpan OneDayTimeout = TimeSpan.FromDays(1);

Minor, but inconsistent with the "minimize hot-path overhead" intent of this PR.

---

What's Well Done

• LazyInitializer.EnsureInitialized in Context.cs: Correct, thread-safe, and well-commented. The GetStandardOutput() null-check fast path is a good touch — avoids lock acquisition when no output was captured.
• TUnitLoggerFactory volatile cache: Properly implemented — mutations under lock, reads via volatile field, cache rebuilt eagerly on mutation. The DisposeAllAsync reuse of _sinksCache as sinksToDispose before clearing is subtle but correct.
• HasInstanceDataAccessor helper: Clean extraction that removes four identical LINQ expressions. Exactly the right level of abstraction.
• GetNestedClassName two-pass array fill: The depth-count + reverse-fill approach (avoiding List.Reverse()) is a genuinely clever micro-optimization.
• GetTrxMessages → TrxMessage[]: Eliminates the IEnumerable + ToArray() round-trip and state machine allocation. The pre-sized array fill pattern is clean.
• TestBuilderContext lazy init for Events/StateBag: Confirmed that TestBuilderContext.cs already supports deferred allocation via ??= — the init removal at call sites is safe.
• Random.Shared: Correct fix, avoids unnecessary allocations.
• EventReceiverRegistry targeted invalidation: Surgically removing only the affected type entry from _cachedTypedReceivers is strictly better than clearing the whole cache.
• ReferenceEqualityComparer<T>: Clean, properly using RuntimeHelpers.GetHashCode.

---

Summary: The direction is right and most changes are solid. The main concerns are the data race in GetOrCreateUidFilterSet, the correctness risk of parallelizing RegisterTestsAsync, and the behavioral change from FirstOrDefault to SingleOrDefault in the reporter. These three are worth addressing before merge; the others are minor improvements.

[claude]

Follow-Up Performance Review

Reviewing the current diff in full. A previous review from earlier today flagged several issues — let me update the status on those and add a new finding.

---

Previous Review Findings: Status Update

Issue 1 — _uidFilterSet data race: The current code already uses LazyInitializer.EnsureInitialized correctly. That concern does not apply to the code as it stands. ✅

Issue 5 — TimeSpan.FromDays(1) recomputed: The current code already extracts this to private static readonly TimeSpan EffectivelyInfiniteTimeout = TimeSpan.FromDays(1). ✅

Issue 3 — SingleOrDefault in GitHubReporter: This does not appear in the current diff. The only GitHubReporter.cs change is new Random() → Random.Shared. ✅

Issue 2 — RegisterTestsAsync parallel execution: Still present and warrants attention (see below).

Issue 4 — ClassHookContext dual collection: Still present (see below).

---

New Finding: ConcurrentBag<T> → List<T> for Timings and Artifacts (thread-safety regression risk)

// TestContext.Output.cs
internal List<TimingEntry> Timings { get; } = [];
private readonly List<Artifact> _artifacts = [];

The PR description says these are "written sequentially during test execution and read once after completion." That's true for Timings (written by the framework in a predictable sequence), but Artifacts is exposed through ITestOutput.AttachArtifact, which test code can call freely, including from parallel branches inside a test:

// A user can legally do this:
await Task.WhenAll(
    Task.Run(() => TestContext.Current!.AttachArtifact("a.log")),
    Task.Run(() => TestContext.Current!.AttachArtifact("b.log"))
);

List<T>.Add is not thread-safe. Two concurrent adds can corrupt the internal array (lost write, ArgumentOutOfRangeException on resize, or silent data loss).

ConcurrentBag<T> was the right type for _artifacts because of this. If the allocation cost is a real concern, ImmutableInterlocked.Update or a thin lock around _artifacts additions would maintain correctness without the full ConcurrentBag overhead.

Recommendation: Revert _artifacts to a thread-safe collection. For Timings, if it can be proven that all writes come from a single thread, List<T> is fine — but that proof should be a comment, not an assumption.

---

Remaining Concern: RegisterTestsAsync Parallelism

if (testList.Count < 8)
{
    foreach (var test in testList)
        await RegisterTest(test);
    return;
}

await Parallel.ForEachAsync(testList,
    new ParallelOptions { MaxDegreeOfParallelism = Environment.ProcessorCount },
    async (test, _) => await RegisterTest(test).ConfigureAwait(false));

RegisterTest invokes ITestRegisteredEventReceiver.OnTestRegistered(...) on any registered receivers. These are user-provided hooks. Even if the framework's internal path through RegisterTest is safe, user receivers that perform any shared writes will silently break under the 8+ test case with no indication the call site is now concurrent.

The correctness of this depends on a contract (RegisterTest is safe to call concurrently) that is not documented, not tested, and not visible to users implementing receivers. The threshold of 8 is also effectively invisible to test authors — their receiver runs sequentially with 7 tests and breaks with 8+.

Recommendation: If parallel registration is kept, it needs: (a) a comment stating the concurrency contract, (b) a test exercising concurrent OnTestRegistered calls, and (c) documentation in the ITestRegisteredEventReceiver interface that implementations must be thread-safe.

---

Remaining Minor: ClassHookContext Dual Collection

Maintaining both _testSet (HashSet) and _tests (List) doubles memory per class context and adds fragility if the two ever diverge. If Tests iteration order is not guaranteed elsewhere, simplifying to HashSet<TestContext> as the sole collection would be cleaner. If order matters, this is the right tradeoff — but it should be documented.

---

What's Genuinely Good

The bulk of this PR is high quality:

• Lazy Context.cs output state: Correct use of LazyInitializer, proper null-check fast-path in GetStandardOutput()/GetErrorOutput(), and the GetOutputLock() vs _outputLock field access in the read path is careful and correct.
• TUnitLoggerFactory volatile cache: Classic SWMR pattern — mutations under lock, reads via volatile snapshot. The DisposeAllAsync reuse of _sinksCache is elegant.
• HasInstanceDataAccessor helper: Clean extraction of a repeated pattern at the right level of abstraction.
• GetTrxMessages array rewrite: Eliminates both the iterator state machine and the ToArray() allocation, with a pre-sized array. Well done.
• GetNestedClassName two-pass fill: Depth-count + reverse-fill avoids List + Reverse. Genuinely clever.
• EventReceiverRegistry targeted invalidation: Surgically correct — only the affected type key is removed.
• _uidFilterSet HashSet for O(1) UID matching: Good use of LazyInitializer.EnsureInitialized. One note: the cached set is keyed by the first filter seen; if MatchesTest is ever called with a new TestNodeUidListFilter (different UIDs) on the same service instance in IDE "run specific tests again" scenarios, the new UIDs won't be seen. May be worth a comment documenting the single-filter-per-run assumption.
• Random.Shared: Correct fix.
• ReferenceEqualityComparer<T>: Clean generic variant with a singleton Instance — consistent with BCL style.

---

Summary: Two issues warrant action before merge — the List<T> for _artifacts (silent thread-safety regression) and the RegisterTestsAsync parallelism (undocumented concurrency contract on user-facing receivers). The rest is solid work.

[claude]

Third Review — Follow-up on Previous Findings

Previous reviews flagged five issues. Here's the final status and one remaining concern.

---

Previous Issues: Status

Issue 1 — _uidFilterSet data race: Resolved. LazyInitializer.EnsureInitialized is used correctly. ✅

Issue 5 — TimeSpan.FromDays(1) recomputed in hot path: Resolved. Extracted to private static readonly TimeSpan EffectivelyInfiniteTimeout. ✅

Issue 3 — SingleOrDefault in GitHubReporter: Not present in the diff. ✅

Issue — _artifacts thread-safety (second review): Resolved in the latest commit (53cecf0). Lock _artifactsLock protects all List<Artifact> mutations and the snapshot read in Artifacts. This is the right approach — the Lock type in .NET 9 is lower-overhead than Monitor/lock on older types. ✅

---

Remaining Concern: RegisterTestsAsync Parallelism Contract

The XML doc comment added in the latest commit states:

/// ITestRegisteredEventReceiver implementations must be thread-safe.

This documents the assumption on the call site, but the contract is not visible where it needs to be — at the interface definition. Any user who implements ITestRegisteredEventReceiver reads the interface, not the call site. The behavior is also threshold-dependent (sequential for <8 tests, concurrent for 8+), which means a non-thread-safe implementation will appear to work in small test suites and break silently at scale.

Recommended minimum fix: add the same thread-safety note to the ITestRegisteredEventReceiver interface or the OnTestRegistered method XML doc. This is a one-line change that makes the contract visible at the point of implementation.

---

Minor Remaining: ClassHookContext Dual Collection

_testSet (HashSet) and _tests (List) are both maintained. The O(1) dedup improvement is real. If insertion order for Tests is tested or relied upon downstream, the dual structure is the right tradeoff — worth a brief comment to that effect. If order is not required, HashSet<TestContext> alone would be cleaner.

---

Overall Assessment

The bulk of this PR is high quality and the three main correctness concerns from the previous reviews have been addressed. The only remaining ask is a one-line interface doc update so the thread-safety contract on ITestRegisteredEventReceiver is visible to implementors. Everything else is solid work.

[claude]

Fourth Review — Latest Commit (1bb39fe)

Reviewing the changes since the third review. Previous concerns from all three earlier rounds are tracked below.

---

Previous Issues: Status Update

Issue 1 — _uidFilterSet data race: Resolved. ✅

Issue 2 — RegisterTestsAsync parallelism contract: The XML doc on RegisterTestsAsync documents the contract, but the third review specifically asked for the same note on ITestRegisteredEventReceiver.OnTestRegistered — the interface that implementors read. That change is not present in any commit so far. Still outstanding (see below).

Issue 3 — SingleOrDefault in GitHubReporter: Not in the diff. ✅

Issue 4 — ClassHookContext dual collection: The RemoveTest bug (missing _testSet.Remove) is now fixed in this commit ✅. The dual-collection design remains — see minor note below.

Issue 5 — TimeSpan.FromDays(1) recomputed: Resolved. ✅

Issue — _artifacts thread-safety: Resolved in commit 53cecf0. ✅

---

Remaining: ITestRegisteredEventReceiver.OnTestRegistered — missing thread-safety contract

The third review asked for a one-line doc update on the interface itself:

Any user who implements ITestRegisteredEventReceiver reads the interface, not the call site. The behavior is also threshold-dependent (sequential for <8 tests, concurrent for 8+).

The RegisterTestsAsync doc comment says "ITestRegisteredEventReceiver implementations must be thread-safe", but that comment is on the call site inside TestFilterService, not on the interface. A user implementing the interface has no way to discover this requirement. A one-line addition to the OnTestRegistered method XML doc (or a remark on the interface) would close this out:

/// <remarks>
/// Implementations must be thread-safe: this method may be called concurrently
/// when the test suite contains 8 or more tests.
/// </remarks>

This is the only outstanding ask from all three previous rounds.

---

New in This Commit: GetOrCreateUidFilterSet Pre-check

if (_uidFilterSet is { } cached)
{
    return cached;
}
return LazyInitializer.EnsureInitialized(ref _uidFilterSet, () => { ... })!;

The intent is to avoid closure allocation on the hot path after initialization. That's a legitimate micro-optimization. One subtle note: _uidFilterSet is not a volatile field, so the outer null check is a non-interlocked read — technically a benign data race (worst case: a thread sees stale null and falls through to EnsureInitialized, which uses CompareExchange and provides the needed memory fence). This is a well-known double-checked locking pattern that is safe in practice on x86/x64 and on ARM with the CLR's memory model guarantees. No action needed, but worth a comment explaining the intention if the pattern looks unusual to future readers.

---

Minor Remaining: ClassHookContext Dual Collection

The RemoveTest bug is now fixed. The dual structure (_testSet + _tests) is fragile by design, but the critical correctness issue is gone. If insertion order for Tests is load-bearing downstream (e.g., hooks fire in registration order), the dual structure is the right call — worth a brief comment to that effect. If order is not required, HashSet<TestContext> alone would be simpler.

---

Snapshot of the Latest Commit

• ClassHookContext.RemoveTest: Bug fix is correct — _testSet.Remove and _tests.Remove now both called. ✅
• TestExtensions artifact snapshot: Correct — testContext.Artifacts captured once avoids double lock acquisition. ✅
• Context.cs comment trim: No behavior change. ✅
• GetOrCreateUidFilterSet pre-check: Legitimate optimization, safe in practice. ✅

---

Overall Assessment

This PR is in excellent shape. All major correctness concerns across four review rounds have been addressed — thread-safe lazy init, artifact thread safety, RemoveTest bug, _uidFilterSet race, SingleOrDefault revert, and timeout fast-path. The one remaining ask is the interface-level doc on ITestRegisteredEventReceiver.OnTestRegistered to surface the thread-safety requirement where implementors will actually see it. The rest is solid, well-reasoned performance work.

[claude]

Fifth Review — All Outstanding Issues Resolved

This is a follow-up to four prior review rounds. The final commit 1ede29cae0 closes the last remaining ask.

---

All Previous Issues: Final Status

#	Issue	Status
1	_uidFilterSet data race	✅ Resolved — LazyInitializer.EnsureInitialized
2	RegisterTestsAsync parallelism contract	✅ Resolved — documented at call site AND now on ITestRegisteredEventReceiver.OnTestRegistered
3	SingleOrDefault in GitHubReporter	✅ Not present (reverted in commit 9f0bda4)
4	ClassHookContext RemoveTest bug (missing _testSet.Remove)	✅ Resolved in commit 1bb39fe
5	TimeSpan.FromDays(1) recomputed in hot path	✅ Resolved — extracted to EffectivelyInfiniteTimeout static field
6	_artifacts List<T> thread-safety regression	✅ Resolved — Lock _artifactsLock guards all mutations and snapshot reads

---

Final Commit: ITestRegisteredEventReceiver Interface Doc

The addition of the thread-safety note directly on the interface is exactly right. Implementors read the interface, not the call site — this is the only place the contract is discoverable. The phrasing is clear and accurate:

Thread safety: When many tests are registered, OnTestRegistered may be called concurrently from multiple threads. Implementations must be thread-safe.

One very minor note worth considering but not blocking: "when many tests are registered" is slightly ambiguous about the threshold (it's actually 8+). A future improvement could say something like "when the test suite contains 8 or more tests" — but this is not a blocker, just a readability suggestion.

---

Overall Assessment

This PR is ready to merge. Five commits and five review rounds have progressively addressed every concern:

• Thread safety: lazy init in Context.cs, volatile cache in TUnitLoggerFactory, Lock for artifacts, LazyInitializer for UID filter set — all handled correctly.
• Correctness: RemoveTest dual-collection bug fixed, SingleOrDefault behavioral change reverted, parallelism contract documented end-to-end.
• Performance: The underlying optimizations (lazy allocation, O(1) dedup, lock-free sink dispatch, LINQ elimination, iterator-to-array conversions) are sound and well-targeted.

Solid, thorough performance work.