perf: reduce allocations in EventReceiverOrchestrator and TestContextExtensions

[thomhurst]

Summary

• Remove redundant .ToArray() in SortAndFilter: FilterScopedAttributes already returns T[], so calling .ToArray() on the result was creating an unnecessary array copy on every call
• Eliminate double LINQ sort in InvokeHookRegistrationEventReceiversAsync: Was calling .OrderBy() before FilterScopedAttributes AND .OrderBy() after it. Replaced with a single in-place Array.Sort after filtering
• Add TryGetValue fast path before GetOrAdd in InvokeFirstTestInSession/Assembly/Class: The GetOrAdd lambda captures context, sessionContext/assemblyContext/classContext, and cancellationToken, causing a closure+delegate allocation on every call even when the key exists. The manual TryGetValue check avoids this allocation on the common fast path (all tests after the first)
• Make sort comparison lambda static in SortAndFilter to prevent implicit capture

Test plan

• Verify build succeeds across all target frameworks (net8.0, net9.0, net10.0, netstandard2.0)
• Run existing event receiver tests to confirm no behavior changes
• All changes are allocation-only optimizations with no behavioral impact

[claude]

Code Review

Overall this is a solid set of allocation-reduction improvements aligned with TUnit's performance-first principle. The changes are well-explained and targeted. A few observations:

✅ static lambda in SortAndFilter (TestContextExtensions.cs)

Good catch — static lambdas are guaranteed not to capture state and avoid the display class allocation. The compiler was likely already doing this, but making it explicit is a good self-documenting constraint.

✅ Remove redundant .ToArray() in SortAndFilter

Correct. FilterScopedAttributes already returns T[] (via vlb.AsSpan().ToArray()), so the .ToArray() call was doubling up on array allocation unnecessarily. Clean fix.

✅ TryGetValue fast path before GetOrAdd (session/assembly/class)

The reasoning is sound — ConcurrentDictionary.GetOrAdd(key, valueFactory) always allocates a closure/delegate for the factory even when the key already exists (since the factory delegate must be constructed before the call). The TryGetValue short-circuit avoids that on the hot path (every test after the first). Thread-safety is preserved: ConcurrentDictionary is safe for concurrent reads, and if two threads race past TryGetValue, GetOrAdd still correctly returns the same stored Task.

⚠️ Potential behavioral change: pre-sort removal in InvokeHookRegistrationEventReceiversAsync

The PR description says "all changes are allocation-only optimizations with no behavioral impact", but removing the .OrderBy() before FilterScopedAttributes can affect which scoped attribute is retained when there are multiple IHookRegisteredEventReceiver attributes sharing the same IScopedAttribute.ScopeType.

FilterScopedAttributes uses TryAdd — it keeps the first encountered:

scopedAttributesByType.TryAdd(scopedAttribute.ScopeType, item);

• Before: input was pre-sorted by Order, so the attribute with the lowest Order was kept.
• After: input is in declaration/attribute-collection order, so the attribute appearing first in the array is kept.

For the typical case (one attribute per scope type per hook), this makes no difference. But if two scoped attributes of the same type are applied to a hook with different Order values, the one that "wins" may change.

This is likely a rare or contrived scenario in practice, but since the PR description claims no behavioral change, it's worth acknowledging or verifying that the pre-sort was truly vestigial (e.g., if hookContext.HookMethod.Attributes is already ordered by declaration position and scoped types are enforced as unique by design).

If the existing pre-sort behavior (lowest Order wins for scoped deduplication) was intentional, consider:

// Sort first so lowest-Order wins during scoped deduplication
hookContext.HookMethod.Attributes
    .OfType<IHookRegisteredEventReceiver>()
    .OrderBy(static r => r.Order)

...then pass the sorted sequence to FilterScopedAttributes, and skip the post-Array.Sort (since the output order is already determined).

Or, if ordering within scoped deduplication is unimportant, document that assumption explicitly.

Summary

Three of the four changes are clearly correct and safe. The fourth (removing the pre-sort in InvokeHookRegistrationEventReceiversAsync) is very likely fine in practice but is technically a behavioral change in an edge case that the PR description doesn't account for. Worth a quick confirmation that hook attributes of the same scoped type per hook are either impossible by design or that declaration order is acceptable as the tiebreaker.

[claude]

Code Review

Summary

This is a focused, well-reasoned allocation reduction PR. The changes are correct and the PR description accurately documents the intent. A few observations worth noting:

---

— removal ✅

Clean fix. already returns , so the redundant copy was pure waste. The lambda annotation is also a good catch to prevent implicit capture.

---

— double sort ✅ (with a note)

The pre-sort → filter → post-sort pattern is correct and necessary, and the comment explains it well. Worth explicitly noting the semantic improvement: the old code used order (arbitrary) to determine which scoped attribute was kept during deduplication, then sorted after. The new code pre-sorts by Order so that FilterScopedAttributes's TryAdd-first-wins behavior now deterministically retains the lowest-Order scoped attribute per ScopeType. This is a subtle but meaningful correctness improvement, not just a perf change.

One thing to be aware of: FilterScopedAttributes appends scoped items from a Dictionary<Type, T>, which in .NET has implementation-defined iteration order. The post-filter Array.Sort handles this correctly by re-sorting everything, so the behavior is well-defined. No issue here, just worth being aware of.

---

+ fast path ✅

The pattern is correct and this is the canonical way to avoid closure allocation on the hot path with . A few things to confirm:

1. Race safety: If two threads both fail , they both fall through to . 's factory may be called twice, producing two instances, but only one is stored — the other is abandoned. This is a pre-existing characteristic of the original usage, not introduced by this PR. It's fine here because the tasks are idempotent in effect (receivers are invoked once per task, but the abandoned task is never awaited).

2. The optimization is valid: On the common path (2nd+ test in session/assembly/class), returns true and the closure + delegate allocation never happens. The savings are real.

---

Minor

The attributes already present on the three methods pair well with this pattern since the fast path is now just a single dictionary lookup + .

---

Overall this is a clean, low-risk perf improvement. The changes are well-documented and the optimizations are all valid. No issues to block merging.

[claude]

Code Review

Summary

This is a focused, well-reasoned allocation reduction PR. The changes are correct and the PR description accurately documents the intent. A few observations:

---

SortAndFilter — .ToArray() removal ✅

Clean fix. FilterScopedAttributes already returns T[], so the redundant .ToArray() copy was pure waste. The static lambda annotation is also a good catch.

---

InvokeHookRegistrationEventReceiversAsync — double sort ✅ (with a note)

The pre-sort → filter → post-sort pattern is correct and necessary, and the comment explains it well. Worth noting explicitly: this is a subtle semantic improvement too, not just a perf change.

The old code used .OfType<>() iteration order (arbitrary) to determine which scoped attribute was retained during deduplication. The new code pre-sorts by Order so that FilterScopedAttributes's TryAdd-first-wins behavior now deterministically retains the lowest-Order scoped attribute per ScopeType. This is more correct behavior.

One thing to be aware of: FilterScopedAttributes appends scoped items from a Dictionary<Type, T> (implementation-defined iteration order), so their relative ordering is undefined after filtering. The post-filter Array.Sort re-sorts everything, which handles this correctly. No issue — just worth understanding the invariant.

---

TryGetValue + GetOrAdd fast path ✅

The pattern is correct. A few things worth noting:

1. Race safety: If two threads both fail TryGetValue, they both fall through to GetOrAdd. The factory delegate may be called twice, producing two Task instances — but only one is stored and the other is abandoned. This is a pre-existing property of GetOrAdd, not introduced here, and it's acceptable because the abandoned task is never awaited.

2. The optimization is valid: On the common path (2nd+ test in session/assembly/class), TryGetValue returns true and the closure allocation is avoided entirely. Given these methods are hot paths, the savings are real.

The AggressiveInlining attributes already on these methods pair nicely with this pattern — the fast path is now just a dictionary lookup + new ValueTask(existing).

---

Overall

Clean, low-risk performance improvement. The changes are well-documented and all optimizations are valid. No concerns blocking merge.

[claude]

Follow-up Review

The author has addressed the behavioral concern raised in my previous review by restoring the .OrderBy(static x => x.Order) pre-sort before passing attributes to FilterScopedAttributes. The updated comment also clearly explains the semantic intent:

"Pre-sort by Order before filtering so that FilterScopedAttributes (which uses TryAdd and keeps the first encountered item per ScopeType) retains the lowest-Order attribute."

All changes now look correct ✅

1. static lambda in SortAndFilter — prevents implicit capture, good self-documenting constraint.
2. Redundant .ToArray() removal — correct, FilterScopedAttributes already returns T[].
3. TryGetValue fast path before GetOrAdd — avoids closure allocation on the hot path (every test after the first), thread-safe.
4. Pre-sort → filter → in-place sort in InvokeHookRegistrationEventReceiversAsync — the pre-sort now correctly ensures the lowest-Order attribute wins during scoped deduplication, and the post-sort handles the dictionary iteration order indeterminacy from FilterScopedAttributes.

No concerns. This is a clean, low-risk allocation reduction that aligns well with TUnit's performance-first principle. Ready to merge.