Uh oh!
There was an error while loading. Please reload this page.
perf: avoid GetParameters() allocation in test-invocation hot path - #9671
Conversation
MethodInfo.GetParameters() returns a fresh ParameterInfo[] on every call (CLR safety guarantee). GetInvokeResultAsync re-called it per invocation, allocating one array per data-driven test row (two for generic methods, via ConstructGenericMethod). Add GetInvokeResultWithParametersAsync, which accepts a pre-cached ParameterInfo[], and keep GetInvokeResultAsync as a thin wrapper for non-hot-path callers. Thread the cached array into ConstructGenericMethod and update the two hot-path call sites in TestMethodInfo.Execution.cs to pass the already-cached TestMethodInfo.ParameterTypes. Fixes#9661, #9647, #9659. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR eliminates redundant ParameterInfo[] heap allocations in the MSTest test-invocation hot path. MethodInfo.GetParameters() returns a freshly allocated array on every call, and MethodInfoExtensions.GetInvokeResultAsync re-invoked it on every test invocation (twice for generic methods, via ConstructGenericMethod). TestMethodInfo.ParameterTypes already caches this array (added in #9514/#9614), but the invocation call site never received it. This change threads that cached array through, completing the allocation-caching effort. It fits into src/Adapter/MSTestAdapter.PlatformServices, the platform-services layer shared by both test hosts.
Changes:
- Added internal
GetInvokeResultWithParametersAsync, which accepts a pre-cachedParameterInfo[]and holds the previous implementation;GetInvokeResultAsyncbecomes a thin wrapper that callsGetParameters()once and delegates. - Threaded the cached array into
ConstructGenericMethod, removing its internalGetParameters()call (the second allocation for generic methods). - Updated the two hot-path call sites in
TestMethodInfo.Execution.csto pass the cachedParameterTypes, and dropped now-redundant?./?? []on the non-nullable array.
Show a summary per file
| File | Description |
|---|---|
| src/Adapter/MSTestAdapter.PlatformServices/Extensions/MethodInfoExtensions.cs | Adds cached-parameter method, converts GetInvokeResultAsync to a delegating wrapper, and threads the array into ConstructGenericMethod. |
| src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodInfo.Execution.cs | Updates the two hot-path invocation sites to pass the cached ParameterTypes. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Medium
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.
Expert Review — PR #9671
Summary
Clean, well-scoped performance optimization that eliminates per-invocation ParameterInfo[] allocations on the hot path by threading the already-cached TestMethodInfo.ParameterTypes array into the invocation chain. No correctness or compatibility concerns.
Dimension Results
| # | Dimension | Result | Severity |
|---|---|---|---|
| 1 | Algorithmic Correctness | ✅ Clean | MAJOR |
| 2 | Test Coverage | ✅ Clean | MAJOR |
| 3 | Public API Design | ✅ Clean | MAJOR |
| 4 | Performance | ✅ Clean — achieves stated goal | MAJOR |
| 5 | Thread Safety & Concurrency | ✅ Clean | MAJOR |
| 6 | Error Handling & Resilience | ✅ Clean | MAJOR |
| 7 | Security | N/A | — |
| 8 | Breaking Changes | ✅ Clean | MAJOR |
| 9 | Cross-TFM Correctness | ✅ Clean | MAJOR |
| 10 | Naming & Style | ✅ Clean | MINOR |
| 11 | Documentation & Comments | ✅ Clean | MINOR |
| 12 | Code Duplication | ✅ Clean — reduced | MINOR |
| 13 | Simplicity | ✅ Clean | MINOR |
| 14 | IPC Contract Stability | N/A | — |
| 15 | Localization | N/A | — |
| 16 | Backward Compatibility | ✅ Clean | MAJOR |
| 17 | Resource Management | N/A | — |
| 18 | Null Safety | ✅ Clean | MINOR |
| 19 | Async Correctness | ✅ Clean | MAJOR |
| 20 | Test Quality | N/A (no test files changed) | — |
| 21 | Platform-Specific Code | ✅ Clean | MAJOR |
| 22 | Scope Discipline | ✅ Clean | MINOR |
Notes
- Correctness:
ConstructGenericMethodis called before anymethodInforeassignment, so the passedmethodParameters(from the samemethodInfo) is semantically identical to the removed internalmethodInfo.GetParameters()call. ✅ - Backward compatibility:
GetInvokeResultAsyncremains as a thin expression-bodied wrapper — all non-hot-path callers (assembly/class init & cleanup, test init/cleanup) are completely unchanged. - Null safety: The
?.→.and?? []→ removal simplifications are correct becausemethodParametersis now typed as non-nullableParameterInfo[]. - Thread safety: The backing
field ??= MethodInfo.GetParameters()has a benign race (multiple threads may allocate), but the array content is deterministic and never mutated, so sharing is safe. - Style: The
== null→is nullchange aligns with the repo's.editorconfigpreference.
Verdict
No issues found. The change is minimal, well-documented with the inline comment on the wrapper, and delivers a clear measurable allocation reduction on the hot path.
Uh oh!
There was an error while loading. Please reload this page.
Summary
MethodInfo.GetParameters()returns a freshly allocatedParameterInfo[]on every call (CLR safety guarantee).MethodInfoExtensions.GetInvokeResultAsyncre-called it on every test invocation, allocating one array per data-driven test row — and two for generic test methods, becauseConstructGenericMethodcalledGetParameters()again internally.TestMethodInfo.ParameterTypesalready caches this array (field ??= MethodInfo.GetParameters(), added in #9514/#9614), but the invocation call site never received it.Changes
GetInvokeResultWithParametersAsync, which accepts a pre-cachedParameterInfo[], holding the previous implementation.GetInvokeResultAsyncas a thin wrapper that callsGetParameters()once and delegates — all non-hot-path callers (assembly/class init & cleanup, test init/cleanup) are unchanged.ConstructGenericMethodinstead of it callingGetParameters()itself, removing the second allocation for generic methods.TestMethodInfo.Execution.csto pass the already-cachedParameterTypes.?./?? []on the non-nullableParameterInfo[].A distinct method name (rather than an overload) is used deliberately: an overload with a
ParameterInfo[]parameter would win overload resolution over theparams object?[]?wrapper for callers passing a literalnull(array covariance makesnull → ParameterInfo[]the "better" conversion), breaking theGetInvokeResultAsync(instance, null)call sites.Impact
ParameterInfo[]allocsParameterInfo[]allocsValidation
MSTestAdapter.PlatformServices.csprojbuilds clean (0 warnings, 0 errors) across all TFMs.MSTestAdapter.PlatformServices.UnitTests(incl. theGetInvokeResultAsyncregion) pass on net462/net48/net8.0/net9.0/net8.0-windows.Fixes#9661
Fixes#9647
Fixes#9659
Co-authored-by: Copilot App 223556219+Copilot@users.noreply.github.com