perf: engine discovery/init path cleanup (#5528 C)

[thomhurst]

Part C of three-PR split for issue #5528.

Scope

Items 7-10 from the perf checklist — discovery/init path cleanup.

Changes

Item 7 — TestGroupingService sync hot path

GroupTestsByConstraintsAsync previously built an async state machine per call even though every await was behind an IsTraceEnabled guard. The core grouping work is now synchronous; trace messages are collected into a list and flushed through a thin async wrapper only when tracing is enabled. Hot path: zero state-machine overhead, no list allocation for trace messages.

Item 8 — LINQ chains → direct loops

• TestSessionCoordinator.InitializeEventReceivers — dropped Select(t => t.Context) and passed the test list through directly.
• EventReceiverOrchestrator.InitializeTestCounts — accepts IReadOnlyList<AbstractExecutableTest> directly; no more allTestContexts as IList<TestContext> ?? ToList() fallback. Internal-only API, no public surface change.
• EventReceiverOrchestrator.InvokeHookRegistrationEventReceiversAsync — replaced OfType<T>().OrderBy(...) with an explicit filtering loop + in-place List<T>.Sort.
• HookDelegateBuilder — three copies of .OrderBy().ThenBy().Select().ToList() consolidated into a single SortAndProject<TContext> helper using in-place List.Sort + index loop.
• TestDiscoveryContext.Assemblies / .TestClasses — Select(...).Distinct().ToArray() replaced with inlined HashSet<T>.Add loops + CopyTo.
• TestDiscoveryService.GetCachedTestContexts — materializes a List<TestContext> from the cached queue instead of returning a Select enumerable that allocates a lambda-backed iterator per foreach.

Item 9 — TestSessionContext.HasFailures via atomic counter

Added public bool HasFailures backed by a lock-free int _failureCount incremented via Interlocked.Increment in TestCoordinator's terminal switch for Failed/Timeout/Cancelled states. HookExecutor.ExecuteAfterTestSessionHooksAsync now reads the flag in O(1) instead of materializing AllTests (which itself eagerly flattens SelectMany across every class) and LINQ-scanning it.

Item 10 — TestContext.Id lazy string

TestContext now stores a Guid field and materializes the string form lazily via field ??= _idGuid.ToString(). The internal registry is keyed by Guid (ConcurrentDictionary<Guid, TestContext>); GetById(string) parses once on the lookup path. TestCoordinator cleanup calls the new RemoveFromRegistry() instance method so a test's string Id is never materialized unless something external asks for it. Public surface unchanged.

Verification

• Build clean across all TFMs.
• TUnit.Engine.Tests (net10.0): 148/148 passed, 99 skipped (platform/CI-only).
• TUnit.UnitTests (net10.0): 180/180 passed.
• TUnit.PublicAPI (net10.0): 4/4 passed. One expected snapshot update (HasFailures property added to TestSessionContext) committed in Tests.Core_Library_Has_No_API_Changes.DotNet10_0.verified.txt.

Related PRs

• perf: engine hot-path allocation wins (#5528 B) #5610 — Part B (hot-path allocation wins).
• perf: consolidate test dedup + hook register guards (#5528 A) #5612 — Part A (consolidate dedup + hook register guards).

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity

Metric	Results
Complexity	0

View in Codacy

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

[claude]

Code Review — PR #5611 (Part C, Draft Placeholder)

Status: No code to review yet — this PR contains only a scaffolding commit with 0 file changes. The review below covers the planned changes described in the PR description.

---

Planned Item #7 — TestGroupingService.GroupTestsByConstraintsAsync: drop async state machine

Good call. If the sort is truly synchronous, wrapping it in an async state machine adds unnecessary overhead (allocation of IAsyncStateMachine, AsyncTaskMethodBuilder, etc.). Returning ValueTask.CompletedTask or making the method return void/sync will be a clean win. Just verify no callers await it in a way that depends on yielding back to the caller mid-operation.

Planned Item #8 — Replace LINQ chains with direct loops/sorts

Solid micro-optimization for hot discovery paths. One architectural note: when replacing LINQ, prefer List<T>.Sort() with a Comparison<T> delegate over OrderBy().ToList() to avoid intermediate allocations. If the collection size is small and bounded, also consider whether sorting is even needed vs. insertion-order maintenance.

Planned Item #9 — TestSessionContext.HasFailures: atomic counter (O(1))

This is the highest-value change in this set. O(N) traversal on every HasFailures check is a correctness-risk under concurrent test runs, not just a perf issue. Using Interlocked.Increment in TestStateManager.MarkFailed is exactly right. A few things to confirm when the code lands:

• The counter must be incremented before the state is observable to other threads (memory ordering).
• Consider whether a decrement path is needed if a test transitions out of failed state (e.g., retry mechanisms).
• Expose as bool HasFailures => _failureCount > 0 (volatile read or Interlocked.Read) rather than the counter value directly, to keep the API surface clean.

Planned Item #10 — TestContext.Id: store Guid, lazy .ToString()

Good. Guid.ToString() is non-trivial (allocates a 36-char string). Storing the Guid and deferring .ToString() until actually needed is idiomatic for perf-sensitive paths. If the Id is only ever used for logging/display (not as a dictionary key on the string form), you might also consider a monotonic long counter via Interlocked.Increment — cheaper to allocate, compare, and format than a Guid.

---

Overall

The planned scope is well-targeted: each item has a clear, measurable benefit and a narrow blast radius. No architectural concerns with the approach. Looking forward to reviewing the actual implementation once Parts A and B are approved.

Nothing to block on yet — come back when code lands.

[claude]

Code Review — PR #5611 (Part C: Discovery/Init Path Cleanup)

Following up on the prior placeholder comment from when this PR had no code. All four items are now implemented — reviewing the actual changes.

---

Overall

Well-targeted perf work with a narrow blast radius. Each change is individually motivated and the implementation is clean. A few observations below, none blocking.

---

Item 7 — TestGroupingService: hot/cold path split

Good. Returning new ValueTask<GroupedTests>(result) on the hot path avoids the async state-machine allocation entirely. The tracing path is correctly isolated to GroupTestsByConstraintsWithTracingAsync.

One observable behavior change worth noting: trace messages are now buffered in a List<string> and flushed after grouping completes, rather than emitted inline as each test is categorized. Under concurrent test runs, this means grouping traces now appear as a batch, potentially out-of-order relative to other concurrent log output. This is an acceptable trade-off for the perf win, but could make trace-based debugging slightly harder — worth a note in the change description if you haven't already.

---

Item 8 — LINQ → loops + SortAndProject

Good. SortAndProject<TContext> is clean — HookDelegateBuilder is non-generic, so the method's own type parameter is unambiguous with no shadowing concern.

Observed: the existing BuildBefore/AfterTest/ClassHooksAsync methods already use a SortAndAddHooks helper (not touched by this PR), while the three assembly/global methods used raw LINQ. This PR completes the consistency.

Double-sort in InvokeHookRegistrationEventReceiversAsync is intentional and preserved correctly — the comment explains why (FilterScopedAttributes keyed on first-encountered order), no issue here.

---

Item 9 — HasFailures atomic counter

Excellent. Volatile.Read(ref _failureCount) > 0 is correctly implemented, and Interlocked.Increment in MarkFailure() provides the necessary memory-order guarantees. Replacing the O(N) AllTests LINQ traversal in HookExecutor with this O(1) read is the highest-value change in this PR.

One edge case to confirm: Cancelled tests also call MarkFailure() — this matches the original Any(t => t.Result is { State: Failed or Timeout or Cancelled }) check in HookExecutor, so semantics are preserved.

---

Item 10 — TestContext.Id lazy string

Mostly good. The Guid-as-primary-key approach is correct and the registry change is clean. However:

Potential concern with _idString ??= _idGuid.ToString(): This compound null-check-and-assign is not atomic. Under concurrent access, two threads can both observe _idString == null, both call _idGuid.ToString(), and each write its own allocation. The result is correct (both strings are identical) and reference assignment is atomic on x64, but _idString is not volatile, so a thread may read a stale null. On strong-memory-order platforms (x64) this is benign in practice; on platforms with weaker guarantees (ARM, WASM), or under AOT compilation without JIT memory-fence insertion, it is technically not safe.

Given TUnit's AOT-compatibility requirement, consider one of:

• Marking _idString as volatile (simplest fix, no API change)
• Using Interlocked.CompareExchange(ref _idString, _idGuid.ToString(), null) if strict once-init matters
• Or explicitly document the intentional benign-race pattern if the current behavior is acceptable

Note: The retained static RemoveById(string id) now does a Guid.TryParse on every call. If external callers use it, they pay a parse cost that didn't exist before. If it's internal-only with no remaining callers (the coordinator now uses RemoveFromRegistry() directly), consider removing RemoveById to avoid confusion. If it must stay for API surface reasons, the Guid.TryParse overhead is fine for a non-hot path.

---

Summary

Item	Status
Item 7 — Sync hot path in TestGroupingService	✅ Good, minor trace-ordering note
Item 8 — LINQ → SortAndProject	✅ Clean and complete
Item 9 — HasFailures atomic counter	✅ Excellent
Item 10 — Lazy Id string + Guid registry	✅ Good, volatile concern on non-x64/AOT

No blocking issues. The volatile concern on _idString is the only thing I'd fix before merge, especially given the AOT compatibility mandate.

[claude]

Follow-up Review — PR #5611

Correction from prior review

The previous review stated _idString is not volatile and suggested adding it. That was a misread — the diff already declares:

private volatile string? _idString;

The implementation is correct: volatile ensures ARM/WASM/AOT visibility, the benign double-ToString race is documented inline, and the comment explains why it's acceptable. No action needed on Item 10.

---

New observation: HashSet ordering in BuildUniqueAssemblies / BuildUniqueTestClasses

The replaced code used Select().Distinct().ToArray() — LINQ's Distinct() is documented to preserve first-occurrence insertion order. The new code uses HashSet<T> + CopyTo, which is not contractually ordered by the HashSet<T> documentation (the spec says "unordered").

In practice, the current .NET HashSet<T> implementation does iterate in insertion order, so the behavior is identical today. However, this is an implementation detail that could change. If the order of Assemblies or TestClasses affects hook execution sequencing, this is worth a brief comment or a switch to LinkedHashSet-equivalent (e.g., an OrderedDictionary-backed dedup, or just a List + HashSet pair). Low risk today, but flagging it.

---

Minor: null-check inconsistency in GroupTestsByConstraintsCore

Three of the four tracing branches use the null-conditional form:

traceMessages?.Add(...);

But the notInParallel != null branch uses an explicit block:

if (traceMessages != null) { ...; traceMessages.Add(...); }

The explicit block is needed there because it also computes keys (which is only needed for the message). This is correct as-is — just noting it's intentional, not an inconsistency.

---

Overall

The prior review's volatile concern is a non-issue — it was already addressed in the code. The HasFailures atomic counter (Item 9) and SortAndProject consolidation (Item 8) are the strongest changes here. The HashSet ordering note is low risk but worth keeping in mind for long-term maintainability. No blocking issues.

[claude]

Code Review: perf engine discovery/init path cleanup (Part C)

Solid, targeted performance work across the discovery/init path. Most changes are straightforward and well-executed. One semantic concern is worth investigating before merging.

---

Concern: HasFailures does not account for retries

File: TUnit.Core/Models/TestSessionContext.cs, TUnit.Engine/Services/TestExecution/TestCoordinator.cs

MarkFailure() is called eagerly when a test transitions to Failed/Timeout/Cancelled. There is no corresponding decrement if the test is later retried and passes.

The original O(N) scan read final result state at the time the after-session hook ran:

// Original — reflects final outcome after retries complete
AllTests.Any(t => t.Result is { State: TestState.Failed or Timeout or Cancelled })

With the new implementation, any test that fails an intermediate retry attempt permanently sets HasFailures = true, even if the session ultimately ends with zero failed tests. Users relying on after-session hooks that branch on HasFailures (e.g. failure notifications, conditional teardown) would get incorrect behavior when [Retry] is in play.

Suggested approach: Gate MarkFailure() on whether the test is in its truly final state. If TUnit distinguishes "retrying" from "finally failed" in TestCoordinator, call MarkFailure() only in the final-failure branch, not the retry-attempt branch. Alternatively, track a final-pass counter alongside the failure counter and expose HasFailures => _failureCount > _retriedToPassCount.

This needs verification — if retried tests always produce a fresh TestContext so the original Failed context is never in AllTests by the time the hook runs, the concern may not apply and the semantics are equivalent. Worth a quick confirmation either way.

---

Minor: volatile + ??= on TestContext.Id — the comment is doing heavy lifting

File: TUnit.Core/TestContext.cs:52

The multi-line comment explaining why volatile is needed here is good, but the design itself is fragile. Two threads can both observe null, both materialize the string, and both write — which is benign only because Guid.ToString() on the same Guid always yields equal strings. If a future maintainer changes the ID format to include a counter or timestamp, this becomes a silent correctness bug.

Lazy<string> or Interlocked.CompareExchange(ref _idString, _idGuid.ToString(), null) makes the intent explicit and eliminates the race documentation burden:

public string Id => Interlocked.CompareExchange(ref _idString, _idGuid.ToString(), null) 
                    ?? _idString!;

Or more idiomatically with LazyInitializer.EnsureInitialized. The performance difference is negligible for a field that is accessed at most once per test in normal flow.

---

What's well done

• TestGroupingService split — the ValueTask<GroupedTests> fast-path using return new ValueTask<GroupedTests>(...) for the synchronous case is the right pattern. Zero state-machine overhead on the hot path, and the null-conditional traceMessages?.Add(...) correctly short-circuits string interpolation evaluation when tracing is disabled.

• SortAndProject<TContext> helper — three identical sort+project chains reduced to one. The List.Sort with a Comparison<T> delegate matches the original OrderBy.ThenBy semantics exactly (stable sort, same key precedence), so this is a safe consolidation.

• Volatile.Read(ref _failureCount) on the reader side — correct. Interlocked.Increment provides a full fence on the write side; Volatile.Read is the minimal correct fence on the read side. No over-synchronization.

• RemoveFromRegistry() instance method — avoiding string materialization in the cleanup path is a nice touch; the string Id property was the only path that allocated a string from the Guid, and cleanup doesn't need it.

• InitializeTestCounts accepting IReadOnlyList<AbstractExecutableTest> — eliminates the intermediate Select projection and avoids the hidden ToList() that the old as IList<T> fallback forced. The for (var i = 0; ...) loop is preferable here since the index avoids an enumerator allocation.

[claude]

Code Review — PR #5611: Engine Discovery/Init Path Cleanup (Part C)

This is a well-scoped, performance-focused PR. The changes are individually motivated and most implementations are correct. I have one semantic concern about retry correctness, a note on hash-set ordering, and a few minor observations below.

---

TestGroupingService: Sync/async hot-path split

Verdict: Good, with one behavioral note.

The split is correctly implemented. Returning new ValueTask<GroupedTests>(result) for the non-tracing hot path eliminates the async state-machine allocation entirely — this is the right pattern for ValueTask optimization.

Behavioral change worth noting: trace messages are now buffered in a List<string> and emitted after all grouping completes, rather than interleaved inline. Under parallel test-session output, grouping traces will appear as a batch at the end of the grouping phase. Fine for production use, but worth noting if anyone uses trace logs for debugging ordering issues.

Minor inconsistency: Three branches use traceMessages?.Add(...) (null-conditional), while the notInParallel != null branch uses an explicit if (traceMessages != null) block. The explicit block is correct there (it avoids allocating keys when tracing is off), but a short comment would make the asymmetry look intentional rather than accidental.

---

LINQ → direct loops + SortAndProject

Files: HookDelegateBuilder.cs, EventReceiverOrchestrator.cs, TestDiscoveryService.cs, TestSessionCoordinator.cs

Verdict: Clean and correct.

SortAndProject<TContext> consolidates three identical sort+project chains. List<T>.Sort with a Comparison<T> delegate is a stable sort with the same semantics as OrderBy(...).ThenBy(...), so this is a safe refactor.

TestDiscoveryContext.BuildUniqueAssemblies/BuildUniqueTestClasses — ordering caveat:

// Before: LINQ Distinct() — documents first-occurrence insertion order
TestClasses.Select(x => x.AssemblyContext).Distinct().ToArray()

// After: HashSet<T> + CopyTo — iteration order is an implementation detail, not guaranteed
var seen = new HashSet<AssemblyHookContext>();
...
seen.CopyTo(result);

In practice, .NET's HashSet<T> iterates in insertion order for small sets, but the framework specification does not guarantee it. If the order of Assemblies or TestClasses affects hook execution sequencing (e.g., assembly-level before-hooks run in assembly order), a later .NET runtime change could silently reorder things. Low risk today, but if order matters here, consider a List<T> + HashSet<T> pair (append to list only when seen.Add returns true) to get both deduplication and guaranteed insertion-order iteration.

---

TestSessionContext.HasFailures atomic counter

Files: TestSessionContext.cs, TestCoordinator.cs, HookExecutor.cs

Verdict: Semantically correct — but one edge case worth verifying.

Volatile.Read + Interlocked.Increment is the right primitive pair here. The O(1) replacement of the O(N) AllTests.Any(...) scan in ExecuteAfterTestSessionHooksAsync is the highest-value change in this PR.

Retry semantics: MarkFailure() is reached only after all retry attempts are exhausted (since RetryHelper.ExecuteWithRetry absorbs intermediate failures and only re-throws on the last attempt, which propagates to catch → MarkFailed → switch). Semantics are equivalent to the original scan.

One edge case not covered: MarkCircularDependencyFailed and MarkDependencyResolutionFailed in TestStateManager set test.State = TestState.Failed but are invoked from TestDependencyResolver, which does not route through the finally-block switch. These tests will appear as Failed in the results without ever calling MarkFailure(). If HasFailures is used to decide whether to flag the session as having errors, dependency-resolution failures would not increment the counter. Worth explicitly verifying this is the intended behavior.

---

TestContext.Id lazy string + Guid registry

File: TUnit.Core/TestContext.cs

Verdict: Correct.

volatile string? _idString + _idString ??= _idGuid.ToString() is correct: volatile ensures cross-thread visibility on weak-memory-order platforms (ARM, WASM, AOT), the benign double-ToString race is harmless because two Guid.ToString calls on the same Guid produce equal strings, and reference assignment is atomic. The inline comment documenting this reasoning is exactly right.

RemoveById(string id) — consider removing if unused:

internal static void RemoveById(string id)
{
    if (Guid.TryParse(id, out var guid))
        _testContextsByGuid.TryRemove(guid, out _);
}

If there are no remaining callers (the coordinator now calls RemoveFromRegistry() directly), this method should be removed to avoid confusion. If it must stay, note that a caller passing an invalid (non-Guid) string will now silently do nothing — make sure that's the intended behavior.

---

Summary

Area	Verdict	Action
Sync hot path + buffered trace messages	✅ Good	Minor: comment on if vs ?. asymmetry
SortAndProject + loop refactors	✅ Clean	Consider List+HashSet pair if assembly/class order matters for hooks
HasFailures atomic counter	✅ Correct	Verify dependency-resolution failures path also triggers MarkFailure
Lazy Id string + Guid registry	✅ Correct	Consider removing unused RemoveById

No blocking issues. The most substantive thing to verify before merge is whether MarkCircularDependencyFailed/MarkDependencyResolutionFailed paths need to call MarkFailure() as well.

[thomhurst]

Thanks for the follow-up review. Addressing both points:

Re: HasFailures + retries — not an issue. RetryHelper.ExecuteWithRetry catches intermediate exceptions inside its own loop and only rethrows on final failure. The MarkFailure() call in TestCoordinator lives in the post-lifecycle switch (test.State) block, which runs once per test after all retry attempts complete. So test.State reflects the final outcome and MarkFailure() is never called for intermediate retry failures. Semantics match the original AllTests.Any(...) scan exactly.

Re: TestContext.Id lazy materialization — fair point. Switched to Interlocked.CompareExchange so we publish a single string even if the Id format ever becomes non-deterministic. Removes the documentation burden too.