-
-
Notifications
You must be signed in to change notification settings - Fork 108
perf: lazy initialization for TestBuilderContext properties #4305
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
Add comprehensive design document for optimizing test discovery through lazy materialization. Key improvements: - Lightweight TestDescriptor struct for fast enumeration - Two-phase discovery: filter first, materialize matching tests only - Pre-computed filter hints at compile time - Deferred data source evaluation This architectural change targets ~20% improvement in single test execution time by avoiding eager materialization of tests that won't run due to filtering. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Reduce allocations in the test building hot path by making three properties in TestBuilderContext lazy-initialized: - DefinitionId: Guid.NewGuid().ToString() only generated on first access - StateBag: ConcurrentDictionary only allocated when accessed - Events: TestContextEvents only allocated when accessed Also removed pre-allocations in TestBuilder.cs (7 locations) that were eagerly creating StateBag and Events instances. This reduces memory pressure during test discovery/building, especially beneficial for large test suites. Co-Authored-By: Claude Opus 4.5 <[email protected]>
SummaryThis PR optimizes test building performance by making TestBuilderContext properties (DefinitionId, StateBag, Events) lazy-initialized to avoid unnecessary allocations. Critical IssuesNone found ✅ SuggestionsConsider thread-safety documentationWhile the implementation is correct (AsyncLocal ensures per-context isolation), consider adding a comment explaining why the non-atomic /// <summary>
/// Gets the unique definition ID for this context. Generated lazily on first access.
/// Thread-safe due to AsyncLocal isolation - each async context has its own instance.
/// </summary>
public string DefinitionId => _definitionId ??= Guid.NewGuid().ToString();Record semantics consideration (informational)The private backing fields don't participate in record equality, but this appears intentional since TestBuilderContext equality comparisons aren't used in the codebase. If record equality becomes important later, be aware that two contexts with the same property values but different lazy initialization states won't compare equal based on the backing fields. Verdict✅ APPROVE - No critical issues The implementation correctly:
The performance improvement is solid - avoiding ConcurrentDictionary allocations and GUID generation for tests that don't use these properties. |
|
Nice, change. Looks like you've saved around Could this use the Edit: I think I made a mistake calculating the memory saved mb |
Summary
TestBuilderContextproperties lazy-initializedTestBuilder.csthat were eagerly creatingStateBagandEventsinstancesChanges
TestBuilderContext.cs
Made three properties lazy-initialized:
DefinitionId= Guid.NewGuid().ToString()(eager)StateBag= new ConcurrentDictionary<>()(eager)Events= new TestContextEvents()(eager)TestBuilder.cs
Removed pre-allocations in 7 locations that were explicitly passing
new ConcurrentDictionary<>()andnew TestContextEvents()when creating contexts.Why This Matters
Before these changes, for every test iteration during building:
new ConcurrentDictionary<string, object?>()- Very expensive due to internal lock arraysnew TestContextEvents()- 7 nullable event delegate slotsGuid.NewGuid().ToString()- GUID generation + string allocationNow these allocations only happen when the properties are actually accessed, which for many tests during the build phase, may not happen at all.
Test plan
TUnit.Engine.Tests- 290 tests pass (270 succeeded, 20 skipped)TUnit.PerformanceBenchmarks- 60 tests pass🤖 Generated with Claude Code