perf: TestExtensions eliminate closure with equality

[TimothyMakkison]

Each time GetOrCreateCachedProperties is called a Func<string, CachedTestNodeProperties> and display class would be created for GetOrAdd function.

I changed TestNodePropertiesCache to ConcurrentDictionary<TestContext, CachedTestNodeProperties> and used a custom IEqualityComparer to use TestId as the actual key. This way the testContext would be passed to the valueFactory and no closure would be created.

Before

After

This trick can probably be used in a lot of other places

[thomhurst]

Summary

Eliminates closure allocations in TestExtensions.GetOrCreateCachedProperties by using a custom equality comparer and static factory method.

Critical Issues

None found ✅

Suggestions

Memory Retention Consideration

The PR changes the cache key from string (TestId) to TestContext (reference type). While the custom comparer correctly uses TestId for equality, this means the cache now holds references to TestContext objects instead of lightweight strings.

Potential impact:

• TestContext objects cannot be GC'd until ClearCaches() is called
• Each TestContext likely has references to test instances, metadata, output buffers, etc.
• For large test suites, this could increase memory usage during test runs

Questions:

1. Have you measured memory impact for this change in addition to allocation reduction?
2. Does ClearCaches() get called frequently enough that memory retention isn't a concern?

Looking at TUnitServiceProvider.cs:310, ClearCaches() appears to be called during cleanup, which should be fine. But it would be good to verify this doesn't cause issues for very large test suites.

Alternative Approach (Optional)

If memory retention becomes an issue, you could achieve the same allocation elimination while keeping strings as keys:

private static CachedTestNodeProperties GetOrCreateCachedProperties(TestContext testContext)
{
    var testId = testContext.TestDetails.TestId;
    return TestNodePropertiesCache.GetOrAdd(
        testId, 
        static (_, context) => CreateProperties(context),
        testContext  // passed as state parameter
    );
}

private static CachedTestNodeProperties CreateProperties(TestContext testContext)
{
    // ... existing implementation
}

This uses the GetOrAdd overload that accepts a state parameter, avoiding closures while keeping string keys.

Verdict

✅ APPROVE - The optimization is sound and correctly eliminates allocations. The memory retention concern is minor and likely acceptable given the cache is cleared during cleanup. The performance improvement shown in the screenshots is valuable.

[TimothyMakkison]

TestContext objects cannot be GC'd until ClearCaches() is called

Damn, hadn't thought of that good catch.

I don't think ClearCaches is run until the program ends? So this would be an issue.

GetOrAdd(TKey, Func<TKey,TArg,TValue>, TArg)

Didn't think this was available for .NET standard 2.0 but it looks like the polyfill has added it, I'll update my PR

[TimothyMakkison]

Updated this PR

Just noticed that my original changes got the TestId incorrectly in the comparer.

[thomhurst]

Summary

This PR eliminates a closure allocation in the hot path of test node property caching by using the GetOrAdd overload that accepts a state parameter.

Critical Issues

None found ✅

Suggestions

None - this is a clean performance optimization that:

• Uses the static lambda to prevent closure capture
• Passes testContext as state via the GetOrAdd overload
• Maintains identical behavior while eliminating heap allocations in the hot path
• Aligns with TUnit's "Performance First" principle

The PR description mentions a custom IEqualityComparer approach, but the actual implementation takes a simpler and equally effective approach using the TArg overload of GetOrAdd, which is the right choice for this case.

Verdict

✅ APPROVE - No critical issues. Clean performance improvement in a hot path.