perf: eliminate async state machine in TestCoordinator.ExecuteTestAsync

[thomhurst]

Summary

• Convert TestCoordinator.ExecuteTestAsync from async ValueTask to non-async ValueTask
• On the synchronous completion fast path (common case), avoids allocating an async state machine entirely
• Async slow path handled by a static local function to prevent closure captures

Context

Profiling shows AsyncMethodBuilderCore.Start at 3.44% and AwaitUnsafeOnCompleted at 2.51% exclusive CPU time (~6% total). The test execution call chain is deeply nested with async wrappers. This eliminates one state machine per test execution on the fast path.

Other methods in the chain (TestRunner, TestExecutionGuard) have try/catch/finally blocks that require async and cannot be flattened.

Test plan

• Builds successfully (0 errors)
• BasicTests pass
• CI passes

[claude]

Code Review

Summary

The optimization goal is sound — removing the async state machine overhead from — but there's a subtle correctness issue with the "fast path" claim, and a cleaner approach is available.

---

Issue: The Fast Path () Never Triggers

Looking at , both execution paths always return an incomplete task:

1. Duplicate-execution path → — wraps a , always pending
2. Normal execution path → — an async ValueTask<bool> that awaits executionFunc(), which is always async

The IsCompletedSuccessfully check on line 57 will never be true in practice. The "fast path" is dead code, and the actual improvement is only the slightly smaller state machine allocation in AwaitAndDiscard (which only captures t vs. the original capturing test, cancellationToken, and this).

This isn't wrong — it's a real, albeit minor, allocation reduction — but the PR description overstates the impact with "synchronous completion fast path (common case)".

---

Better Approach: Fix the Root Cause in

The real opportunity here is that the bool returned by TryStartExecutionAsync is never used — it's discarded in both the old and new code. Changing its return type to ValueTask would:

1. Eliminate the type mismatch entirely (no wrapper needed)
2. Allow ExecuteTestAsync to directly forward the task — zero state machine, zero wrapper
3. Make the API cleaner (no misleading bool that callers must ignore)

Then becomes a pure one-liner with no state machine at all:

This is a strictly better outcome: eliminates the state machine entirely rather than replacing it with a smaller one, and removes a meaningless bool from the public API surface of TestExecutionGuard.

---

Minor: Exception Handling Correctness

The current PR is correct on exception handling: if the task faults synchronously (edge case), IsCompletedSuccessfully is false, so it falls through to AwaitAndDiscard which re-throws properly. No issue here.

---

Verdict

The change is safe and provides a small allocation reduction, but the "fast path" framing is misleading and the approach misses a simpler, more impactful refactor. Suggest revising TestExecutionGuard to return ValueTask instead of ValueTask<bool> to achieve the full optimization without the conditional complexity.

[claude]

Code Review

Summary

The optimization goal is sound — removing the async state machine overhead from ExecuteTestAsync — but there's a subtle correctness issue with the "fast path" claim, and a cleaner approach is available.

---

Issue: The Fast Path (IsCompletedSuccessfully) Never Triggers

Looking at TestExecutionGuard.TryStartExecutionAsync, both execution paths always return an incomplete task:

1. Duplicate-execution path → new ValueTask<bool>(WaitForExistingExecutionAsync(existingTcs)) — wraps a Task, always pending
2. Normal execution path → ExecuteAndCompleteAsync(...) — an async method that awaits executionFunc(), which is always async

The IsCompletedSuccessfully check will never be true in practice. The "fast path" is dead code, and the actual improvement is only the slightly smaller state machine allocation in AwaitAndDiscard (which captures only t vs. the original capturing test, cancellationToken, and this).

This isn't wrong — it's a real, albeit minor, allocation reduction — but the PR description overstates the impact with "synchronous completion fast path (common case)".

---

Better Approach: Fix the Root Cause in TestExecutionGuard

The real opportunity here is that the bool returned by TryStartExecutionAsync is never used — it's discarded in both the old and new code. Changing its return type to ValueTask would:

1. Eliminate the type mismatch entirely (no wrapper needed)
2. Allow ExecuteTestAsync to directly forward the task — zero state machine, zero wrapper
3. Make the API cleaner (no misleading bool that callers must ignore)

// TestExecutionGuard.cs
public ValueTask TryStartExecutionAsync(string testId, Func<ValueTask> executionFunc)
{
    if (_executingTests.TryGetValue(testId, out var existingTcs))
        return new ValueTask(WaitForExistingExecutionAsync(existingTcs));

    var tcs = new TaskCompletionSource<bool>();
    existingTcs = _executingTests.GetOrAdd(testId, tcs);

    if (existingTcs != tcs)
        return new ValueTask(WaitForExistingExecutionAsync(existingTcs));

    return ExecuteAndCompleteAsync(testId, tcs, executionFunc);
}

private async ValueTask ExecuteAndCompleteAsync(string testId, TaskCompletionSource<bool> tcs, Func<ValueTask> executionFunc)
{
    try
    {
        await executionFunc().ConfigureAwait(false);
        tcs.SetResult(true);
    }
    catch (Exception ex)
    {
        tcs.SetException(ex);
        throw;
    }
    finally
    {
        _executingTests.TryRemove(testId, out _);
    }
}

Then ExecuteTestAsync becomes a pure one-liner with no state machine at all:

public ValueTask ExecuteTestAsync(AbstractExecutableTest test, CancellationToken cancellationToken)
{
    return _executionGuard.TryStartExecutionAsync(test.TestId,
        () => ExecuteTestInternalAsync(test, cancellationToken));
}

This is a strictly better outcome: eliminates the state machine entirely rather than replacing it with a smaller one, and removes a meaningless bool from the public API surface of TestExecutionGuard.

---

Minor: Exception Handling Correctness

The current PR is correct on exception handling: if the task faults synchronously, IsCompletedSuccessfully is false, so it falls through to AwaitAndDiscard which re-throws properly. No issue here.

---

Verdict

The change is safe and provides a small allocation reduction, but the "fast path" framing is misleading and the approach misses a simpler, more impactful refactor. Suggest revising TestExecutionGuard to return ValueTask instead of ValueTask<bool> to achieve the full optimization without the conditional complexity.

[claude]

Follow-up Review

The previous review suggestions have been fully addressed in this PR. Here's what was done and my assessment:

Previous Feedback: Addressed ✅

The earlier review flagged that TryStartExecutionAsync returned a meaningless ValueTask<bool> that callers always discarded, and suggested changing it to ValueTask. This PR has implemented exactly that:

• TestExecutionGuard.TryStartExecutionAsync: ValueTask<bool> → ValueTask
• WaitForExistingExecutionAsync: Task<bool> → Task (removed the dead return false)
• ExecuteAndCompleteAsync: ValueTask<bool> → ValueTask (removed the dead return true)
• ExecuteTestAsync: Now a pure expression-bodied forwarding call — zero state machine

Correctness Analysis

Exception propagation is correct: When executionFunc() throws, ExecuteAndCompleteAsync calls tcs.SetException(ex) before re-throwing. Any concurrent waiters in WaitForExistingExecutionAsync will receive the exception from tcs.Task, which is the right behavior (if a test execution fails, all waiters should observe the failure).

Duplicate-execution slow path: new ValueTask(WaitForExistingExecutionAsync(existingTcs)) wraps a Task in a ValueTask, which does allocate — but this is only the contention path (same test running concurrently), which is presumably rare. Acceptable tradeoff.

Fast path in TryStartExecutionAsync: The initial TryGetValue check before GetOrAdd is a valid optimization — avoids TaskCompletionSource allocation on the common case where the same test is already running. Clean.

Overall

This is now a clean, minimal change that achieves the stated goal: zero state machine allocation in ExecuteTestAsync on every call, with no misleading bool return cluttering the API. The code is correct, readable, and aligns well with TUnit's performance-first principle.

Approved.

[claude]

Approved — previous feedback addressed, code is correct and clean.