Skip to content

perf: reduce allocations in data-driven test execution hot path#9617

Open
Evangelink wants to merge 1 commit into
mainfrom
dev/amauryleve/perf-data-driven-allocations
Open

perf: reduce allocations in data-driven test execution hot path#9617
Evangelink wants to merge 1 commit into
mainfrom
dev/amauryleve/perf-data-driven-allocations

Conversation

@Evangelink

Copy link
Copy Markdown
Member

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() / ReflectionTestMethodInfo across data rows); this PR unifies them.

Changes

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

Copilot AI review requested due to automatic review settings July 5, 2026 09:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 snapshot Dictionary in CloneForDataDrivenIteration by passing _properties directly to the constructor (which already deep-copies in the null/null branch).
  • Add a null-ExecutionContext fast path in ExecuteTestAsync that awaits the executor directly, avoiding the TaskCompletionSource + closure + delegate bridge when no context was captured.
  • Cache ParameterInfo[] in ReflectionTestMethodInfo.GetParameters(), reuse a single ReflectionTestMethodInfo wrapper across rows, and switch ResolveArguments() to the already-cached ParameterTypes.
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

@Evangelink Evangelink added the state/needs-review Awaiting review from the team. label Jul 5, 2026
@Evangelink Evangelink enabled auto-merge (squash) July 5, 2026 09:30
@Evangelink

Copy link
Copy Markdown
Member Author

/backport to rel/4.3

@github-actions

github-actions Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Started backporting to rel/4.3: https://github.com/microsoft/testfx/actions/runs/28736320032

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-provided ITestDataSource implementations without cloning. Unlikely to cause issues in practice, but differs from the defensive clone pattern used in TestMethodInfo.ParameterTypes. Consider either cloning or documenting the contract.

Summary of review findings:

  • CloneForDataDrivenIteration (#9602): Correct. Eliminating the intermediate snapshot Dictionary is safe because the constructor's [with(properties)] branch already creates an isolated copy. No new race window introduced (the lock-free read of _properties was equally present in the old code).
  • ExecuteTestAsync fast path (#9603): Correct. The capturedContext is null guard faithfully reproduces the semantics of RunOnContext(null, action) (which just calls action() directly). Exception propagation, using-block disposal order, and ConfigureAwait(false) are all preserved.
  • ReflectionTestMethodInfo.GetParameters() cache (#9605, #9593): Correct and effective. MakeGenericMethod returns 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. ParameterTypes is the same cached array; ResolveArguments only 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();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Evangelink pushed a commit that referenced this pull request Jul 5, 2026
…vangelink in #9617 (backport to rel/4.3) (#9619)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Evangelink Evangelink force-pushed the dev/amauryleve/perf-data-driven-allocations branch from 9cf3807 to 716f157 Compare July 5, 2026 19:32
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>
Copilot AI review requested due to automatic review settings July 5, 2026 19:45
@Evangelink Evangelink force-pushed the dev/amauryleve/perf-data-driven-allocations branch from 716f157 to 0a4c2dd Compare July 5, 2026 19:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

  • Files reviewed: 4/4 changed files
  • Comments generated: 0 new
  • Review effort level: Medium

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

state/needs-review Awaiting review from the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[perf-improver] perf: skip intermediate Dictionary alloc in CloneForDataDrivenIteration

4 participants