From 365bf91d1bb4907e259ff418457a16d3a1ad65d2 Mon Sep 17 00:00:00 2001 From: Tom Longhurst <30480171+thomhurst@users.noreply.github.com> Date: Fri, 24 Apr 2026 12:50:40 +0100 Subject: [PATCH 1/3] perf(engine): replace global lock in EventReceiverRegistry with lock-free CAS (#5715) RegisterReceivers took a global Lock once per test; with ProcessorCount*4 concurrent tests this showed up in traces as Monitor.Enter_Slowpath at 2.16% self. The registry is conceptually a copy-on-write Dictionary and doesn't need a shared mutex. Replace: - Dictionary + Lock -> ConcurrentDictionary with copy-on-write append via AddOrUpdate (inherent CAS-retry semantics). - volatile EventTypes _registeredEvents -> int with Interlocked.CompareExchange OR loop so concurrent registrations never drop flag bits. - Lock-protected cache miss in GetReceiversOfType -> snapshot + source-identity validation after GetOrAdd; if a writer raced, evict our stale entry and retry so the next reader rebuilds from the latest source. Receiver discovery semantics are unchanged: receivers keyed by interface type, queried via GetReceiversOfType(). Cache invalidation on writer (TryRemove per-type) already existed; extended with retry to close the race window that the old lock previously masked. Closes #5715 --- TUnit.Engine/Events/EventReceiverRegistry.cs | 188 ++++++++++++------- 1 file changed, 116 insertions(+), 72 deletions(-) diff --git a/TUnit.Engine/Events/EventReceiverRegistry.cs b/TUnit.Engine/Events/EventReceiverRegistry.cs index f2517a6916..e8566fce6b 100644 --- a/TUnit.Engine/Events/EventReceiverRegistry.cs +++ b/TUnit.Engine/Events/EventReceiverRegistry.cs @@ -4,7 +4,9 @@ namespace TUnit.Engine.Events; -/// Fast registry for event receiver presence checks using bit flags +/// Fast registry for event receiver presence checks using bit flags. +/// Thread-safe without locks: uses ConcurrentDictionary + copy-on-write arrays +/// updated via AddOrUpdate. Reads are lock-free. internal sealed class EventReceiverRegistry { // Bit flags for fast checking @@ -25,42 +27,41 @@ private enum EventTypes All = ~0 } - private volatile EventTypes _registeredEvents = EventTypes.None; - private readonly Dictionary _receiversByType = new(); + // _registeredEvents is volatile for publication semantics; updated via Interlocked OR. + private int _registeredEvents; + + // Copy-on-write: each value is an immutable snapshot array. Writers replace the + // entry via AddOrUpdate with a new array that contains the appended receiver. + private readonly ConcurrentDictionary _receiversByType = new(); + + // Cache of typed arrays (T[]) built on first read per type. Invalidated by + // writers removing the entry for the affected interface type. private readonly ConcurrentDictionary _cachedTypedReceivers = new(); - private readonly Lock _lock = new(); /// - /// Register event receivers from a collection of objects + /// Register event receivers from a collection of objects. /// public void RegisterReceivers(ReadOnlySpan objects) { - lock (_lock) + foreach (var obj in objects) { - foreach (var obj in objects) - { - RegisterReceiverInternal(obj); - } + RegisterReceiverInternal(obj); } } /// - /// Register a single event receiver + /// Register a single event receiver. /// public void RegisterReceiver(object receiver) { - lock (_lock) - { - RegisterReceiverInternal(receiver); - } + RegisterReceiverInternal(receiver); } private void RegisterReceiverInternal(object receiver) { UpdateEventFlags(receiver); - // Register for each interface type the object implements - // We use a simpler approach that doesn't require reflection + // Register for each interface type the object implements. RegisterIfImplements(receiver); RegisterIfImplements(receiver); RegisterIfImplements(receiver); @@ -75,141 +76,184 @@ private void RegisterReceiverInternal(object receiver) private void RegisterIfImplements(object receiver) where T : class { - if (receiver is T) + if (receiver is not T) { - var interfaceType = typeof(T); - if (_receiversByType.TryGetValue(interfaceType, out var existing)) + return; + } + + var interfaceType = typeof(T); + + // Copy-on-write append. AddOrUpdate guarantees the updateValueFactory is + // retried until the CAS succeeds, so concurrent writers do not lose updates. + _receiversByType.AddOrUpdate( + interfaceType, + _ => [receiver], + (_, existing) => { var newArray = new object[existing.Length + 1]; existing.CopyTo(newArray, 0); - newArray[^1] = receiver; - _receiversByType[interfaceType] = newArray; - } - else - { - _receiversByType[interfaceType] = [receiver]; - } + newArray[existing.Length] = receiver; + return newArray; + }); - // Invalidate only the changed type instead of clearing the entire cache - _cachedTypedReceivers.TryRemove(interfaceType, out _); - } + // Invalidate only the changed type instead of clearing the entire cache. + // The next read of GetReceiversOfType will rebuild from the freshest + // snapshot in _receiversByType. + _cachedTypedReceivers.TryRemove(interfaceType, out _); } /// /// Fast check if any receivers registered for event type /// [MethodImpl(MethodImplOptions.AggressiveInlining)] - public bool HasTestStartReceivers() => (_registeredEvents & EventTypes.TestStart) != 0; + public bool HasTestStartReceivers() => (Volatile.Read(ref _registeredEvents) & (int)EventTypes.TestStart) != 0; [MethodImpl(MethodImplOptions.AggressiveInlining)] - public bool HasTestEndReceivers() => (_registeredEvents & EventTypes.TestEnd) != 0; + public bool HasTestEndReceivers() => (Volatile.Read(ref _registeredEvents) & (int)EventTypes.TestEnd) != 0; [MethodImpl(MethodImplOptions.AggressiveInlining)] - public bool HasTestSkippedReceivers() => (_registeredEvents & EventTypes.TestSkipped) != 0; + public bool HasTestSkippedReceivers() => (Volatile.Read(ref _registeredEvents) & (int)EventTypes.TestSkipped) != 0; [MethodImpl(MethodImplOptions.AggressiveInlining)] - public bool HasTestRegisteredReceivers() => (_registeredEvents & EventTypes.TestRegistered) != 0; + public bool HasTestRegisteredReceivers() => (Volatile.Read(ref _registeredEvents) & (int)EventTypes.TestRegistered) != 0; [MethodImpl(MethodImplOptions.AggressiveInlining)] - public bool HasFirstTestInSessionReceivers() => (_registeredEvents & EventTypes.FirstTestInSession) != 0; + public bool HasFirstTestInSessionReceivers() => (Volatile.Read(ref _registeredEvents) & (int)EventTypes.FirstTestInSession) != 0; [MethodImpl(MethodImplOptions.AggressiveInlining)] - public bool HasLastTestInSessionReceivers() => (_registeredEvents & EventTypes.LastTestInSession) != 0; + public bool HasLastTestInSessionReceivers() => (Volatile.Read(ref _registeredEvents) & (int)EventTypes.LastTestInSession) != 0; [MethodImpl(MethodImplOptions.AggressiveInlining)] - public bool HasFirstTestInAssemblyReceivers() => (_registeredEvents & EventTypes.FirstTestInAssembly) != 0; + public bool HasFirstTestInAssemblyReceivers() => (Volatile.Read(ref _registeredEvents) & (int)EventTypes.FirstTestInAssembly) != 0; [MethodImpl(MethodImplOptions.AggressiveInlining)] - public bool HasLastTestInAssemblyReceivers() => (_registeredEvents & EventTypes.LastTestInAssembly) != 0; + public bool HasLastTestInAssemblyReceivers() => (Volatile.Read(ref _registeredEvents) & (int)EventTypes.LastTestInAssembly) != 0; [MethodImpl(MethodImplOptions.AggressiveInlining)] - public bool HasFirstTestInClassReceivers() => (_registeredEvents & EventTypes.FirstTestInClass) != 0; + public bool HasFirstTestInClassReceivers() => (Volatile.Read(ref _registeredEvents) & (int)EventTypes.FirstTestInClass) != 0; [MethodImpl(MethodImplOptions.AggressiveInlining)] - public bool HasLastTestInClassReceivers() => (_registeredEvents & EventTypes.LastTestInClass) != 0; + public bool HasLastTestInClassReceivers() => (Volatile.Read(ref _registeredEvents) & (int)EventTypes.LastTestInClass) != 0; [MethodImpl(MethodImplOptions.AggressiveInlining)] - public bool HasAnyReceivers() => _registeredEvents != EventTypes.None; + public bool HasAnyReceivers() => Volatile.Read(ref _registeredEvents) != (int)EventTypes.None; public T[] GetReceiversOfType() where T : class { var typeKey = typeof(T); - // Lock-free fast path for cache hit (common case) - if (_cachedTypedReceivers.TryGetValue(typeKey, out var cached)) + while (true) { - return (T[])cached; - } - - // Simple lock for cache miss (rare, happens once per type) - lock (_lock) - { - // Double-check after acquiring lock - if (_cachedTypedReceivers.TryGetValue(typeKey, out cached)) + // Lock-free fast path: cache hit (common case after warmup). + if (_cachedTypedReceivers.TryGetValue(typeKey, out var cached)) { return (T[])cached; } - // Build and cache - if (_receiversByType.TryGetValue(typeKey, out var receivers)) + // Cache miss. Snapshot the source so we can validate via identity after publish. + _receiversByType.TryGetValue(typeKey, out var sourceSnapshot); + var typedArray = BuildTypedArray(sourceSnapshot); + + var published = (T[])_cachedTypedReceivers.GetOrAdd(typeKey, typedArray); + + // If a writer raced with us (mutated source between our snapshot and publish), + // the writer's TryRemove-on-cache could have been ordered before our GetOrAdd, + // leaving a stale value cached. Detect by re-reading source: if it changed, + // evict our entry so the next reader rebuilds. + _receiversByType.TryGetValue(typeKey, out var latest); + if (!ReferenceEquals(latest, sourceSnapshot)) { - var typedArray = new T[receivers.Length]; - for (var i = 0; i < receivers.Length; i++) + if (ReferenceEquals(published, typedArray)) { - typedArray[i] = (T)receivers[i]; + ((ICollection>)_cachedTypedReceivers) + .Remove(new KeyValuePair(typeKey, typedArray)); } - _cachedTypedReceivers[typeKey] = typedArray; - return typedArray; + continue; } - T[] emptyArray = []; - _cachedTypedReceivers[typeKey] = emptyArray; - return emptyArray; + return published; } } + private static T[] BuildTypedArray(object[]? source) where T : class + { + if (source is null || source.Length == 0) + { + return []; + } + + var typedArray = new T[source.Length]; + for (var i = 0; i < source.Length; i++) + { + typedArray[i] = (T)source[i]; + } + return typedArray; + } + private void UpdateEventFlags(object receiver) { + var flags = EventTypes.None; if (receiver is ITestStartEventReceiver) { - _registeredEvents |= EventTypes.TestStart; + flags |= EventTypes.TestStart; } if (receiver is ITestEndEventReceiver) { - _registeredEvents |= EventTypes.TestEnd; + flags |= EventTypes.TestEnd; } if (receiver is ITestSkippedEventReceiver) { - _registeredEvents |= EventTypes.TestSkipped; + flags |= EventTypes.TestSkipped; } if (receiver is ITestRegisteredEventReceiver) { - _registeredEvents |= EventTypes.TestRegistered; + flags |= EventTypes.TestRegistered; } if (receiver is IFirstTestInTestSessionEventReceiver) { - _registeredEvents |= EventTypes.FirstTestInSession; + flags |= EventTypes.FirstTestInSession; } if (receiver is ILastTestInTestSessionEventReceiver) { - _registeredEvents |= EventTypes.LastTestInSession; + flags |= EventTypes.LastTestInSession; } if (receiver is IFirstTestInAssemblyEventReceiver) { - _registeredEvents |= EventTypes.FirstTestInAssembly; + flags |= EventTypes.FirstTestInAssembly; } if (receiver is ILastTestInAssemblyEventReceiver) { - _registeredEvents |= EventTypes.LastTestInAssembly; + flags |= EventTypes.LastTestInAssembly; } if (receiver is IFirstTestInClassEventReceiver) { - _registeredEvents |= EventTypes.FirstTestInClass; + flags |= EventTypes.FirstTestInClass; } if (receiver is ILastTestInClassEventReceiver) { - _registeredEvents |= EventTypes.LastTestInClass; + flags |= EventTypes.LastTestInClass; + } + + if (flags == EventTypes.None) + { + return; } - } + // Atomic OR without a lock. Retries on contention. + var current = Volatile.Read(ref _registeredEvents); + while (true) + { + var desired = current | (int)flags; + if (desired == current) + { + return; // Already set. + } + var actual = Interlocked.CompareExchange(ref _registeredEvents, desired, current); + if (actual == current) + { + return; + } + current = actual; + } + } } From c599e084fa0e291d46804f39453c71f3409e7098 Mon Sep 17 00:00:00 2001 From: Tom Longhurst <30480171+thomhurst@users.noreply.github.com> Date: Fri, 24 Apr 2026 13:03:54 +0100 Subject: [PATCH 2/3] docs(engine): clarify lock-free EventReceiverRegistry semantics Address automated review feedback on #5731: - Fix misleading `volatile` comment; field uses Volatile.Read + Interlocked.CompareExchange - Document why the ICollection.Remove cast is used (netstandard2.0 TryRemove equivalent) - Note that batch RegisterReceivers is NOT atomic; callers must register before execution --- TUnit.Engine/Events/EventReceiverRegistry.cs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/TUnit.Engine/Events/EventReceiverRegistry.cs b/TUnit.Engine/Events/EventReceiverRegistry.cs index e8566fce6b..cd42f98cca 100644 --- a/TUnit.Engine/Events/EventReceiverRegistry.cs +++ b/TUnit.Engine/Events/EventReceiverRegistry.cs @@ -7,6 +7,13 @@ namespace TUnit.Engine.Events; /// Fast registry for event receiver presence checks using bit flags. /// Thread-safe without locks: uses ConcurrentDictionary + copy-on-write arrays /// updated via AddOrUpdate. Reads are lock-free. +/// +/// Batch is NOT atomic: a concurrent +/// reader may observe a subset of a batch because each receiver is registered independently. +/// Callers must not depend on all-or-nothing visibility. Current callers register receivers +/// before any test executes (see EventReceiverOrchestrator, which also dedups), so +/// this relaxation is safe. Future maintainers must preserve that invariant. +/// internal sealed class EventReceiverRegistry { // Bit flags for fast checking @@ -27,7 +34,8 @@ private enum EventTypes All = ~0 } - // _registeredEvents is volatile for publication semantics; updated via Interlocked OR. + // Accessed via Volatile.Read + Interlocked.CompareExchange to provide acquire/release + // semantics without the cost of a volatile field on every write path. private int _registeredEvents; // Copy-on-write: each value is an immutable snapshot array. Writers replace the @@ -165,6 +173,8 @@ public T[] GetReceiversOfType() where T : class { if (ReferenceEquals(published, typedArray)) { + // Conditional remove: only evicts the entry if its value still matches the stale typedArray. + // netstandard2.0-compatible equivalent of ConcurrentDictionary.TryRemove(KeyValuePair). ((ICollection>)_cachedTypedReceivers) .Remove(new KeyValuePair(typeKey, typedArray)); } From 48cffd24df5ad17b260949c97fbcc0dd4394a40c Mon Sep 17 00:00:00 2001 From: Tom Longhurst <30480171+thomhurst@users.noreply.github.com> Date: Fri, 24 Apr 2026 13:39:18 +0100 Subject: [PATCH 3/3] docs(engine): document termination contract of EventReceiverRegistry read loop Adds a WHY comment above the retry loop in GetReceiversOfType noting that termination relies on writers completing before steady-state reads. Registrations happen during setup; adversarial registration during test execution could cause the loop to spin under continuous writer pressure. Addresses non-blocking review suggestion on PR #5731. --- TUnit.Engine/Events/EventReceiverRegistry.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/TUnit.Engine/Events/EventReceiverRegistry.cs b/TUnit.Engine/Events/EventReceiverRegistry.cs index cd42f98cca..85cf83f0ab 100644 --- a/TUnit.Engine/Events/EventReceiverRegistry.cs +++ b/TUnit.Engine/Events/EventReceiverRegistry.cs @@ -150,6 +150,10 @@ public T[] GetReceiversOfType() where T : class { var typeKey = typeof(T); + // Termination contract: this retry loop relies on writers (RegisterReceivers) completing + // before steady-state reads. All registrations happen during test setup; during execution + // the receiver set is effectively frozen. A caller that registers receivers during test + // execution could cause this loop to spin under continuous writer pressure. while (true) { // Lock-free fast path: cache hit (common case after warmup).