perf: replace object locks with Lock type for efficient synchronization

[thomhurst]

Summary

• Replace object lock fields with System.Threading.Lock across the entire codebase for more efficient synchronization
• Introduce ConsoleLineBuffer class in TUnit.Core to encapsulate line buffer + Lock, solving cross-assembly polyfill type mismatch while keeping all locking internal
• Simplify OptimizedConsoleInterceptor by removing inline lock blocks in favor of ConsoleLineBuffer method calls

Changed files

File	Change
TUnit.Core/Logging/ConsoleLineBuffer.cs	New — encapsulates StringBuilder + Lock with Append/Drain/Flush operations
TUnit.Core/Context.cs	Replace raw StringBuilder+object buffer with ConsoleLineBuffer
TUnit.Core/AotCompatibility/GenericTestRegistry.cs	object → Lock
TUnit.Engine/Logging/OptimizedConsoleInterceptor.cs	Use ConsoleLineBuffer methods instead of inline lock blocks
TUnit.Engine/Logging/StandardOutConsoleInterceptor.cs	Return ConsoleLineBuffer instead of tuple
TUnit.Engine/Logging/StandardErrorConsoleInterceptor.cs	Return ConsoleLineBuffer instead of tuple
TUnit.Engine/Scheduling/ConstraintKeyScheduler.cs	object → Lock
TUnit.Playwright/BrowserTest.cs	object → Lock
TUnit.Mocks.Http/MockHttpHandler.cs	object → Lock
TUnit.Engine.Tests/ThreadSafeOutput.cs	object → Lock

Why Lock > object

The Lock type (polyfilled on pre-.NET 9) generates Lock.EnterScope() instead of Monitor.Enter/Exit when used with lock(). This avoids the Monitor slow path overhead that showed up as 1.62% exclusive time in CPU profiling.

Test plan

• All TFMs build (netstandard2.0, net8.0, net9.0, net10.0)
• TUnit.Profile: 1013/1013 tests pass
• CI pipeline passes

[claude]

Code Review: perf: replace object locks with Lock type for efficient synchronization

Overall this is a well-executed performance improvement. The changes are correct, the design is clean, and the encapsulation solves a real problem (cross-assembly polyfill type mismatch). A few observations below.

---

What's working well

ConsoleLineBuffer abstraction is well-designed. The semantically-named methods (Drain, AppendAndDrain, FlushIfNonEmpty) clearly express intent vs. raw lock (bufferLock) { ... } blocks. The calling code in OptimizedConsoleInterceptor is significantly easier to read.

Correctness is maintained. I verified that RouteToSinks is null-safe (if (message is not null && message.Length > 0)), so the new Flush() calling RouteToSinks(GetLineBuffer().FlushIfNonEmpty()) where content can be null is fine — same terminal behavior as the original guarded if (buffer.Length > 0).

The cross-assembly polyfill problem is correctly solved. Keeping Lock internal to ConsoleLineBuffer in TUnit.Core prevents consumers in other assemblies from needing to reference the polyfill directly.

ConstraintKeyScheduler change is correct. Lock doesn't have thread affinity and is safe to pass as a parameter to async methods.

---

Minor concern: Lazy<StringBuilder> inside ConsoleLineBuffer

// ConsoleLineBuffer.cs
private readonly Lazy<StringBuilder> _buffer = new(() => new StringBuilder());

Since ConsoleLineBuffer is now eagerly constructed as a field of Context (which is constructed per-test), the Lazy<T> wrapper defers exactly one small StringBuilder allocation — but adds the overhead of the Lazy<T> wrapper itself (object allocation + volatile read on access). In the hot path of console interception, this Lazy is likely always realized for tests that produce any output, making it a net negative.

Consider either:

• Direct initialization: private readonly StringBuilder _buffer = new StringBuilder(); — simpler, slightly faster, one fewer object.
• Or keep Lazy if there's evidence that a meaningful fraction of Context instances never write to console (in which case the deferral saves the StringBuilder allocation). But given the context of console interception tests, this seems unlikely to matter.

This is a minor nit and doesn't affect correctness — just worth considering given the perf-motivation of this PR.

---

Semantic note: Drain() vs FlushIfNonEmpty()

The two draining methods have distinct semantics that are correctly matched to their call sites:

• Drain() → used in WriteLine() — always returns content (even "" for empty buffer), preserving the behavior of Console.WriteLine() which always emits a line.
• FlushIfNonEmpty() → used in Flush() — returns null when empty, avoiding unnecessary sink routing.

Both are consistent with RouteToSinks's null+empty guard. Good.

---

Summary

The object → Lock changes across the codebase are all correct and straightforward. The ConsoleLineBuffer extraction is the most significant architectural change and it's a net improvement. The Lazy<StringBuilder> inside ConsoleLineBuffer is worth reconsidering given the performance focus of this PR, but it's not blocking.

[claude]

Follow-up Review: perf: replace object locks with Lock type for efficient synchronization

Previous Feedback Addressed

My earlier concern about inside has been resolved — the current implementation uses direct initialization:

private readonly StringBuilder _buffer = new();
private readonly Lock _lock = new();

This is the right choice for a perf-focused PR. No wrapper overhead, no volatile reads on hot paths.

---

Overall Assessment

The PR is clean and correct. A few observations on the final state:

AppendAndDrain handles null safely. StringBuilder.Append(null) is a no-op, so passing a null value when the buffer is non-empty produces the buffered content unchanged — correct behavior.

Flush() semantics preserved. FlushIfNonEmpty() returning null when the buffer is empty correctly matches the original if (buffer.Length > 0) guard — RouteToSinks is only called with actual content.

ConstraintKeyScheduler is safe. Lock has no thread affinity, so passing it across async Task boundaries is correct — unlike Monitor, which requires Enter/Exit on the same thread.

Encapsulation is a genuine improvement. Centralizing the lock inside ConsoleLineBuffer removes the possibility of a caller accidentally locking on the wrong object or forgetting to acquire the lock at all. The semantically-named methods (Drain, AppendAndDrain, FlushIfNonEmpty) are also self-documenting at each call site.

---

No issues found. This is ready to merge.

[claude]

Follow-up Review: perf: replace object locks with Lock type for efficient synchronization

Previous Feedback Addressed

My earlier concern about Lazy<StringBuilder> inside ConsoleLineBuffer has been resolved — the current implementation uses direct initialization:

private readonly StringBuilder _buffer = new();
private readonly Lock _lock = new();

This is the right choice for a perf-focused PR. No wrapper overhead, no volatile reads on hot paths.

---

Overall Assessment

The PR is clean and correct. A few observations on the final state:

AppendAndDrain handles null safely. StringBuilder.Append(null) is a no-op, so passing a null value when the buffer is non-empty produces the buffered content unchanged — correct behavior.

Flush() semantics preserved. FlushIfNonEmpty() returning null when the buffer is empty correctly matches the original if (buffer.Length > 0) guard — RouteToSinks is only called with actual content.

ConstraintKeyScheduler is safe. Lock has no thread affinity, so passing it across async Task boundaries is correct — unlike Monitor, which requires Enter/Exit on the same thread.

Encapsulation is a genuine improvement. Centralizing the lock inside ConsoleLineBuffer removes the possibility of a caller accidentally locking on the wrong object or forgetting to acquire the lock at all. The semantically-named methods (Drain, AppendAndDrain, FlushIfNonEmpty) are also self-documenting at each call site.

---

No issues found. This is ready to merge.