-
-
Notifications
You must be signed in to change notification settings - Fork 108
perf: defer allocations in event receivers and hook executors #4306
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Defer List<object> allocation in RegisterReceivers until needed - Use single TryAdd operation instead of Contains + Add pattern - Defer List<Exception> allocations in after-hook methods until exception occurs - Return empty collection literal [] instead of pre-allocated list when no items These changes reduce GC pressure by avoiding unnecessary allocations in the common case where no exceptions occur and objects are already registered. Co-Authored-By: Claude Opus 4.5 <[email protected]>
SummaryPerformance optimization that defers allocations in event receivers and hook executors until actually needed. Critical IssuesNone found ✅ SuggestionsNone - the changes are well-implemented and follow TUnit's performance-first principles. AnalysisThis PR optimizes hot paths in the TUnit engine by applying lazy allocation patterns: EventReceiverOrchestrator.cs (TUnit.Engine/Services/EventReceiverOrchestrator.cs:41-82)
Both files - Exception collection pattern (TUnit.Engine/Services/EventReceiverOrchestrator.cs:148-196, TUnit.Engine/Services/HookExecutor.cs:65-330)
TUnit Rules Compliance
The changes are well-justified by CPU profiling data (0.36% exclusive time in RegisterReceivers) and target the common case where hooks execute successfully without exceptions. Verdict✅ APPROVE - No critical issues. Solid performance optimization following TUnit's engineering principles. |
| { | ||
| var exceptions = new List<Exception>(); | ||
| // Defer exception list allocation until actually needed | ||
| List<Exception>? exceptions = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to move this down after receivers == null check as well. This was done everywhere else
Summary
List<object>allocation inRegisterReceiversuntil actually neededTryAddoperation instead ofContains + Addpattern (reduces dictionary lookups)List<Exception>allocations in after-hook methods until an exception actually occurs[]instead of pre-allocated list when no itemsRationale
CPU profiling with
dotnet traceidentifiedEventReceiverOrchestrator.RegisterReceiversas taking 0.36% exclusive time. The changes reduce GC pressure by avoiding unnecessary allocations in the common case where:Test plan
🤖 Generated with Claude Code