Skip to content

[perf-improver] perf: single-pass message iteration in VSTestBridge ObjectModelConverters#8823

Merged
Evangelink merged 4 commits into
mainfrom
perf-assist/vstest-bridge-single-pass-messages-7ba386ead473d7a7
Jun 5, 2026
Merged

[perf-improver] perf: single-pass message iteration in VSTestBridge ObjectModelConverters#8823
Evangelink merged 4 commits into
mainfrom
perf-assist/vstest-bridge-single-pass-messages-7ba386ead473d7a7

Conversation

@Evangelink
Copy link
Copy Markdown
Member

🤖 This is an automated contribution from Perf Improver.

Goal and Rationale

When converting a VSTest TestResult to a TestNode in the VSTestBridge adapter, testResult.Messages was iterated twice when TRX reporting was enabled:

  1. A LINQ Select pass to build a TrxMessage[] for TrxMessagesProperty — creates a SelectIterator<> state machine and an intermediate array via [.. ...] collection spread
  2. A separate foreach to collect standardErrorMessages / standardOutputMessages for StandardErrorProperty / StandardOutputProperty

This also removes a dead addVSTestProviderProperties variable that was declared but its value was never read (the call-site was removed at some earlier point).

Approach

Replace the two separate iterations with a single foreach that builds all three collections in one pass:

  • When isTrxEnabled, transform each message to a TrxMessage and collect it in a pre-allocated List<TrxMessage>
  • Simultaneously collect standard-error and standard-output strings (as before)
  • After the loop, use the collected List<TrxMessage> to create TrxMessagesProperty

Performance Evidence

Path Before After
testResult.Messages iterations (TRX enabled) 2 (LINQ Select + foreach) 1 (single foreach)
Allocations per call (TRX enabled) SelectIterator<> + intermediate TrxMessage[] + dead bool + List<string>? × 2 List<TrxMessage> (pre-allocated) + List<string>? × 2
Allocations per call (TRX disabled) dead bool + List<string>? × 2 List<string>? × 2

For a typical CI run with 1 000 tests and TRX enabled, this eliminates ≥1 000 SelectIterator state machines and ≥1 000 intermediate TrxMessage[] allocations.

Methodology: code inspection + diff review. ToTestNode(TestResult) is called once per test result in the VSTestBridge execution path.

Trade-offs

  • Slightly more verbose loop body when TRX is enabled (explicit TrxMessage trxMsg = switch { ... }), but the logic is clearer than splitting across two separate iterations.
  • No change to observable behaviour: the same properties are added in the same order.

Test Status

  • Microsoft.Testing.Platform.UnitTests (net8.0): 1079 passed, 0 failed, 3 skipped
  • Microsoft.Testing.Extensions.UnitTests (net8.0): 392 passed, 0 failed, 7 skipped
  • Build: 0 warnings, 0 errors

Reproducibility

./build.sh
artifacts/bin/Microsoft.Testing.Extensions.UnitTests/Debug/net8.0/Microsoft.Testing.Extensions.UnitTests

Generated by Perf Improver

Generated by Perf Improver · sonnet46 10.8M ·

Add this agentic workflows to your repo

To install this agentic workflow, run

gh aw add githubnext/agentics/workflows/perf-improver.md@main

…ters

When converting a VSTest TestResult to a TestNode, testResult.Messages was
iterated twice when TRX reporting was enabled:
  1. A LINQ Select pass to build TrxMessage[] (creates SelectIterator state
     machine + intermediate array via collection spread)
  2. A separate foreach to collect standard-error/standard-output strings

Replace with a single foreach that collects all three outputs in one pass.
Also removes a dead addVSTestProviderProperties variable whose value was
never read after the usage was previously removed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 4, 2026 14:42
@Evangelink Evangelink added area/performance Runtime / build performance / efficiency. type/automation Created or maintained by an agentic workflow. labels Jun 4, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

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 optimizes the VSTestBridge adapter’s conversion from VSTest TestResult to MTP TestNode by consolidating testResult.Messages processing into a single pass, reducing iterator/collection allocations when TRX reporting is enabled.

Changes:

  • Replaces the prior Select(...)+[..] TRX message projection plus a separate foreach with a single foreach that collects TRX messages, stdout, and stderr together.
  • Removes an unused addVSTestProviderProperties local that was computed but never read.
Show a summary per file
File Description
src/Platform/Microsoft.Testing.Extensions.VSTestBridge/ObjectModel/ObjectModelConverters.cs Consolidates message processing into one loop and removes a dead local to reduce allocations in the VSTestBridge conversion path.

Copilot's findings

  • Files reviewed: 1/1 changed files
  • Comments generated: 1

@Evangelink Evangelink marked this pull request as ready for review June 4, 2026 16:40
@Evangelink Evangelink enabled auto-merge (squash) June 4, 2026 16:41
Pre-allocate the trxMessages list with the exact message count to avoid List<T> growth reallocations, and switch the loop guard from isTrxEnabled to a direct null check on trxMessages so the null-forgiveness operator is no longer needed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Evangelink
Copy link
Copy Markdown
Member Author

🔍 Build Failure Analysis

Workflow run: https://github.com/microsoft/testfx/actions/runs/26975085514

❌ Root Cause: IDE0028 — Collection initialization can be simplified

The build fails with 3 errors (one per target framework: netstandard2.0, net8.0, net9.0), all pointing to the same single location:

src/Platform/Microsoft.Testing.Extensions.VSTestBridge/ObjectModel/ObjectModelConverters.cs(145,56): error IDE0028

The newly introduced line:

List<TrxMessage>? trxMessages = isTrxEnabled ? new List<TrxMessage>(testResult.Messages.Count) : null;

triggers IDE0028 ("Collection initialization can be simplified") because .editorconfig sets dotnet_style_prefer_collection_expression = true and the type List<TrxMessage> is redundant — it can be inferred from the left-hand side. In CI the project builds with /p:TreatWarningsAsErrors=true, promoting this warning to an error.

✅ Fix

Use a target-typed new expression to drop the redundant type annotation while preserving the capacity hint:

// Before (triggers IDE0028)
List<TrxMessage>? trxMessages = isTrxEnabled ? new List<TrxMessage>(testResult.Messages.Count) : null;

// After
List<TrxMessage>? trxMessages = isTrxEnabled ? new(testResult.Messages.Count) : null;

This is the minimal one-character change that satisfies the analyzer, keeps the pre-sizing optimisation intact (important on netstandard2.0 where EnsureCapacity is unavailable), and is idiomatic C#.

An inline suggestion has been posted directly on the offending line below.

Generated by Build Failure Analysis for issue #8823 · sonnet46 2.6M ·

Copy link
Copy Markdown
Member Author

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

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

Generated by Build Failure Analysis for issue #8823 · sonnet46 2.6M

The new List<TrxMessage>(testResult.Messages.Count) constructor call
triggers IDE0028 (warn-as-error in CI) which suggests a collection
expression. Capacity hints are not expressible in collection-expression
form, so wrap the line with the same pragma pattern other call sites in
this repo already use (TestContextImplementation.cs, TelemetryCollector.cs).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 5, 2026 07:39
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot's findings

  • Files reviewed: 1/1 changed files
  • Comments generated: 1

…ception ordering

Addresses review comment on PR #8823: the original pre-PR code threw
InvalidOperationException from FullyQualifiedName parsing before iterating
testResult.Messages, so an UnreachableException from an unexpected msg.Category
could not fire in the malformed-FQN case. The previous version of the
single-pass refactor moved the foreach above the FQN validation, which changed
the observable exception order for error paths.

This commit hoists the TRX-only property additions (TrxExceptionProperty,
TrxTestDefinitionName, TrxFullyQualifiedTypeNameProperty) above the message
loop while keeping TrxMessagesProperty after the loop so it can consume the
collected list. Behavior is now identical to the pre-PR code for both happy
and error paths, and the single-pass perf win is preserved.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Evangelink Evangelink merged commit 073d794 into main Jun 5, 2026
48 checks passed
@Evangelink Evangelink deleted the perf-assist/vstest-bridge-single-pass-messages-7ba386ead473d7a7 branch June 5, 2026 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/performance Runtime / build performance / efficiency. type/automation Created or maintained by an agentic workflow.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants