perf: reduce allocations in data-driven test execution hot path#9617
perf: reduce allocations in data-driven test execution hot path#9617Evangelink wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR groups four allocation micro-optimizations (#9602, #9603, #9605, #9593) that all target the same MSTest adapter data-driven test-execution hot path. For a test with N data rows, several redundant per-row allocations collapse from O(N) to O(1) with no intended behavioral change. It builds directly on the caching approach introduced in #9514.
Changes:
- Skip the intermediate
snapshotDictionary inCloneForDataDrivenIterationby passing_propertiesdirectly to the constructor (which already deep-copies in the null/null branch). - Add a null-
ExecutionContextfast path inExecuteTestAsyncthat awaits the executor directly, avoiding theTaskCompletionSource+ closure + delegate bridge when no context was captured. - Cache
ParameterInfo[]inReflectionTestMethodInfo.GetParameters(), reuse a singleReflectionTestMethodInfowrapper across rows, and switchResolveArguments()to the already-cachedParameterTypes.
Show a summary per file
| File | Description |
|---|---|
src/TestFramework/TestFramework/Internal/ReflectionTestMethodInfo.cs |
Adds a _parameters field and caches GetParameters() to avoid a fresh array per call. |
src/Adapter/MSTestAdapter.PlatformServices/Services/TestContextImplementation.cs |
Passes _properties directly to the constructor in CloneForDataDrivenIteration, removing the intermediate snapshot Dictionary. |
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodRunner.cs |
Adds the null-ExecutionContext fast path in ExecuteTestAsync and reuses a cached ReflectionTestMethodInfo across data rows. |
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodInfo.ArgumentResolution.cs |
Uses the cached ParameterTypes property instead of calling MethodInfo.GetParameters() directly. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Medium
|
/backport to rel/4.3 |
|
Started backporting to rel/4.3: https://github.com/microsoft/testfx/actions/runs/28736320032 |
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.
✅ 20/22 dimensions clean (Analyzer Quality and IPC Wire Compatibility skipped as N/A).
| # | Dimension | Verdict |
|---|---|---|
| 5 | Performance & Allocations | 🟢 NIT — cached array exposed without clone |
Single nit (see inline comment on ReflectionTestMethodInfo.GetParameters):
- The cached
ParameterInfo[]is returned directly to user-providedITestDataSourceimplementations without cloning. Unlikely to cause issues in practice, but differs from the defensive clone pattern used inTestMethodInfo.ParameterTypes. Consider either cloning or documenting the contract.
Summary of review findings:
- CloneForDataDrivenIteration (#9602): Correct. Eliminating the intermediate
snapshotDictionary is safe because the constructor's[with(properties)]branch already creates an isolated copy. No new race window introduced (the lock-free read of_propertieswas equally present in the old code). - ExecuteTestAsync fast path (#9603): Correct. The
capturedContext is nullguard faithfully reproduces the semantics ofRunOnContext(null, action)(which just callsaction()directly). Exception propagation,using-block disposal order, andConfigureAwait(false)are all preserved. - ReflectionTestMethodInfo.GetParameters() cache (#9605, #9593): Correct and effective.
MakeGenericMethodreturns a fresh wrapper with a null cache, so genericization is unaffected. Single nit about array mutation risk for external callers (see inline). - TestMethodInfo.ResolveArguments (#9605): Correct.
ParameterTypesis the same cached array;ResolveArgumentsonly reads, never mutates. - TestMethodRunner._cachedReflectionMethodInfo (#9605, #9593): Correct. Data rows are processed sequentially (
foreach+await), so the lazy field has no concurrent access.
| // MethodInfo.GetParameters() allocates a fresh ParameterInfo[] on every call (CLR safety | ||
| // guarantee). The wrapped MethodInfo is immutable, so cache the array and share it across | ||
| // callers of this wrapper (e.g. display-name computation for every data-driven row). | ||
| public override ParameterInfo[] GetParameters() => _parameters ??= _methodInfo.GetParameters(); |
There was a problem hiding this comment.
[NIT] Performance & Allocations / Defensive Coding
ReflectionTestMethodInfo.GetParameters() now returns a cached ParameterInfo[] directly to any caller — including user-provided ITestDataSource.GetDisplayName(MethodInfo, ...) implementations. Unlike TestMethodInfo.ParameterTypes (which clones the array in the explicit ITestMethod interface implementation), this override hands out the shared cached instance.
If a (misbehaving) user data source mutates the returned array, the cache is silently corrupted for all subsequent rows. The CLR's own MethodInfo.GetParameters() returns a fresh array precisely to guard against this.
This is unlikely in practice, and the PR description acknowledges the trade-off. However, for parity with the TestMethodInfo.ParameterTypes defensive pattern, consider returning a clone here too (the per-row allocation is a single shallow array copy):
public override ParameterInfo[] GetParameters()
=> (ParameterInfo[])(_parameters ??= _methodInfo.GetParameters()).Clone();Alternatively, if the cost of even that copy matters, document the "must not mutate" contract in the XML doc for the override.
Recommendation: Add a brief XML doc <remarks> noting the cached-array contract, or clone before returning. Either way this is a nit — the optimization is sound for well-behaved callers.
There was a problem hiding this comment.
Good call. Cloning on every call would reallocate a fresh array each time and defeat the allocation-reduction goal of this change, so I went with your alternative: I documented the shared cached-array contract in an XML on the override, noting the returned array must be treated as read-only and that mutating it (including from user ITestDataSource.GetDisplayName implementations) would corrupt the cache. Fixed in 0a4c2dd.
…vangelink in #9617 (backport to rel/4.3) (#9619) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
9cf3807 to
716f157
Compare
Combines four micro-optimizations in the MSTest adapter execution path: - CloneForDataDrivenIteration: pass _properties directly to the ctor instead of building an intermediate snapshot Dictionary. The null/null ctor branch already takes a shallow copy, so one Dictionary + O(n) copy per data-driven iteration is removed. (#9602) - ExecuteTestAsync: add a fast path when no ExecutionContext was captured by [AssemblyInitialize]/[ClassInitialize] (the common case). Skips the TaskCompletionSource + closure + Action delegate allocations and awaits the executor directly; the captured-context slow path is unchanged. (#9603) - ReflectionTestMethodInfo.GetParameters(): cache the ParameterInfo[] since MethodInfo.GetParameters() allocates a fresh array on every call and the wrapped MethodInfo is immutable. (#9605, #9593) - TestMethodInfo.ResolveArguments(): use the cached ParameterTypes property instead of calling MethodInfo.GetParameters() directly. (#9605) - TestMethodRunner: reuse a single ReflectionTestMethodInfo wrapper across all data rows instead of allocating one per row. (#9605, #9593) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
716f157 to
0a4c2dd
Compare
Fixes #9602, #9603, #9605, #9593.
These four issues are all allocation micro-optimizations in the same MSTest adapter test-execution hot path, so they are grouped into a single PR. Notably #9605 and #9593 were near-duplicates (both cache
GetParameters()/ReflectionTestMethodInfoacross data rows); this PR unifies them.Changes
CloneForDataDrivenIteration([perf-improver] perf: skip intermediate Dictionary alloc in CloneForDataDrivenIteration #9602) — Pass_propertiesdirectly to theTestContextImplementationconstructor instead of building an intermediatesnapshotDictionary. ThetestMethod: null, testClassFullName: nullctor branch already takes a shallow copy, so the clone's property bag stays fully isolated while oneDictionary+ O(n) copy per data-driven iteration is eliminated.ExecuteTestAsync([perf-improver] perf: skip TaskCompletionSource bridge in ExecuteTestAsync when no ExecutionContext #9603) — Add a fast path for the common case where neither[AssemblyInitialize]nor[ClassInitialize]captured anExecutionContext. In that caseRunOnContext(null, …)just invokes the action directly, so theTaskCompletionSource<TestResult[]>+ async-lambda closure +Actiondelegate are pure overhead. The fast path awaits the executor directly; the captured-context slow path is unchanged and still bridges through the TCS to restore the captured context. Observable behavior (success and exception paths) is identical.ReflectionTestMethodInfo.GetParameters()([efficiency-improver] perf: cache GetParameters() and ReflectionTestMethodInfo across data rows #9605, [efficiency-improver] perf: cache ReflectionTestMethodInfo and GetParameters() across data rows #9593) — Cache theParameterInfo[].MethodInfo.GetParameters()allocates a fresh array on every call (CLR safety guarantee) and the wrappedMethodInfois immutable, so the array can be shared.MakeGenericMethodcreates a new wrapper with a fresh (null) cache, so genericization is unaffected.TestMethodInfo.ResolveArguments()([efficiency-improver] perf: cache GetParameters() and ReflectionTestMethodInfo across data rows #9605) — Use the already-cachedParameterTypesproperty instead of callingMethodInfo.GetParameters()directly (which bypassed the cache added in [efficiency-improver] perf: cache MethodInfo.GetParameters() to avoid per-row array allocations in data-driven tests #9514).ResolveArgumentsonly reads the array, so sharing is safe.TestMethodRunner([efficiency-improver] perf: cache GetParameters() and ReflectionTestMethodInfo across data rows #9605, [efficiency-improver] perf: cache ReflectionTestMethodInfo and GetParameters() across data rows #9593) — Reuse a single lazily-createdReflectionTestMethodInfowrapper across all data rows instead of allocating one per row. Both the wrappedMethodInfoandDisplayNameare immutable for the runner's lifetime.Impact
For a data-driven test with N rows, the redundant per-row allocations (intermediate snapshot Dictionary, TCS bridge trio, and
ReflectionTestMethodInfo+ParameterInfo[]) collapse from O(N) to O(1), reducing GC pressure with no behavioral change.Validation
.\build.cmd -c Release— Build succeeded, 0 warnings, 0 errors.MSTestAdapter.PlatformServices.UnitTests— 893 passed.MSTestAdapter.UnitTests— 21 passed.TestFramework.UnitTests— 1354 passed.Co-authored-by: Copilot App 223556219+Copilot@users.noreply.github.com