perf: remove unnecessary loop

[TimothyMakkison]

Removes unnecessary loop to calculate HashSet capacity. I haven't benchmarked this, but I really doubt the savings from preallocating a Hashset outways the cost of allocating a second ConcurrentDictionary.Enumerator and locking and looping the dictionary a second time.

I'll close this PR if this was added to address a legitimate issue

[thomhurst]

Summary

Removes double-pass capacity pre-calculation in FlattenTrackedObjects to avoid second enumeration and locking overhead.

Critical Issues

Incorrect capacity calculation - TUnit.Core/Tracking/ObjectTracker.cs:57

The new code uses trackedObjects.Count as the HashSet capacity, which is the number of dictionary entries (depth levels), not the total number of objects. This will create an undersized HashSet that requires multiple resizes as objects are added.

Example:

• If trackedObjects has 3 entries with 100 objects each
• Old code: Creates HashSet with capacity 300 (correct)
• New code: Creates HashSet with capacity 3 (massively undersized)

This makes the optimization actually WORSE than the original, as HashSet resizes are expensive operations that copy all elements.

Suggested fix: If the goal is to avoid the double-pass, remove the capacity hint entirely.

Modern HashSet implementations handle dynamic growth reasonably well, and this avoids both the double enumeration AND the incorrect capacity.

Verdict

REQUEST CHANGES - Critical bug in capacity calculation

[TimothyMakkison]

The capacity hint is so we at least have enough space for at least one item, I don't see how it could be harmful.

I'll see if I can write a benchmark to see if this PR is necessary. It won't be able to replicate issues with locks tho

[thomhurst]

Summary

This PR attempts to optimize FlattenTrackedObjects by removing the first pass that calculates total capacity.

Critical Issues

Semantic Bug: Incorrect Capacity Calculation

The proposed change uses trackedObjects.Count as the HashSet capacity, but this is semantically incorrect. trackedObjects.Count is the number of dictionary entries (depth levels), NOT the sum of all HashSet sizes. The original totalCapacity calculation summed all HashSet counts across all depth levels.

Example: If trackedObjects has 3 depth levels with 100 objects each:

• Original code: Creates HashSet with capacity 300 (correct)
• Proposed code: Creates HashSet with capacity 3 (will resize approximately 7 times)

This defeats the entire purpose of pre-sizing and likely makes performance worse than the original two-pass approach.

Suggestions

If you want to optimize this method, consider these alternatives:

1. If the pre-sizing optimization is not valuable: Remove the capacity parameter entirely and let HashSet grow naturally without any capacity hint.

2. If pre-sizing matters: Keep the current two-pass approach, or measure with benchmarks whether the lock and iterate overhead truly exceeds the resize cost.

The current implementation comments at ObjectTracker.cs:54 suggest pre-sizing was intentional. If this optimization is questionable, the author should clarify whether the two-pass approach was added to solve a real performance issue.

Verdict

REQUEST CHANGES - The current change introduces a semantic bug that would make performance worse than intended.

[TimothyMakkison]

Rebased and removed the capacity.

[thomhurst]

Summary

This PR removes the pre-calculation loop that sizes the HashSet capacity in FlattenTrackedObjects, eliminating the double-iteration and lock overhead.

Critical Issues

None found ✅

Suggestions

Performance Analysis: The PR author's intuition is correct. The change is a performance improvement because:

1. Double enumeration cost: The original code iterates the ConcurrentDictionary twice - once to calculate capacity, once to populate. Each iteration requires acquiring locks on every HashSet.

2. Lock contention: Holding locks longer during the first pass increases the potential for contention in concurrent scenarios.

3. Diminishing returns: Modern HashSet implementations use power-of-2 sizing with efficient doubling. The cost of a few resizes (typically 1-2 for small collections) is far less than the cost of:

   • A second ConcurrentDictionary.Enumerator allocation
   • Acquiring all locks a second time
   • Iterating all entries again

4. Context: This is called per-test during cleanup (UntrackObjects at TUnit.Engine/Services/TestExecution/TestCoordinator.cs:197) and during tracking setup (TrackObjects). The typical case has few tracked objects per test, where HashSet resizing is negligible.

Minor observation: The #if NETSTANDARD2_0 block removal is correct - since only the modern path had the capacity logic, removing it unifies the code paths.

Verdict

✅ APPROVE - This is a good performance optimization that reduces allocations, lock contention, and unnecessary iteration. The change aligns with TUnit's "Performance First" principle from CLAUDE.md.

[thomhurst]

Thanks for all these improvements btw!

[TimothyMakkison]

Thanks for all these improvements btw!

Thanks, this is kind of thing is super fun 😄