perf: use GetUpperCaseName in InternalSyncLog to avoid Enum.ToString() allocation - #10265
Conversation
…) 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
There was a problem hiding this comment.
Pull request overview
Optimizes synchronous file logging by avoiding Enum.ToString() allocations and aligning log-level casing with asynchronous logging.
Changes:
- Uses
GetUpperCaseNamein 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
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.
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.
🧪 Test quality grade — PR #10265
This advisory comment was generated automatically. Grades are heuristic
|
Fixes #10261
What
FileLogger.InternalSyncLoginterpolated{logLevel}directly, which callsEnum.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
BuildLogEntry→GetUpperCaseName, 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'sHH:mm:ss.ffftimestamp format — is unchanged.Tests
Log_WhenSyncFlush_LogLevelIsWrittenInUpperCase(data-driven over the existing exhaustiveExpectedUpperCaseNamestable) andLog_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.