More perf tweaks

[thomhurst]

No description provided.

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review Complete

I've thoroughly reviewed this performance optimization PR. Here's my comprehensive analysis:

Todo List:

• Read repository guidelines and development rules
• Analyze the 16 changed files for performance improvements
• Check for potential bugs and issues
• Evaluate security considerations
• Assess test coverage implications
• Provide comprehensive review feedback

🎯 Overall Assessment

This PR introduces solid performance optimizations focused on caching and memory allocation reduction. The changes align well with TUnit's "performance first" philosophy and demonstrate good understanding of .NET performance patterns.

✅ Code Quality & Best Practices

Excellent Improvements:

1. New InterfaceCache Class (TUnit.Core.SourceGenerator/Helpers/InterfaceCache.cs)

   • ✅ Smart caching strategy: Uses ConcurrentDictionary to cache interface implementation checks
   • ✅ Thread-safe design: Proper use of concurrent collections for multi-threaded scenarios
   • ✅ Reduced AllInterfaces traversals: Major performance win for source generator scenarios
   • ✅ Clean API: Well-documented methods with clear purposes
   • ✅ Custom comparer: TypeStringTupleComparer properly implements symbol equality

2. TestGroupingService Optimizations (TUnit.Engine/Services/TestGroupingService.cs:17-23)

   • ✅ Struct-based sorting: TestSortKey struct reduces allocations during test sorting
   • ✅ Cached string values: Pre-computing ClassFullName avoids repeated property access
   • ✅ Pre-sized collections: Smart capacity hints for dictionaries

3. Memory Optimizations in Core Models:

   • ✅ Cached collections: AssemblyHookContext.cs:30,40 and TestSessionContext.cs:61-62,72,74 cache expensive LINQ operations
   • ✅ Cache invalidation: Proper cache invalidation when collections change

Minor Code Style Notes:

• Collection expressions: Good use of modern C# [] syntax (follows CLAUDE.md guidelines)
• Naming conventions: Consistent with established patterns

🔍 Potential Issues & Concerns

1. Memory Growth Risk (Medium Priority)

Location: TUnit.Core.SourceGenerator/Helpers/InterfaceCache.cs:12-13

private static readonly ConcurrentDictionary<(ITypeSymbol Type, string InterfaceName), bool> _implementsCache = new(TypeStringTupleComparer.Default);
private static readonly ConcurrentDictionary<(ITypeSymbol Type, string GenericInterfacePattern), INamedTypeSymbol?> _genericInterfaceCache = new(TypeStringTupleComparer.Default);

• ⚠️ Static caches never clear: In long-running scenarios or with many types, these caches could grow indefinitely
• Recommendation: Consider adding a cache eviction policy or size limits

2. Potential Race Condition (Low Priority)

Location: TUnit.Engine/Discovery/ReflectionTestDataCollector.cs:33-39

private static Assembly[] GetCachedAssemblies()
{
    lock (_assemblyCacheLock)
    {
        return _cachedAssemblies ??= AppDomain.CurrentDomain.GetAssemblies();
    }
}

• ⚠️ Assembly loading timing: AppDomain.CurrentDomain.GetAssemblies() returns assemblies loaded at that moment
• Risk: New assemblies loaded after caching won't be discovered
• Recommendation: Consider cache invalidation when assembly load events occur

3. Cache Invalidation Timing

Location: TUnit.Core/Models/TestSessionContext.cs:76-80

• ✅ Good: Cache invalidation exists
• ⚠️ Concern: Multiple invalidations during batch operations could cause unnecessary recomputation
• Suggestion: Consider batch invalidation for bulk operations

🔒 Security Considerations

• ✅ No security concerns identified: Changes are internal optimizations without external attack surface
• ✅ Proper AOT handling: Reflection code properly marked with suppression attributes
• ✅ Thread safety: Concurrent collections used appropriately

🧪 Test Coverage Assessment

Areas Needing Attention:

1. InterfaceCache testing: The new caching system should have unit tests covering:

   • Cache hit/miss scenarios
   • Thread safety under concurrent access
   • Memory behavior with large type sets

2. Dual-mode verification: Per CLAUDE.md requirements, changes affect source generator behavior - ensure identical behavior in reflection mode

Recommended Testing:

# Test source generator with new caching
dotnet test TUnit.Core.SourceGenerator.Tests

# Verify no public API changes
dotnet test TUnit.PublicAPI

# Performance regression testing
# Consider benchmarking interface detection before/after changes

⚡ Performance Analysis

Expected Improvements:

1. Source Generator: Major reduction in AllInterfaces traversals (potentially 50-90% reduction in interface checks)
2. Test Discovery: Reduced allocations in reflection mode through better caching
3. Test Grouping: Struct-based sorting reduces GC pressure during test organization

Performance Risks:

• Memory usage: Static caches will increase baseline memory consumption
• Cold start: Initial cache population might be slightly slower

📋 Recommendations

High Priority:

1. Add InterfaceCache tests - Critical for such a core optimization
2. Consider cache size limits - Prevent unbounded memory growth
3. Benchmark the changes - Verify expected performance improvements

Medium Priority:

1. Assembly discovery refresh - Handle dynamically loaded assemblies
2. Batch cache invalidation - Optimize cache invalidation patterns

Low Priority:

1. Monitor memory usage - In production scenarios with many types
2. Consider weak references - For very long-running scenarios

✅ Compliance Check

• ✅ Dual-mode compatibility: Changes appear to maintain parity between source-gen and reflection modes
• ✅ Modern C# usage: Proper collection expressions and patterns
• ✅ Thread safety: Appropriate use of concurrent collections
• ⚠️ Snapshot tests: May need updating if source generator output changes

🎯 Final Verdict

Approval Recommended with suggested testing additions. This is a solid performance optimization that demonstrates good understanding of .NET performance patterns and TUnit's architecture. The caching strategies are well-implemented and should provide meaningful performance improvements.

Key Action Items:

1. Add unit tests for InterfaceCache
2. Run snapshot tests: dotnet test TUnit.Core.SourceGenerator.Tests
3. Verify no API surface changes: dotnet test TUnit.PublicAPI

---