perf: eliminate LINQ allocation in ObjectTracker.UntrackObjectsAsync

[thomhurst]

Summary

• Replace trackedObjects.Keys.OrderByDescending(k => k) with a manual max-find loop + countdown iteration
• Eliminates LINQ enumerator and sorted buffer allocation on every test cleanup call (~1,000 allocations per 1,000 tests)
• Depth keys are small non-negative integers (typically 0-3), so counting down from max with TryGetValue is allocation-free

Test plan

• Builds on all target frameworks (net8.0, net9.0, net10.0)
• DisposableFieldTests and AsyncDisposableFieldTests pass
• CI passes

[claude]

Code Review

The optimization is correct and the allocation elimination reasoning is sound. A few observations:

Correctness ✅

The logic is correct. Finding the max key then counting down with properly handles sparse depth sets and maintains the same descending-depth disposal order as the original LINQ approach.

Assumption vs. Reality: Depth Range

The PR description states depths are "typically 0-3", but . In the worst case, the new loop runs 51 iterations (0–50) even if only a handful of depths are populated. The original LINQ approach allocated but iterated only the actually populated keys.

For typical tests this is a clear win. For deeply nested object graphs with sparse depth distribution, the loop does more iterations than LINQ would (though all iterations are O(1) TryGetValue, so it's still fast).

Better Structural Approach

Rather than working around the unordered Dictionary<int, HashSet<object>>, consider changing TrackedObjects in TestContext.cs from:

internal Dictionary<int, HashSet<object>> TrackedObjects { get; } = new();

to:

internal SortedList<int, HashSet<object>> TrackedObjects { get; } = new();

This would let UntrackObjectsAsync use index-based reverse iteration — zero allocations, zero extra loop overhead, and only iterates actually-populated keys:

var keys = trackedObjects.Keys;
var values = trackedObjects.Values;
for (var i = keys.Count - 1; i >= 0; i--)
{
    var bucket = values[i];
    // ...
}

Why this is better: The sorted order becomes an invariant of the data structure rather than something every caller must re-establish. It also eliminates the max-find loop entirely and avoids iterating missing depth integers. Insertions into SortedList are O(n) but n (number of distinct depths) is always tiny (≤ MaxRecursionDepth = 50), so this cost is negligible.

That said, this is a broader change touching TestContext.cs and potentially ObjectGraphDiscoverer.cs. The current PR's narrower approach is pragmatic and is an improvement over the status quo — I just wanted to flag the cleaner long-term path.

Minor: Comment Accuracy

// Find max depth key to iterate descending without LINQ allocation.

This comment is accurate. No issues.

---

Overall, this is a valid, targeted perf improvement that achieves its stated goal. The approach would be architecturally cleaner but is out of scope for this PR. Approving as-is with the suggestion noted for a future follow-up.

[claude]

Code Review

The optimization is correct and the allocation elimination reasoning is sound. A few observations:

Correctness ✅

The logic is correct. Finding the max key then counting down with TryGetValue properly handles sparse depth sets and maintains the same descending-depth disposal order as the original LINQ approach.

Assumption vs. Reality: Depth Range

The PR description states depths are "typically 0-3", but ObjectGraphDiscoverer.MaxRecursionDepth = 50. In the worst case, the new loop runs 51 iterations (0–50) even if only a handful of depths are populated. The original LINQ approach allocated but iterated only the actually populated keys.

For typical tests this is a clear win. For deeply nested object graphs with sparse depth distribution, the loop does more iterations than LINQ would (though all iterations are O(1) TryGetValue, so it's still fast).

Better Structural Approach

Rather than working around the unordered Dictionary<int, HashSet<object>>, consider changing TrackedObjects in TestContext.cs (line 335) from:

internal Dictionary<int, HashSet<object>> TrackedObjects { get; } = new();

to:

internal SortedList<int, HashSet<object>> TrackedObjects { get; } = new();

This would let UntrackObjectsAsync use index-based reverse iteration — zero allocations, zero extra loop overhead, and only iterates actually-populated keys:

var keys = trackedObjects.Keys;
var values = trackedObjects.Values;
for (var i = keys.Count - 1; i >= 0; i--)
{
    var bucket = values[i];
    // ...
}

Why this is better: The sorted order becomes an invariant of the data structure rather than something every caller must re-establish. It also eliminates the max-find loop entirely and avoids iterating missing depth integers. Insertions into SortedList are O(n) but n (number of distinct depths) is always tiny (≤ MaxRecursionDepth = 50), so this cost is negligible.

That said, this is a broader change touching TestContext.cs and potentially ObjectGraphDiscoverer.cs. The current PR's narrower approach is pragmatic and is an improvement over the status quo — flagging the cleaner long-term path for consideration.

Minor: Comment Accuracy

// Find max depth key to iterate descending without LINQ allocation.

Accurate and helpful. No issues.

---

Overall, this is a valid, targeted perf improvement that achieves its stated goal. The SortedList approach would be architecturally cleaner but is out of scope for this PR.

[claude]

Code Review — Updated Implementation

The previous review suggested adopting SortedList<int, HashSet<object>> as the cleaner structural approach. This PR has fully implemented that suggestion. All previous observations are now resolved.

Previous Comments: Addressed ✅

• Max-find loop concern (sparse depth iteration): Gone — SortedList only stores populated keys, so reverse index iteration touches only real data.
• SortedList suggestion: Implemented throughout — TestContext.TrackedObjects, ObjectGraphDiscoverer, ObjectTracker, ObjectLifecycleService, TrackableObjectGraphProvider, and the interface IObjectGraphDiscoverer.

---

Review of Current Implementation

Correctness ✅

SortedList<int, HashSet<object>> maintains ascending key order by definition. The reverse index-based for loop correctly yields descending depth order for disposal — same semantics as the original OrderByDescending, zero allocations.

ObjectLifecycleService simplification ✅

The removal of the int[] levels + Array.Sort workaround is a clean improvement. The if (levelCount > 0) guard was also correctly dropped — the for loop naturally handles empty collections.

IDictionary in private helpers — Minor note

DiscoverNestedObjectsForTracking and TryAddToHashSet take IDictionary<int, HashSet<object>>. Since these are private methods always called with the concrete SortedList, using the interface here is a reasonable choice (avoids leaking the concrete type deeper). No issue.

SortedList insertion cost

O(n) insertions vs. O(1) amortized for Dictionary. With n ≤ MaxRecursionDepth (50, typically 1–3), this is completely negligible and not worth worrying about.

TrackableObjectGraphProvider.cs

Return type updated here too — correctly keeping all call sites consistent. Good.

---

Summary

This is a well-executed performance improvement. Sorted order is now a structural invariant of TrackedObjects rather than a runtime concern every caller must re-establish. The hot-path iteration in both ObjectTracker and ObjectLifecycleService is now allocation-free and iterates only populated depth keys. The implementation is clean, correct, and simpler than what it replaces.

No blocking issues.