Skip to content

perf: use GetUpperCaseName in InternalSyncLog to avoid Enum.ToString() allocation - #10265

Merged
Evangelink merged 1 commit into
mainfrom
dev/amauryleve/perf-sync-log-upper-case-name
Jul 28, 2026
Merged

perf: use GetUpperCaseName in InternalSyncLog to avoid Enum.ToString() allocation#10265
Evangelink merged 1 commit into
mainfrom
dev/amauryleve/perf-sync-log-upper-case-name

Conversation

@Evangelink

Copy link
Copy Markdown
Member

Fixes #10261

What

FileLogger.InternalSyncLog interpolated {logLevel} directly, which calls Enum.ToString() — boxing the enum and allocating a fresh string for every log line written on the sync-flush path.

The async/enqueue paths already go through BuildLogEntryGetUpperCaseName, which returns an interned literal ("TRACE", "WARNING", …) with zero allocation. This applies the same helper to the sync path.

Behavior change

The level in the sync-flush diagnostic log now renders upper-cased ([00:00:00.000 Test - TRACE] … instead of - Trace]), matching the async path. Everything else — including the sync path's HH:mm:ss.fff timestamp format — is unchanged.

Tests

  • Updated the two existing sync-path assertions to expect the upper-cased level.
  • Added Log_WhenSyncFlush_LogLevelIsWrittenInUpperCase (data-driven over the existing exhaustive ExpectedUpperCaseNames table) and Log_WhenSyncFlush_UndefinedLogLevelIsWrittenAsItsNumericValue, mirroring the async-path coverage so the sync formatter no longer relies on the async tests for level rendering.

Microsoft.Testing.Platform.UnitTests — 127/127 passing on net9.0.

…) allocation

The sync-flush log path (InternalSyncLog) used {logLevel} directly in an
interpolated string, which calls Enum.ToString() -- boxing the enum and
allocating a string on every log entry.

The async/queue paths already route through BuildLogEntry + GetUpperCaseName,
which returns an interned literal instead. Apply the same pattern to the
sync-flush path for consistency and to eliminate the per-entry allocation.

Fixes #10261

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 48fea024-c525-4a42-b6c3-20c3eed414e2
Copilot AI review requested due to automatic review settings July 27, 2026 16:21

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

Optimizes synchronous file logging by avoiding Enum.ToString() allocations and aligning log-level casing with asynchronous logging.

Changes:

  • Uses GetUpperCaseName in the synchronous logging path.
  • Updates existing expectations and adds exhaustive sync-path coverage.
Show a summary per file
File Description
src/Platform/Microsoft.Testing.Platform/Logging/FileLogger.cs Uses allocation-free uppercase names for known log levels.
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Logging/FileLoggerTests.cs Verifies uppercase and undefined log-level formatting.

Review details

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

@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. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.

Review Summary

Clean, well-scoped performance fix. No issues found.

Verdict Table

# Dimension Verdict
1 Algorithmic Correctness GetUpperCaseName is a pure switch expression already used by the async path; applying it to the sync path is trivially correct.
2 Edge Cases & Defensive Coding ✅ Undefined enum values fall through to ToString().ToUpper() — tested by new UndefinedLogLevelIsWrittenAsItsNumericValue test.
3 Concurrency & Thread Safety ✅ N/A — no change to locking; _semaphore still guards the sync path.
4 Performance & Allocations ✅ This is the fix — eliminates Enum.ToString() boxing + allocation per sync log line.
5 API Design & Public Surface ✅ N/A — no public API change; GetUpperCaseName is private static.
6 Backward Compatibility ✅ Behavior change (upper-case level name in sync logs) is intentional and aligns sync with async output. Diagnostic log format is not a public contract.
7 Error Handling ✅ N/A — no change to error paths.
8 Cross-TFM Correctness ✅ N/A — changed line is outside any #if block.
9 Resource Management ✅ N/A.
10 Nullability & Type Safety ✅ N/A.
11 Naming & Readability ✅ Clear.
12 Test Quality ✅ New tests mirror the async-path coverage (DynamicData over all levels + undefined value). Good use of SyncFlushDefaultClockTimestamp constant and LogSingleEntryWithSyncFlush helper.
13 Test Conventions ✅ Uses MSTest Assert, consistent with BannedSymbols.txt for this project.
14 Localization N/A — no resource string changes.
15 Security N/A.
16 IPC & Serialization N/A.
17 Configuration & Options N/A.
18 Logging & Diagnostics ✅ The logging output format is the subject of the fix; both paths now agree.
19 MSBuild & Packaging N/A.
20 Documentation ✅ Inline comments on the new constant and tests are helpful.
21 Scope Discipline ✅ Single-concern PR with matching issue reference.
22 PublicAPI.Unshipped.txt ✅ N/A — no public API additions.

Result: All clear. No blocking or non-blocking issues identified.

@github-actions

Copy link
Copy Markdown
Contributor

🧪 Test quality grade — PR #10265

GradeTestMutationNotesHow to improve
B (80–89) mod FileLoggerTests.
Write_
IfMalformedUTF8_
ShouldNotCrash
2/3 killed Partial-match assertions (StartsWith/EndsWith) leave the timestamp format and full entry structure unchecked; intentional because a real clock is used. Inject a mock clock and switch to Assert.AreEqual with the full expected string to kill timestamp-format mutations.
A (90–100) mod FileLoggerTests.
Log_
WhenSyncFlush_
StreamWriterIsCalledOnlyWhenLogLevelAllowsIt
4/4 killed Exact string match for the enabled path and a length check for the disabled path cover all meaningful mutations; oracle uses ToUpperInvariant rather than independent literals but the level-gating intent is fully verified.
A (90–100) new FileLoggerTests.
Log_
WhenSyncFlush_
LogLevelIsWrittenInUpperCase
4/4 killed Independent hardcoded oracle (ExpectedUpperCaseNames) prevents the test from passing if GetUpperCaseName regresses; all 7 named levels covered as separate data rows.
A (90–100) new FileLoggerTests.
Log_
WhenSyncFlush_
UndefinedLogLevelIsWrittenAsItsNumericValue
4/4 killed Exact match for the switch-fallback branch with an out-of-range value; console-no-write check prevents silent rerouting.

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 55.5 AIC · ⌖ 5.84 AIC · ⊞ 10.3K · [◷]( · )

@Evangelink
Evangelink enabled auto-merge (squash) July 27, 2026 17:29
@Evangelink Evangelink added the state/needs-review Awaiting review from the team. label Jul 27, 2026
@Evangelink
Evangelink merged commit 9d25639 into main Jul 28, 2026
28 checks passed
@Evangelink
Evangelink deleted the dev/amauryleve/perf-sync-log-upper-case-name branch July 28, 2026 07:19
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: use GetUpperCaseName in InternalSyncLog to avoid Enum.ToString() allocation

3 participants