Uh oh!
There was an error while loading. Please reload this page.
perf: cache JsonSerializerOptions in performance runner steps - #9735
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR is a small performance/best-practices improvement to the internal performance-runner tooling under test/Performance/MSTest.Performance.Runner. It caches the JsonSerializerOptions { WriteIndented = true } instance in DotnetTestProcess as a private static readonly field instead of constructing a new instance on every ExecuteAsync() call, and removes the #pragma warning disable/restore CA1869 pair that was previously suppressing the analyzer warning for repeated construction.
I verified:
- The new field
JsonOptionsuses PascalCase, consistent with the repo'sprivate static readonlynaming convention and with the identicalJsonOptionsfield already present in the siblingPlainProcess.cs(line 11). - The only
JsonSerializer.Serializecall in the file (line 124) now uses the cached field, so removing the CA1869 pragma is safe — no remaining inlinenew JsonSerializerOptionsconstruction sites exist. - The UTF-8 BOM on line 1 is preserved.
Changes:
- Added
private static readonly JsonSerializerOptions JsonOptions = new() { WriteIndented = true };toDotnetTestProcess. - Replaced the inline
new JsonSerializerOptions { WriteIndented = true }at the serialization call with the cached field. - Removed the now-unnecessary
#pragma warning disable/restore CA1869pair.
Show a summary per file
| File | Description |
|---|---|
test/Performance/MSTest.Performance.Runner/Steps/DotnetTestProcess.cs | Caches JsonSerializerOptions in a static readonly field, updates the serialize call to use it, and drops the CA1869 suppression pragma. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Medium
🧪 Test quality grade — PR #9735No new or modified test methods were identified in the changed regions of this PR. Nothing to grade. Re-run with
|
There was a problem hiding this comment.
Note
🤖 Automated review by GitHub Copilot. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.
✅ 22/22 dimensions clean — no findings.
The change correctly caches JsonSerializerOptions as a private static readonly field, consistent with the existing pattern in PlainProcess.cs. JsonSerializerOptions is thread-safe once frozen (which happens on first serialization call), so the static field is safe even under concurrent access. The CA1869 pragma removal is appropriate now that the analyzer's guidance is followed.
Uh oh!
There was an error while loading. Please reload this page.
Fixes#9714
Summary
Cache the
JsonSerializerOptions { WriteIndented = true }instance as aprivate static readonlyfield inDotnetTestProcess, and remove theCA1869pragma suppression that was previously hiding the repeated-construction anti-pattern.JsonSerializerOptionsconstruction triggers reflection-based scanning of converter types and type metadata. Constructing a new instance on everyExecuteAsync()call repeats this work unnecessarily — a benchmark tool that measures performance should not itself incur avoidable overhead.Changes
test/Performance/MSTest.Performance.Runner/Steps/DotnetTestProcess.csprivate static readonly JsonSerializerOptions JsonOptions; removed#pragma warning disable/restore CA1869pairNote:
PlainProcess.csalready had an equivalent cachedJsonOptionsfield on this branch, so no change was needed there. The new field uses the sameJsonOptionsname for consistency.Trade-offs
None. Purely mechanical — same behaviour and output, no logic changes.
Co-authored-by: Copilot App 223556219+Copilot@users.noreply.github.com