perf: remove unnecessary lock in ObjectTracker.TrackObjects

[thomhurst]

Summary

• Remove lock (kvp.Value) in ObjectTracker.TrackObjects() that caused Monitor.Enter_Slowpath contention (~1.25% exclusive CPU during parallel execution)
• The lock was unnecessary: TrackedObjects is per-TestContext and TrackObjects is called from a single thread per test
• The shared state (s_trackedObjects ConcurrentDictionary) is already thread-safe via GetOrAdd
• Also removed a stale comment in UntrackObject that referenced a non-existent lock

Test plan

• Builds successfully (TUnit.Core and TUnit.Engine, Release, net10.0)
• DisposableFieldTests and DisposablePropertyTests pass
• CI passes

[claude]

Code Review

Overall: Clean and correct performance improvement. The analysis is sound and the change is safe.

Summary of Changes

1. **Removes in ** — the lock was guarding iteration of a that comes from .
2. Removes a stale comment in referencing a lock that no longer existed.

---

Analysis: Was the lock actually necessary?

The PR's reasoning holds up under scrutiny:

• delegates to , which constructs a fresh on every call — the kvp.Value HashSets are not shared references.
• TrackedObjects on TestContext is per-context, not shared across tests.
• The ObjectGraphDiscoverer class itself is documented as thread-safe (static ConcurrentBag, cached reflection), but the returned dictionary is local to each call.

So yes, the lock was guarding a locally-constructed, call-owned collection — providing no real safety benefit while introducing Monitor.Enter_Slowpath overhead under parallel test execution.

---

Minor Consideration

The new comment says:

// No lock needed: TrackedObjects is per-TestContext and TrackObjects is called from a single thread per test.

The "called from a single thread per test" part is an architectural contract that isn't enforced at compile time. If this invariant ever breaks (e.g., a future parallel data-provider feature), it would be a silent regression. Consider whether this guarantee should be documented more formally at the call site in the engine, or whether a Debug.Assert or thread-affinity check could serve as a guardrail. That said, this is a pre-existing condition — not introduced by this PR — and is low risk given the current architecture.

---

Stale Comment Removal

Correct. The removed comment ("Dispose outside the lock to avoid blocking other untrack operations") referenced a lock in UntrackObject that no longer exists. The cleanup is appropriate.

---

No blocking issues. The change correctly identifies and removes unnecessary synchronization overhead.

[claude]

Code Review

Overall: Clean and correct performance improvement. The analysis is sound and the change is safe.

Summary of Changes

1. Removes lock (kvp.Value) in TrackObjects — the lock was guarding iteration of a HashSet<object> that comes from trackableObjectGraphProvider.GetTrackableObjects(testContext).
2. Removes a stale comment in UntrackObject referencing a lock that no longer existed.

---

Analysis: Was the lock actually necessary?

The PR's reasoning holds up under scrutiny:

• GetTrackableObjects delegates to ObjectGraphDiscoverer.DiscoverAndTrackObjects, which constructs a freshDictionary<int, HashSet<object>> on every call — the kvp.Value HashSets are not shared references.
• TrackedObjects on TestContext is per-context, not shared across tests.
• The ObjectGraphDiscoverer class itself is documented as thread-safe (static ConcurrentBag, cached reflection), but the returned dictionary is local to each call.

So yes, the lock was guarding a locally-constructed, call-owned collection — providing no real safety benefit while introducing Monitor.Enter_Slowpath overhead under parallel test execution.

---

Minor Consideration

The new comment says:

// No lock needed: TrackedObjects is per-TestContext and TrackObjects is called from a single thread per test.

The "called from a single thread per test" part is an architectural contract that isn't enforced at compile time. If this invariant ever breaks (e.g., a future parallel data-provider feature), it would be a silent regression. Consider whether this guarantee should be documented more formally at the call site in the engine, or whether a Debug.Assert or thread-affinity check could serve as a guardrail. That said, this is a pre-existing condition — not introduced by this PR — and is low risk given the current architecture.

---

Stale Comment Removal

Correct. The removed comment ("Dispose outside the lock to avoid blocking other untrack operations") referenced a lock in UntrackObject that no longer exists. The cleanup is appropriate.

---

No blocking issues. The change correctly identifies and removes unnecessary synchronization overhead.