Uh oh!
There was an error while loading. Please reload this page.
perf: skip TCS bridge in ExecuteTestAsync when no ExecutionContext is captured - #9636
Conversation
… captured In the common case (tests whose [ClassInitialize] / [AssemblyInitialize] did not capture an ExecutionContext), ExecutionContextHelpers.RunOnContext simply calls the action inline. The existing code still allocated a TaskCompletionSource<TestResult[]>, an async-lambda closure, and an Action delegate for every test even when unnecessary. Add a fast path that skips all three allocations when the captured context is null. The slow path (TCS bridge via ExecutionContext.Run) is preserved unchanged for the rare case where a lifecycle method captured a context. Fixes#9635 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR optimizes MSTest adapter execution by avoiding unnecessary allocations in TestMethodRunner.ExecuteTestAsync when no ExecutionContext was captured by lifecycle methods, improving the common-case performance of running tests.
Changes:
- Compute the captured
ExecutionContextonce and add a fast path forcapturedContext is null. - Skip the
TaskCompletionSource<TestResult[]>bridge (and associated async-void delegate allocations) on the fast path, while preserving the existing slow path for captured contexts.
Show a summary per file
| File | Description |
|---|---|
| src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodRunner.cs | Adds a null-context fast path to avoid per-test TCS/closure/delegate allocations while preserving the captured-context behavior. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Low
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Note
🤖 Automated review by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.
Review Summary — All Clean ✅
This is a well-crafted, low-risk performance optimization. I applied all 22 review dimensions and found no actionable issues.
Key Verification Points
| Dimension | Verdict | Notes |
|---|---|---|
| 1. Algorithmic Correctness | ✅ | Fast path is semantically equivalent to RunOnContext with null context (which calls action() inline) |
| 2. Threading/Concurrency | ✅ | ExecutionContext properties are set during ClassInitialize/AssemblyInitialize — stable by the time ExecuteTestAsync runs |
| 3. Edge Cases / Null Safety | ✅ | testMethodInfo.Parent (TestClassInfo) and .Parent.Parent (TestAssemblyInfo) are both non-nullable; no null-deref risk |
| 5. Error Handling | ✅ | Exceptions propagate through await to the same outer catch (Exception ex) in both paths — identical behavior |
| 7. Performance | ✅ | Eliminates ~3 allocations (TCS, async-lambda closure, Action delegate) per test in the common case |
| 11. Test Coverage | ✅ | Existing 23 TestMethodRunnerTests exercise this path (most use no captured context); behavioral no-op justifies no new tests |
Notes
- Code duplication between fast and slow paths (
SetCurrentTestContext+TestContextassignment +ExecuteAsync) is intentional and well-commented — it's the mechanism enabling the allocation savings. - The fast path actually improves safety slightly by avoiding the
async voiddelegate pattern (theVSTHRD101suppression) in the common case. - The extracted
capturedContextlocal also eliminates double-evaluation of the??chain, which is a minor but welcome cleanup for the slow path too.
Uh oh!
There was an error while loading. Please reload this page.
Fixes#9635
Summary
ExecuteTestAsyncinTestMethodRunnerused aTaskCompletionSource<TestResult[]>bridge to hop test execution onto a capturedExecutionContext(set by[ClassInitialize]or[AssemblyInitialize]). However,ExecutionContextHelpers.RunOnContextalready handles anullcontext by invoking the action inline — so theTaskCompletionSource, async-lambda closure, andActiondelegate were being allocated for every test, even in the common case where noExecutionContextwas captured.Change
Compute the captured context once and short-circuit the bridge when it is
null:capturedContext is null): execute directly, skipping the TCS bridge and its ~3 heap allocations (~114 bytes) per test.ExecutionContext.Run.Verification
MSTestAdapter.PlatformServicesbuilds clean (0 warnings, 0 errors).TestMethodRunnerTestspass unchanged.Co-authored-by: Copilot App 223556219+Copilot@users.noreply.github.com