perf(engine): skip second object-graph walk in ObjectLifecycleService (#5718)

[thomhurst]

Closes #5718

Summary

• InitializeTrackedObjectsAsync now relies on the sorted TrackedObjects list populated at registration time, removing the per-tracked-object call to DiscoverNestedObjectGraph inside InitializeObjectWithSpanAsync.
• Each tracked object's nested IAsyncInitializer dependencies are already in TrackedObjects at higher depths and are initialized first by the outer depth-descending loop, so no per-object walk is needed.
• The class instance is the only object not in TrackedObjects; it still gets one nested graph walk at the end so non-data-source IAsyncInitializer properties (set in the constructor or by BeforeClass hooks) are still picked up.
• Removes ~half of InitializeObjectWithSpanAsync / TraverseInitializerProperties cost and a chunk of HashSet<string> allocations from the per-test hot path.

Test plan

• dotnet build TUnit.Engine/TUnit.Engine.csproj -c Release -f net10.0
• TUnit.UnitTests (180/180 pass)
• TUnit.TestProject lifecycle / data-source / init slices: PropertyInitializationTests, InitializableTestClassTests, CombinedDataSourceTests (198), AbstractBaseClassPropertyInjectionTests, Bugs.Bug3266 (4-level deep chain ordering, multi-fixture), Bugs._3597, Bugs._1187, Bugs._2955, Bugs._3072, InjectedClassDataSourceWithAsyncInitializerTests, NestedClassDataSourceDrivenTests3, KeyedDataSourceTests, ParallelPropertyInjectionTests, ClassDataSourceSharedNoneRegressionTests, GenericPropertyInjectionTests, PropertySetterTests, PropertyInitTest — all pass.
• Bugs.Bug3266.FourLevelDeepChainShouldInitializeInCorrectOrder (deepest dependency must initialize first across 4 levels) passes — equivalence with prior behavior.

[claude]

Code Review: perf(engine): skip second object-graph walk in ObjectLifecycleService (#5718)

Summary

This PR makes a targeted performance improvement to ObjectLifecycleService.InitializeTrackedObjectsAsync, eliminating a redundant DiscoverNestedObjectGraph call per tracked object by relying on the fact that the depth-sorted TestContext.TrackedObjects list already contains all nested dependencies — discovered and sorted at registration time.

The change is small (12 additions / 4 deletions) but the reasoning requires careful understanding of the invariant being exploited. Overall, this is a sound optimization. A few observations below.

---

What the invariant actually is (and why it's safe)

The key insight is:

1. At registration time, ObjectTracker.TrackObjects → DiscoverAndTrackObjects populates TestContext.TrackedObjects as a depth-sorted SortedList<int, HashSet<object>> covering all data-source-injected objects and their nested IAsyncInitializer dependencies.
2. InitializeTrackedObjectsAsync iterates from deepest to shallowest, so by the time an object at depth n is initialized, all its dependencies at depth n+1... are already initialized.
3. Therefore, calling InitializeNestedObjectsForExecutionAsync (which calls DiscoverNestedObjectGraph on each tracked object) is indeed redundant — it re-walks a graph that is already covered.

The PR correctly preserves the one place where the graph walk is still needed: the test class instance itself, which is not in TrackedObjects. Constructor-assigned or BeforeClass-hook-assigned IAsyncInitializer properties on the class instance are not known at registration time, so a fresh walk is required there.

---

Positive aspects

• Correct reasoning. The PR description precisely articulates why the optimization is safe and what the single remaining exception is. This is non-trivial to get right.
• Documentation quality. The new XML doc comment on InitializeObjectWithSpanAsync clearly states the precondition ("Callers are responsible for ensuring nested dependencies are already initialized") and explains how InitializeTrackedObjectsAsync satisfies it. This is important because the method can now be called on either path (tracked objects OR the class instance), and the caller must understand the difference.
• Comprehensive test plan. The PR description lists a wide range of existing test scenarios verified, including the 4-level deep chain case that would immediately break if the invariant were violated.

---

Concerns and questions

1. Implicit contract on InitializeObjectWithSpanAsync is now fragile

The removal of InitializeNestedObjectsForExecutionAsync from inside InitializeObjectWithSpanAsync makes the method's correctness dependent on call-site ordering. The XML doc states the precondition, but there is nothing in the code to enforce it. If a future caller invokes InitializeObjectWithSpanAsync directly on an object whose nested graph has not been pre-walked, those nested initializers will be silently skipped.

A mitigation worth considering: rename the method to InitializeObjectOnlyWithSpanAsync or similar to make the "no nested walk" contract explicit in the name itself, rather than only in the doc comment. Alternatively, add a #if DEBUG assert that verifies the object's properties are already initialized (via ObjectInitializer.IsInitialized). Either approach would surface the broken precondition at the call site rather than producing a silent lifecycle failure at runtime.

2. The class instance walk still uses DiscoverNestedObjectGraph, which allocates

var classInstance = testContext.Metadata.TestDetails.ClassInstance;
await InitializeNestedObjectsForExecutionAsync(classInstance, cancellationToken);
await InitializeObjectWithSpanAsync(classInstance, testContext, cancellationToken);

InitializeNestedObjectsForExecutionAsync calls DiscoverNestedObjectGraph, which allocates a new Dictionary<int, HashSet<object>> and HashSet<object> on every test. For tests where the class instance has no constructor/hook-assigned IAsyncInitializer properties (the vast majority), this is wasted allocation.

A lightweight guard would help:

// Only do the nested walk if the class has IAsyncInitializer properties not in TrackedObjects.
// For most tests (no such properties), skip the walk entirely.

The ObjectInitializer.IsInitialized check could be used here, or a flag on the test context to indicate whether a nested walk is needed. This is a follow-up opportunity rather than a blocker, since the PR's goal is already met (removing the per-tracked-object walk).

3. InitializeObjectForExecutionAsync still does a nested walk (intentionally)

// ObjectLifecycleService.cs line 173–182
public async Task InitializeObjectForExecutionAsync(object? obj, ...)
{
    await InitializeNestedObjectsForExecutionAsync(obj, cancellationToken);
    await ObjectInitializer.InitializeAsync(obj, cancellationToken);
}

This path is called from TestBuilder.CreateInstance for class constructor arguments, and correctly keeps its nested walk because those objects are not in TrackedObjects. The PR leaves this path untouched, which is the right decision. The asymmetry between this path and the now-changed tracked-objects path is worth a brief comment to make the distinction clear to future readers — the current doc comment says "Used to initialize data source objects before they are passed to the test constructor" which is accurate but doesn't explicitly say "these are NOT in TrackedObjects, hence the walk is needed".

4. ValueListBuilder<Task> in InitializeTrackedObjectsAsync vs List<Task>

The hot path in InitializeTrackedObjectsAsync (lines 250–258) uses List<Task> with a capacity hint (objectsAtLevel.Count). Given that the surrounding code already uses ValueListBuilder<Task> in RegisterArgumentsAsync for similar patterns, it might be consistent to use the same here. This is a minor style/performance nit rather than a design concern, and out of scope for this PR.

---

Verdict

Approved with suggestions. The optimization is correct, well-motivated, and well-tested. The main actionable suggestion is to make the implicit precondition on InitializeObjectWithSpanAsync more visible — either via a naming change or a debug-mode assertion — to protect against future misuse. The remaining points are follow-up opportunities.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[thomhurst]

Closing — the optimization is unsafe in reflection mode.

Root cause discovered while investigating CI failures

The premise (depth-sorted TrackedObjects already covers every nested IAsyncInitializer reference) is wrong in reflection mode because of a latent bug:

1. RegisterTestArgumentsAsync is called twice per test — once from TestBuilder.BuildTestAsync (TestBuilder.cs:942) and again from TestFilterService.RegisterTest (TestFilterService.cs:107).
2. PropertyInjector.InjectReflectionPropertyAsync lacks the existingValue != null → skip guard that InjectSourceGeneratedPropertyAsync (PropertyInjector.cs:257-265) has.
3. The second injection resolves a fresh data-source instance and overwrites the property. The first instance is what got tracked + initialised. The second is what the test method reads — its IAsyncInitializer.InitializeAsync is never called.

Today, ObjectLifecycleService.InitializeObjectWithSpanAsync calls InitializeNestedObjectsForExecutionAsync(obj) on the live graph at execution time — masking the double-inject by re-initialising whatever instance is currently bound. Removing that walk surfaces the latent bug, breaking InheritedDataSourceTests.Test_DirectDataSource_WorksCorrectly and CombinedDataSourceTests.CombinedDataSource_WithNestedPropertyInjectionAndMultipleIAsyncInitializers in reflection mode CI.

Source-gen mode passes because of the existing skip-if-set guard.

Suggested follow-up (separate issue)

Fix the underlying reflection-injection asymmetry first:

• Apply the source-gen "skip if set" guard to InjectReflectionPropertyAsync, AND/OR
• De-dupe the two RegisterTestArgumentsAsync call sites or make the second one idempotent.

Once that lands, the perf improvement attempted here can be re-attempted safely. Worth tracking in a fresh issue rather than holding this PR open.