perf(engine): lazy hook metadata registration (#5448)

[thomhurst]

Closes #5448

Summary

• Hook collections store LazyHookEntry (factory + cached singleton); heavy metadata now built once on first execution
• Module-init cost reduced to a static-lambda + index allocation per hook
• RegistrationIndex still computed eagerly to preserve registration ordering
• Source-gen snapshots regenerated

Test plan

• Source-gen snapshot tests pass on net472, net8.0, net9.0, net10.0
• Public API snapshot tests pass on all 4 TFMs
• Engine builds clean (net10.0)
• Hook integration slices pass (SimpleHookTest, BasicTests, EnsureBeforeEveryClassHit, Verify_TestSession_Hook_In_Referenced_Library_Executed, HelperTest_To_Trigger_Assembly_Hooks)

[codacy-production]

Not up to standards ⛔

🔴 Issues 1 minor

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

Results:
1 new issue

Category	Results
CodeStyle	1 minor

View in Codacy

🟢 Metrics -17 complexity

Metric	Results
Complexity	-17

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[claude]

Code Review: perf(engine): lazy hook metadata registration (#5750)

This is a solid, well-executed performance optimization. The core idea — separating "register a hook exists" (cheap, module-init) from "build its metadata" (expensive, deferred) — is architecturally sound and follows established lazy initialization patterns.

---

What works well

Design of LazyHookEntry<T> is clean. Two constructors (factory-based for source-gen, pre-materialized for reflection mode) mean the lazy wrapper is transparent to callers. Clearing _factory = null after first materialization is a nice touch — it allows the closure to be GC'd early.

Preserving registration order across lazy initialization is the tricky part of this change, and it's handled correctly: RegistrationIndex is captured eagerly at module-init (via GetNext*()) so hook ordering is deterministic even when materialization happens out-of-declaration order.

BuildGlobalHooksAsync generic helper (lines 86–110) nicely eliminates the duplication that would otherwise exist across the 10 global hook types.

SortAndProject with a static lambda (line 115) avoids a per-call closure allocation on the sort comparator.

---

Concerns and suggestions

1. Missing volatile on _materialized — subtle correctness risk

private T? _materialized;

The double-check locking (DCL) pattern requires either a volatile field or use of Volatile.Read/Volatile.Write to ensure the first null-check outside the lock sees the latest value across all CPU cores.

Without volatile, the JIT is allowed to cache the _materialized read in a register, meaning a second thread could perpetually see null after materialization and keep spinning into the lock (or in a degenerate JIT path, read a partially-constructed object on certain CPU architectures).

In practice the x64 CLR JIT rarely generates such code, but this is spec-undefined behavior and will fail Roslyn's thread-safety analyzers. The fix is one keyword:

private volatile T? _materialized;

Alternatively, replace the manual DCL with Lazy<T> (with LazyThreadSafetyMode.ExecutionAndPublication), which handles the volatile semantics internally and makes the intent self-documenting:

private readonly Lazy<T> _lazy;
// ...
public T Materialize() => _lazy.Value;

The Lazy<T> approach is simpler, provably correct, and the CLR implementation is well-tested. The trade-off is a small extra allocation per entry (the Lazy<T> wrapper), but that's marginal compared to the metadata graph it's guarding.

---

2. Race in BuildAndCacheBeforeTestHooksAsync causes redundant materialization

private async ValueTask<IReadOnlyList<NamedHookDelegate<TestContext>>> BuildAndCacheBeforeTestHooksAsync(Type testClassType)
{
    var hooks = await BuildBeforeTestHooksAsync(testClassType).ConfigureAwait(false);
    _beforeTestHooksCache.TryAdd(testClassType, hooks);
    return hooks;
}

Two threads racing to the first test of the same class will both call BuildBeforeTestHooksAsync concurrently, both build the full sorted list, and the loser's result is discarded. LazyHookEntry.Materialize() is idempotent so this is safe, but it wastes work (the loser builds the list for nothing).

The conventional fix is GetOrAdd with an async factory — but ConcurrentDictionary doesn't have that natively. A common alternative is Lazy<Task<T>> as the dictionary value, so only one thread ever starts the async build:

private readonly ConcurrentDictionary<Type, Lazy<Task<IReadOnlyList<NamedHookDelegate<TestContext>>>>> _beforeTestHooksCache = new();

public ValueTask<IReadOnlyList<NamedHookDelegate<TestContext>>> CollectBeforeTestHooksAsync(Type testClassType)
{
    var lazyTask = _beforeTestHooksCache.GetOrAdd(
        testClassType,
        static (t, self) => new Lazy<Task<IReadOnlyList<NamedHookDelegate<TestContext>>>>(
            () => self.BuildBeforeTestHooksAsync(t), 
            LazyThreadSafetyMode.ExecutionAndPublication),
        this);
    return new ValueTask<IReadOnlyList<NamedHookDelegate<TestContext>>>(lazyTask.Value);
}

That said, the current behavior is correct and the wasted work is bounded (at most N_threads redundant list builds per type, each happening at most once). If the hook-execution hot path isn't showing contention in practice, this is "nice to have" rather than blocking.

---

3. Non-static comparator lambda in SortAndAddHooks

// Line 163
hooks.Sort((a, b) => a.order != b.order
    ? a.order.CompareTo(b.order)
    : a.registrationIndex.CompareTo(b.registrationIndex));

SortAndProject (line 115) correctly uses static (a, b) => ... — same pattern should apply here to be consistent and avoid a per-call closure allocation on a method that may be called for every class.

---

4. BuildGlobalHooksAsync materializes hooks sequentially despite being async

foreach (var entry in sourceHooks)
{
    var hook = entry.Materialize();            // sync
    var namedHook = await createDelegate(hook); // async
    hooks.Add((hook.Order, hook.RegistrationIndex, namedHook));
}

The createDelegate is awaited one at a time. If delegate creation ever becomes non-trivially async (e.g., loading assemblies), the sequential ordering might become a bottleneck. Since this runs during InitializeAsync() before any tests start, it's fine for now — but worth noting if performance ever becomes an issue here.

---

Minor observations

• The SortAndProject call in BuildGlobalHooksAsync (line 109) and SortAndAddHooks in BuildBeforeTestHooksAsync (line 273) perform separate but semantically identical sorts. These could share a single static comparator or IComparer<T> to keep the logic in one place.

• Generated code indentation: the static __registrationIndex => line in snapshots is indented with 4 spaces, while the outer lambda block uses 8 spaces. This is cosmetic but could be cleaned up in the generator.

---

Summary

The lazy registration approach is the right call — it trades a small per-entry allocation for a large reduction in module-init cost, which is important for large test suites that use many hooks. The RegistrationIndex-at-registration-time trick elegantly preserves ordering without requiring synchronization at sort time.

The primary concern is the missing volatile on _materialized, which is a latent correctness issue. Everything else (the ordering race in the per-type cache, the non-static lambda) is a minor improvement opportunity. Happy to approve once the volatile/Lazy<T> concern is addressed.

[claude]

Follow-up Review: perf(engine): lazy hook metadata registration (#5750)

All three critical concerns from the previous review have been addressed. This is now in good shape.

---

Previous concerns — all resolved ✅

1. Missing volatile on _materialized — FIXED

The field is now correctly declared private volatile T? _materialized (LazyHookEntry.cs:25), and there's even an inline comment explaining exactly why volatile is required for the double-checked-lock pattern on weakly-ordered architectures. Well done.

2. Race causing redundant materialization in per-type hook cache — FIXED

The cache now uses ConcurrentDictionary<Type, Lazy<Task<IReadOnlyList<...>>>> throughout HookDelegateBuilder.cs (lines 24–29), and GetOrBuildHooksAsync (lines 197–213) implements the single-flight Lazy<Task<T>> pattern correctly. Racing threads observe the same Lazy instance and await the same underlying build.

3. Non-static comparator lambda in SortAndAddHooks — FIXED

SortAndAddHooks (line 182) now uses static (a, b) => ..., matching SortAndProject.

---

Remaining observations (non-blocking)

1. Redundant Materialize() calls in TestDiscovery duplicate check (ReflectionHookDiscoveryService.cs:494–495, 677–678)

if (!Sources.AfterTestDiscoveryHooks.Any(h => h.Materialize().MethodInfo.Name == discoveryMetadata.Name &&
                                               h.Materialize().MethodInfo.Type == discoveryMetadata.Type))

Two issues here:

• Materialize() is called twice per hook in the predicate — the result should be stored in a local.
• More importantly, this secondary dedup check appears to be dead code. Both RegisterAfterHook and RegisterAfterEveryHook already guard against re-registration via _registeredMethods.TryAdd(methodKey, true) at the top of each method (lines 441–444, 609–612). By the time execution reaches the TestDiscovery case, the method is already claimed. This means the Any(h => h.Materialize()...) scan iterates and eagerly materializes all existing AfterTestDiscoveryHooks entries for no benefit.

If the intent is to avoid duplicates across [After(TestDiscovery)] and [AfterEvery(TestDiscovery)] on the same method, the _registeredMethods key (derived from GetMethodKey(method)) already handles that. The Any() check can be safely removed.

2. Per-entry _lock object is never freed after materialization

After Materialize() runs, the _factory is nulled out (good — allows the closure to be GC'd), but _lock stays allocated for the lifetime of the entry. For large test suites with many hooks this is a small but persistent overhead. An alternative that avoids the per-entry lock entirely:

public T Materialize()
{
    if (_materialized is not null) return _materialized;
    var built = _factory!(RegistrationIndex);
    // Publish — last writer wins, all readers see a valid object
    Volatile.Write(ref _materialized, built);
    _factory = null;
    return _materialized;
}

This is lock-free and safe because T is a reference type (constructing it twice is idempotent — both produce an equivalent object). The lock pattern prevents the second call entirely; the Volatile.Write pattern accepts that two threads may both construct and the winner is arbitrary but correct. Given that materialization is a one-shot event at startup, either approach is fine — but the lock-free version removes an allocation per hook entry.

3. Generated code indentation (cosmetic)

The static __registrationIndex => line in generated snapshots is still indented with 4 spaces while the surrounding registration block uses 8 spaces. Not a correctness issue.

---

Verdict

The primary correctness concern (volatile) and the main performance concern (single-flight cache) are both properly addressed. The lazy registration design is sound: registration index is captured eagerly for deterministic ordering, while the heavy MethodMetadata/ClassMetadata graph is deferred to first execution. This is a meaningful startup-performance improvement for test suites with many hooks.

The two remaining points above (redundant Any() materialization, per-entry lock overhead) are minor cleanup opportunities — not blocking. Happy to approve.

[claude]

Third-Pass Review: perf(engine): lazy hook metadata registration (#5750)

This review checks the current HEAD (96c0b8a) against the two remaining non-blocking observations flagged in the previous follow-up review.

---

Previous reviews summary

First review raised three critical issues (missing volatile, race condition in per-type hook cache, non-static sort comparator) — all fixed in the follow-up commit.

Second review marked two items as non-blocking:

1. Redundant Materialize() calls in ReflectionHookDiscoveryService duplicate check
2. Per-entry _lock object never freed after materialization

---

Status of remaining observations

1. Redundant Materialize() calls — still present (non-blocking, but worth a note)

ReflectionHookDiscoveryService.cs lines 494–495 and 677–678:

if (!Sources.AfterTestDiscoveryHooks.Any(h => h.Materialize().MethodInfo.Name == discoveryMetadata.Name &&
                                               h.Materialize().MethodInfo.Type == discoveryMetadata.Type))

Materialize() is called twice per element in the bag. Since this is reflection mode and hooks are pre-materialized (the LazyHookEntry(T hook) constructor path), the second call is a no-op (hits the if (_materialized is not null) fast path), so there's no functional bug — but it's still redundant allocation/branching. A var m = h.Materialize(); local would clean this up.

More importantly, as noted last review, this Any() scan appears to be dead code — the _registeredMethods.TryAdd(methodKey, true) guard at the top of RegisterAfterHook/RegisterAfterEveryHook already prevents the same method from being registered twice, making this secondary scan unreachable in practice. If this is intentional (e.g., protecting against future refactors that drop the _registeredMethods guard), a comment explaining why would make this self-documenting.

2. Per-entry _lock allocation — still present (acknowledged trade-off)

LazyHookEntry<T> allocates a private object _lock = new() per hook entry and never clears it after materialization. The previous review suggested a lock-free alternative using Volatile.Write. This remains valid feedback, though for hooks specifically (construction isn't truly idempotent if the factory has side effects like Interlocked.Increment) the lock semantics may be intentional. If the factory is guaranteed side-effect-free at-most-once, the lock-free path would remove one allocation per hook entry. If side-effects matter, the current approach is correct.

---

New observation

3. _processedHooks keyed on HookMethod reference (minor)

In HookDelegateBuilder.cs, ProcessHookRegistrationAsync uses a ConcurrentDictionary<object, bool> _processedHooks to deduplicate event processing:

if (!_processedHooks.TryAdd(hookMethod, true))
{
    return;
}

This deduplicates on object identity (reference equality), which is correct since each Materialize() returns the same cached instance. However, if HookMethod ever overrides Equals/GetHashCode in the future (e.g., for value semantics), this would silently break. Using a HashSet<HookMethod> with a custom ReferenceEqualityComparer (or using ConditionalWeakTable to avoid preventing GC) would make the intent explicit. Not a current bug, but a subtle coupling assumption.

---

Overall

The architecture is sound and the critical correctness issues from prior reviews are fully resolved. The two carry-over items from the previous review remain minor. This PR is ready to merge.

One small ask: consider either removing the unreachable Any(h => h.Materialize()...) duplicate checks (they add noise and confusion), or adding a comment explaining why they need to exist despite the _registeredMethods guard.

[claude]

Fourth-Pass Review: perf(engine): lazy hook metadata registration (#5750)

This pass checks the latest commit (9b95cbee) against the two remaining non-blocking observations from the third-pass review.

---

Previous review history

Review	Critical items	Status
First pass	volatile missing, race in per-type hook cache, non-static comparator	All fixed
Second pass	Redundant Materialize() calls, _lock never freed	Noted as non-blocking
Third pass	Both carry-over items still present, _processedHooks identity-dedup contract noted	All noted as non-blocking

---

Status of remaining items

1. Redundant Materialize() calls — FIXED ✅

Both instances in ReflectionHookDiscoveryService.cs (lines 494 and 679) now hoist h.Materialize() into a local variable m before the predicate. The added comment explaining that this is a defence-in-depth guard despite _registeredMethods short-circuiting above is the right call — it prevents future readers from deleting the check as dead code without understanding the intent.

2. _processedHooks reference-identity comment — ADDED ✅

HookDelegateBuilder.cs now has a clear inline comment explaining:

• dedup relies on reference identity (not value equality)
• correctness assumption: Materialize() returns the same cached instance
• what to revisit if HookMethod ever overrides Equals/GetHashCode

This is exactly what was asked for — the contract is now self-documenting.

3. Per-entry _lock allocation — unchanged (acknowledged trade-off)

The object _lock = new() per LazyHookEntry is still present and intentional. As noted previously, since the factory can have side effects (like Interlocked.Increment) and must execute exactly once, the lock semantics are correct and the cost is bounded to one allocation per hook registration. This is a fine trade-off for correctness.

---

Overall assessment

All critical issues and all non-blocking observations from the prior three reviews have now been addressed — either fixed or explicitly documented. The architecture is sound, the thread safety is correct, and the code is now self-documenting at the key decision points.

This PR is ready to merge. 🟢