Clear the perf-improver backlog: reporting and terminal allocations

[Amaury Levé (Evangelink)]

Handles the remaining open perf-improver backlog items: the filed ticket (#10376) plus the actionable entries listed in the monthly-activity backlogs (#10381, #9604).

Changes

1. Cache TestMethodIdentifierProperty for parameterless tests (#10376)

ParsedManagedName.ToProperty() is called once per node built from a TestMethod. For a parameterless method the property is fully immutable — readonly strings/int plus an empty array that cannot be mutated — and ParsedManagedName is already cached per TestMethod (#10366), so the several nodes one executed test produces (the in-progress node, then one result node per data row and per in-process retry attempt) now share a single property instance.

The property is built eagerly in the ParsedManagedName constructor into a readonly field rather than lazily via ??=, so the shared instance doesn't depend on call ordering or on races between concurrent reporters. ToProperty() is always called on a fresh parse, so nothing is built that wouldn't have been built anyway.

Parameterized methods are deliberately unchanged: they still get a fresh property carrying a fresh copy of ParameterTypeFullNames, because that array is publicly exposed and must never be aliased across nodes.

2. Unblock AnsiTerminal.StopUpdate() (backlog item 3 — previously recorded as blocked by IConsole + netstandard2.0 compat)

StopUpdate() called _stringBuilder.ToString() on every flush — twice a second for cursor rendering, plus around ordinary terminal writes — and the batched progress frame is routinely kilobyte-scale. IConsole now has a Write(StringBuilder) overload; SystemConsole forwards it to TextWriter.Write(StringBuilder) on .NET, which walks the builder's chunks and writes each as a span, so no temporary string is materialised. netstandard2.0 / .NET Framework keep the ToString() copy behind #if, since the overload doesn't exist there — nothing regresses on those TFMs.

3. Reuse the slow-test description builder (backlog item 2)

SilenceDrivenHeartbeatRenderer.BuildSlowTestDescription allocated a StringBuilder (plus backing char[]) per surfaced slow test. It now reuses one lazily-created instance field, so a healthy run that never surfaces a slow test still allocates nothing. Reuse is safe for the same reason as the _slowTestThresholdStates dictionary next to it: both are only touched from the single rendering thread inside OnTick.

Deliberately not done

• DotnetTestHttpClientnew byte[1] probe buffer (backlog item 1). An earlier revision of this PR hoisted it to an instance field; that's been reverted. It saved one ~24-byte allocation per RPC on a path that already allocates a linked CTS, streams, ByteArrayContent and an HttpRequestMessage, and it silently coupled correctness to requests staying serialized by _requestLock forever. Not a good trade.
• OpenTelemetryResultHandler.GetFullyQualifiedName() (backlog item 4). One logical string build per test result, only on the OTel-enabled path. string.Concat with 5 operands, string.Join and a StringBuilder are all neutral-or-worse than what the compiler already emits for the interpolation.

Both are recorded as won't-fix on the tracking issues rather than churning the code.

Tests

• New: ToResultTestNode_ReusesTestMethodIdentifierInstance_ForParameterlessTestMethod and ToResultTestNode_DoesNotReuseTestMethodIdentifierInstance_ForParameterizedTestMethod in MSTestTestNodeConverterTests, pinning both halves of the selective caching policy alongside the existing array-isolation test.
• The TerminalTestReporterTests console fakes now implement Write(StringBuilder), so the whole terminal suite exercises the new flush path.
• .\build.cmd -test — full solution build + unit tests, 0 warnings, 0 errors across net462, net48, net8.0, net9.0 and netstandard2.0.

Review

Two independent review rounds were run over the diff (correctness/concurrency, plus a skeptical is-this-actually-a-win pass). Findings addressed: the lazy ??= identity race, a stale ParsedManagedNameCache doc block that still claimed the property is never cached, an overstated "re-runs" comment (discovery and execution don't share a TestMethod), eager builder allocation on healthy runs, and the byte[1] trade-off above. The final round found no remaining issues.

Closes#10376

[Copilot]

Pull request overview

Reduces allocations across MSTest reporting, terminal rendering, and HTTP transport paths.

Changes:

• Caches parameterless test identifiers and adds reuse tests.
• Reuses terminal-rendering and HTTP probe buffers.
• Adds allocation-free StringBuilder console output on modern .NET.

Show a summary per file

File	Description
MSTestTestNodeConverterTests.cs	Tests identifier sharing behavior.
CTRLPlusCCancellationTokenSourceTests.cs	Updates the console fake.
TerminalTestReporterTests.cs	Updates terminal console fakes.
DotnetTestHttpClient.cs	Reuses the trailing-byte buffer.
SilenceDrivenHeartbeatRenderer.cs	Reuses the description builder.
AnsiTerminal.cs	Writes batched builders directly.
InternalAPI.Unshipped.txt	Tracks new internal APIs.
SystemConsole.cs	Implements builder-based output.
IConsole.cs	Adds the builder write contract.
MSTestTestNodeConverter.cs	Caches parameterless identifiers.

Review details

• Files reviewed: 10/10 changed files
• Comments generated: 2
• Review effort level: Balanced

[github-actions]

Note

🤖 Automated review by GitHub Copilot. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.

Expert Review — 22 Dimensions

All four changes are allocation-reduction optimizations with clear safety arguments. No issues found.

#	Dimension	Verdict
1	Algorithmic Correctness	✅ Caching gated on _parameterTypeFullNames.Length == 0 (immutable case); parameterized path still copies. StringBuilder reuse confined to single-thread tick callback. _trailingByteBuffer guarded by _requestLock.
2	Thread Safety & Concurrency	✅ _cachedParameterlessProperty uses ??= which is safe (idempotent, immutable value). _slowTestDescriptionBuilder is single-thread (OnTick). _trailingByteBuffer is under _requestLock.
3	Public API Surface	✅ No new public API. IConsole and SystemConsole are internal. Correctly added to InternalAPI.Unshipped.txt.
4	Backward Compatibility	✅ No behavioral changes; purely allocation reductions.
5	Cross-TFM Correctness	✅ SystemConsole.Write(StringBuilder) uses #if NETCOREAPP for the zero-copy path, falls back to .ToString() on netstandard2.0/.NET Framework.
6	Performance	✅ This is the perf improvement — all four changes reduce allocations on hot paths.
7	Error Handling	N/A
8	Resource Management	✅ No new disposables or streams.
9	Security	N/A
10	Naming & Conventions	✅ Field names follow existing patterns (_cachedParameterlessProperty, _slowTestDescriptionBuilder, _trailingByteBuffer).
11	Code Style	✅ Consistent with codebase conventions.
12	Documentation	✅ Good inline comments explaining why each optimization is safe.
13	Localization	N/A
14	Test Coverage	✅ Two new tests verify caching for parameterless and non-caching for parameterized. Existing DoesNotShareParameterTypeArray test still applies.
15	Test Quality	✅ Tests use BeSameAs/NotBeSameAs — correct assertions for reference identity checks.
16	IPC Contract	N/A
17	Logging & Diagnostics	N/A
18	PublicAPI.*.txt	✅ No public API changes. Internal API file updated correctly.
19	Analyzers	N/A
20	MSBuild / SDK	N/A
21	Scope Discipline	✅ Four related perf items, all allocation reductions — reasonable to batch.
22	Follow-up Tracking	✅ PR description references #10376, #10381, #9604.

Summary: Clean performance PR. All optimizations have sound safety invariants documented in comments. No issues to flag.

[Copilot]

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

Review details

Suppressed comments (1)

src/Platform/Microsoft.Testing.Platform/Helpers/System/SystemConsole.cs:124

• CaptureConsoleOutWriter is configured with AutoFlush = true. TextWriter.Write(StringBuilder) calls Write(ReadOnlySpan<char>) once per StringBuilder chunk, and StreamWriter auto-flushes after each such call. A multi-chunk progress frame therefore flushes the underlying stdout stream multiple times instead of once as the previous Write(string) path did, potentially replacing the saved allocation with additional blocking I/O. Please use a chunk-writing path that suppresses auto-flush for the complete logical write and flushes once at the end; changing the shared writer's AutoFlush state also needs synchronization with the other write methods.

 CaptureConsoleOutWriter.Write(value);

• Files reviewed: 9/9 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[github-actions]

🧪 Test quality grade — PR #10384

Grade	Test	Mutation	Notes	How to improve
A (90–100)	new MSTestTestNodeConverterTests. ToResultTestNode_ ReusesTestMethodIdentifierInstance_ ForParameterlessTestMethod	2/2 killed	BeSameAs directly falsifies any mutation that skips or bypasses the parameterless property cache.	—
A (90–100)	new MSTestTestNodeConverterTests. ToResultTestNode_ DoesNotReuseTestMethodIdentifierInstance_ ForParameterizedTestMethod	1/1 killed	NotBeSameAs correctly guards the inverse decision — extending the cache to parameterized methods would fail it.	—

_{This advisory comment was generated automatically. Grades are heuristic and informational — they do not block merging. Re-run with /grade-tests.}

🤖 Automated content by GitHub Copilot. Generated by the Grade Tests on PR (on open / sync) workflow. · sonnet46 50 AIC · ⌖ 4.16 AIC · ⊞ 11.8K · [◷]( · ◷)

[Amaury Levé (Evangelink)]

CaptureConsoleOutWriter is configured with AutoFlush = true. [...] A multi-chunk progress frame therefore flushes the underlying stdout stream multiple times instead of once as the previous Write(string) path did

Good catch in principle, but I measured it and it does not hold beyond the first frame, so I am leaving the code as is.

AnsiTerminal reuses one _stringBuilder and calls Clear() in StartUpdate(). StringBuilder.Clear() consolidates the chunk chain back into a single capacity-preserving chunk, so the builder is multi-chunk only while it is growing for the very first frame. Simulating the real pattern (one reused builder, Clear() per frame, a ~19.6 KB frame built from many small Append calls):

frame 1: length=19600 capacity=27264 chunks=12
frame 2: length=19600 capacity=23520 chunks=1
frame 3: length=19600 capacity=23520 chunks=1
frame 4: length=19600 capacity=23520 chunks=1
frame 5: length=19600 capacity=23520 chunks=1

So the extra auto-flushes are bounded to 11 additional ConsoleStream.Flush() calls on the first StopUpdate() of a process, one time. Every subsequent frame is a single chunk, i.e. exactly one Write(ReadOnlySpan<char>) and one auto-flush, which is identical to what Write(string) did. The underlying _stream.Write calls are the same bytes either way.

The proposed fix would mean toggling AutoFlush on CaptureConsoleOutWriter, which is a static writer shared with the file logger and other console paths. As the comment itself notes, that needs synchronization with the other write methods, so it would trade a one-time, negligible cost for shared mutable writer state and a new race. Not a good trade here.