perf: cache TestNode properties to reduce allocations

[thomhurst]

Summary

• Cache AssemblyName.FullName per assembly using ConcurrentDictionary to avoid repeated GetName() allocations
• Cache static TestNode properties per test ID that never change between state transitions (discovered → in-progress → passed/failed)
• Use StringBuilderPool in GetClassTypeName instead of allocating new StringBuilder

Problem

ToTestNode is called 3+ times per test (discovered, in-progress, passed/failed). Before this change, it was creating dozens of objects per call including:

• Assembly.GetName().FullName which allocates a new AssemblyName each time
• TestFileLocationProperty
• TestMethodIdentifierProperty
• TestMetadataProperty[] for categories and custom properties
• TrxCategoriesProperty

Solution

Properties that never change between test state transitions are now cached on first access:

• TestFileLocationProperty
• TestMethodIdentifierProperty
• Category and custom property arrays
• TRX-related properties

Results

Profiling with dotnet-trace shows:

Metric	Before	After	Improvement
TestExtensions.ToTestNode	3.06% exclusive	Not in top 50	~3% reduction
AsyncMethodBuilderCore.Start	4.13% exclusive	3.28% exclusive	~20% reduction
Monitor.Enter_Slowpath	1.19% exclusive	1.00% exclusive	~16% reduction

For a 1000 test suite, this eliminates approximately 6000+ object allocations per run.

Test plan

• Build succeeds for all target frameworks
• Profile shows improved performance
• Existing tests pass

🤖 Generated with Claude Code

[thomhurst]

Summary

Performance optimization that caches TestNode properties and assembly names to reduce allocations in the hot path.

Critical Issues

1. Thread Safety Concern with Cache Invalidation ⚠️

The TestNodePropertiesCache is a ConcurrentDictionary<string, CachedTestNodeProperties> keyed by test ID (testDetails.TestId). This cache grows unbounded and never gets cleared.

Problem: In scenarios with dynamic test discovery or long-running test sessions (e.g., watch mode, test servers), this cache will grow indefinitely, causing memory leaks.

Location: TUnit.Engine/Extensions/TestExtensions.cs:26

Impact: Memory leak in long-running processes.

Solution: Consider one of:

1. Use ConditionalWeakTable<TestDetails, CachedTestNodeProperties> if test details are stable objects
2. Add cache eviction policy (LRU with size limit)
3. Add a method to clear cache between test runs
4. Document that this is intentional for typical short-lived test sessions

2. Missing using for StringBuilderPool Import 🐛

The change in TestContextExtensions.cs references StringBuilderPool but doesn't verify the namespace is imported.

Location: TUnit.Core/Extensions/TestContextExtensions.cs:20

Looking at line 2, I see using TUnit.Core.Helpers; is present in the file, so this is fine. ✅

Suggestions

1. AssemblyFullNameCache also unbounded

Similar to TestNodePropertiesCache, the AssemblyFullNameCache (TUnit.Engine/Extensions/TestExtensions.cs:21) is unbounded. However, assemblies are typically few and stable, so this is likely acceptable. Consider documenting this assumption.

2. Consider ArrayPool<T> for property arrays

The cached TestMetadataProperty[] arrays in CachedTestNodeProperties could use ArrayPool<TestMetadataProperty>.Shared for even better performance, though the current approach is already a significant improvement.

3. Whitespace-only changes

Lines 93-103, 107, 118-120 in TestExtensions.cs have formatting changes (if( → if (). These are fine but note they're style changes not functional changes.

Previous Review Status

Unable to fetch previous comments due to API scope limitations.

Verdict

⚠️ REQUEST CHANGES - Critical memory leak issue (#1) must be addressed before merge.

[thomhurst]

Summary

This PR caches static TestNode properties and assembly names to eliminate ~6000+ allocations per 1000-test run, reducing ToTestNode from 3% of execution time to not appearing in the top 50 hot paths.

Critical Issues

None found ✅

Suggestions

1. Thread-Safety Documentation for Caching Strategy

The PR introduces two static ConcurrentDictionary caches (AssemblyFullNameCache and TestNodePropertiesCache) that are shared across all test executions. While ConcurrentDictionary is thread-safe, consider documenting:

• Lifecycle: These caches grow indefinitely during the test session. For large test suites with many assemblies or dynamic test generation, this could lead to memory accumulation. Is this acceptable for TUnits usage patterns?
• Cache Invalidation: The caches never clear. If tests can be re-run within the same process (e.g., watch mode, or multiple test sessions), stale data might be served. Is this a concern?

Location: TUnit.Engine/Extensions/TestExtensions.cs:19-20

2. Potential Over-Caching of Test IDs

The TestNodePropertiesCache keys on testDetails.TestId. If test IDs are globally unique across all test runs, this is fine. However, if dynamic tests can have colliding IDs across different test sessions or test IDs can be reused in watch mode, the cache might return stale data.

Question: Are test IDs guaranteed to be unique within the lifetime of these static caches?

3. StringBuilder Pool Usage

The change correctly uses StringBuilderPool.Get() and returns it in a finally block. This is a good performance improvement. ✅

Performance Validation

The PR claims significant improvements based on profiling:

• ToTestNode dropped from 3.06% to not in top 50 ✅
• AsyncMethodBuilderCore.Start reduced by ~20% ✅
• ~6000 allocations eliminated per 1000 tests ✅

Recommendation: Run the full test suite to verify no regressions, especially in concurrent execution scenarios.

Verdict

✅ APPROVE - No critical issues. The performance improvements are substantial and the implementation is sound. Suggestions above are optional enhancements for documentation and edge cases.

[thomhurst]

Summary

Caches static TestNode properties (file location, method identifier, categories, TRX metadata) to eliminate 6000+ allocations per 1000-test run.

Critical Issues

None found ✅

Suggestions

1. Consider memory growth in long-running scenarios

The TestNodePropertiesCache and AssemblyFullNameCache grow unbounded during a test session. For most test runs this is fine, but in scenarios with dynamic test generation or very long-running test sessions, this could accumulate memory.

Context: The caches are cleared in TUnitServiceProvider.DisposeAsync(), which is good. However, if tests are dynamically generated during execution (via TestContext.AddDynamicTest or CreateTestVariant), the cache could grow significantly.

Recommendation: Consider adding metrics or documentation about cache behavior, or explore size-bounded caching if dynamic test generation is common. Not a blocker since DisposeAsync cleanup handles typical scenarios.

2. StringBuilderPool usage is excellent

The migration from manual StringBuilder to StringBuilderPool in TestContextExtensions.GetClassTypeName (TUnit.Core/Extensions/TestContextExtensions.cs:20-25) is a great improvement. The try/finally pattern ensures proper cleanup even if ArgumentFormatter.Format throws.

3. Cache key uniqueness assumption

The code assumes TestDetails.TestId is unique across all tests. This appears correct based on the codebase, but worth verifying that test variants (created via CreateTestVariant) get unique IDs to avoid cache collisions.

Performance Notes

The profiling data shows impressive results:

• ToTestNode dropped from 3.06% exclusive time to below top 50
• AsyncMethodBuilderCore.Start reduced 20%
• Monitor.Enter_Slowpath reduced 16%

This aligns well with TUnit's Performance First principle from CLAUDE.md.

Verdict

✅ APPROVE - Solid performance optimization with proper thread-safety and cleanup. Suggestions are optional improvements for edge cases.