Optimize ErrorUtilities several tracing methods for allocation-free message formatting#13556
Open
DustinCampbell wants to merge 19 commits intodotnet:mainfrom
Open
Optimize ErrorUtilities several tracing methods for allocation-free message formatting#13556DustinCampbell wants to merge 19 commits intodotnet:mainfrom
DustinCampbell wants to merge 19 commits intodotnet:mainfrom
Conversation
- Introduce StringBuilderHelper ref struct to hold a StringBuilder for custom interpolated string handlers. - Add TraceInterpolatedStringHandler to handle formatting of interpolated strings passed to TraceEngine calls. Note that TraceInterpolatedStringHandler uses BuildRequestEngine._debugDumpState to control whether it is available for string formatting. - Add TraceEngine(ref TraceInterpolatedStringHandler) overload. - Update calls of TraceEngine overloads that pass format strings are arguments to pass interpolated strings instead. - Update existing TraceEngine method to take a message string, rather than a format string + arguments.
The "EngineTrace" file path used by BuildRequestEngine never changes during its lifetime. So, it can be computed up front.
- Add TraceInterpolatedStringHandler to handle formatting of interpolated strings passed to TraceScheduler calls. Note that TraceInterpolatedStringHandler uses Scheduler._debugDumpState to control whether it is available for string formatting. - Add TraceScheduler(ref TraceInterpolatedStringHandler) overload. - Update calls of TraceScheduler overloads that pass format strings are arguments to pass interpolated strings instead. - Update existing TraceScheduler method to take a message string, rather than a format string + arguments.
The "SchedulerTrace" and "SchedulerState" file paths used by Scheduler never changes during its lifetime. So, they can be computed up front.
- Remove #if BUILDINGAPPXTASKS code blocks. (BUILDINGAPPXTASKS is not enabled anywhere within MSBuild. - Use file-scoped namespace.
- Add DebugTraceInterpolatedStringHandler to handle formatting of interpolated strings passed to DebugTraceMessage calls. Note that DebugTraceInterpolatedStringHandler uses ErrorUtilities.s_enableMSBuildDebugTracing to control whether it is available for string formatting. - Add DebugTraceMessage(string, ref DebugTraceInterpolatedStringHandler) overload. - Update calls of DebugTraceMessage that pass format strings and arguments to pass interpolated strings instead. - Update existing DebugTraceMessage method to take a message string, rather than a format string + arguments.
- Add IsTrueInterpolatedStringHandler to handle formatting of interpolated strings passed to VerifyThrow calls. Note that IsTrueInterpolatedStringHandler uses the condition parameter to control whether it is available for string formatting. - Add VerifyThrow(bool, ref IsTrueInterpolatedStringHandler) overload. - Remove all unnecessary VerifyThrow overloads. (Caller updates are in the next commit.)
- Update calls of ErrorUtilities.VerifyThrow overloads that pass format strings and arguments to pass interpolated strings instead.
ErrorUtilities has two methods that do exactly the same thing: VerifyThrowInternalError and VerifyThrow. This change updates callers of VerifyThrowInternalError to call VerityThrow instead and removes VerifyThrowInternalError.
- Add UnconditionalInterpolatedStringHandler to handle formatting of interpolated strings passed to ThrowInternalError calls. - Add ThrowInternalError(ref UnconditionalInterpolatedStringHandler) overload. - Add ThrowInternalError(ref UnconditionalInterpolatedStringHandler, Exception?) overload. - Remove all unnecessary ThrowInternalError overloads. (Caller updates are in the next commit.)
- Update calls of ErrorUtilities.ThrowInternalError overloads that pass format strings and arguments to pass interpolated strings instead.
EngineFileUtilities calls ErrorUtilities.DebugTraceMessage incorrectly. Instead of passing a category string and message, it passes a format string and argument.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors a large set of ErrorUtilities/tracing call sites to use interpolated strings (and custom interpolated string handlers) to avoid allocations from params object[], boxing, and string.Format in the non-error / tracing-disabled path.
Changes:
- Introduces handler-based APIs in
FrameworkErrorUtilitiesand forwardsShared/ErrorUtilitiesto them. - Updates many
VerifyThrow,ThrowInternalError, andDebugTraceMessagecall sites to interpolated strings. - Reworks scheduler/engine trace helpers to avoid formatting work unless tracing is enabled, and adds shared formatting helpers in
src/Framework/Utilities/.
Reviewed changes
Copilot reviewed 72 out of 72 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Utilities/TrackedDependencies/FileTracker.cs | Convert VerifyThrow to interpolated string. |
| src/Utilities/ToolLocationHelper.cs | Convert many debug trace messages to interpolated strings. |
| src/Utilities/AssemblyResources.cs | Convert missing-resource throw to interpolated string. |
| src/Tasks/XamlTaskFactory/TaskGenerator.cs | Improve VerifyThrow message; remove params formatting. |
| src/Tasks/XamlTaskFactory/CommandLineToolSwitch.cs | Convert type-check throws to interpolated strings. |
| src/Tasks/MSBuild.cs | Convert internal error/verify messages to interpolated strings. |
| src/Tasks/GenerateResource.cs | Convert VerifyThrow messages to interpolated strings. |
| src/Tasks/AssemblyResources.cs | Convert missing-resource throws to interpolated strings. |
| src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs | Convert HRESULT check message to interpolated string. |
| src/Shared/UnitTests/ErrorUtilities_Tests.cs | Updates tests to compile with new overload strategy. |
| src/Shared/TaskParameter.cs | Convert VerifyThrow message to interpolated string. |
| src/Shared/TaskLoggingHelper.cs | Refactor DEBUG-only message code assertion to reduce formatting duplication. |
| src/Shared/ResourceUtilities.cs | Convert internal error formatting to interpolated string. |
| src/Shared/NodePacketFactory.cs | Convert internal error messages to interpolated strings. |
| src/Shared/NodeEndpointOutOfProcBase.cs | Convert link-status internal checks to interpolated strings. |
| src/Shared/LogMessagePacketBase.cs | Convert unsupported event messages to interpolated strings. |
| src/Shared/ErrorUtilities.cs | Major refactor: forward to FrameworkErrorUtilities and add handler-based overloads. |
| src/MSBuild/OutOfProcTaskHostNode.cs | Convert internal error message to interpolated string. |
| src/MSBuild/AssemblyResources.cs | Convert missing-resource throw to interpolated string. |
| src/Framework/Utilities/UnconditionalInterpolatedStringHandler.cs | New unconditional handler for allocation-reduced formatting. |
| src/Framework/Utilities/StringBuilderHelper.cs | New shared helper for handler implementations. |
| src/Framework/ErrorUtilities.cs | Implement FrameworkErrorUtilities handler-based tracing/throw helpers. |
| src/Build/Utilities/EngineFileUtilities.cs | Update debug tracing to new category+message form. |
| src/Build/TelemetryInfra/TelemetryCollectorProvider.cs | Convert component-type assertion message to interpolated string. |
| src/Build/Resources/AssemblyResources.cs | Convert missing-resource throw to interpolated string. |
| src/Build/Logging/ParallelLogger/ParallelConsoleLogger.cs | Convert VerifyThrow message to interpolated string. |
| src/Build/Instance/TaskRegistry.cs | Replace removed VerifyThrowInternalError usage with new VerifyThrow. |
| src/Build/Instance/TaskFactoryWrapper.cs | Convert internal error message to interpolated string. |
| src/Build/Instance/ProjectInstance.cs | Convert VerifyThrow messages to interpolated strings. |
| src/Build/Evaluation/SimpleProjectRootElementCache.cs | Convert VerifyThrow message to interpolated string. |
| src/Build/Evaluation/ProjectRootElementCache.cs | Convert VerifyThrow message to interpolated string. |
| src/Build/Construction/ProjectElement.cs | Convert internal error message to interpolated string. |
| src/Build/BuildCheck/Infrastructure/BuildCheckManagerProvider.cs | Convert component-type assertion message to interpolated string. |
| src/Build/BackEnd/Shared/BuildResult.cs | Convert VerifyThrow message to interpolated string. |
| src/Build/BackEnd/Shared/BuildRequestConfiguration.cs | Convert VerifyThrow messages to interpolated strings. |
| src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs | Convert internal error message to interpolated string + inner exception overload. |
| src/Build/BackEnd/Components/SdkResolution/SdkResolverManifest.cs | Convert internal error message to interpolated string + inner exception overload. |
| src/Build/BackEnd/Components/Scheduler/SchedulingData.cs | Convert many scheduling invariants to interpolated strings. |
| src/Build/BackEnd/Components/Scheduler/Scheduler.cs | Convert trace formatting to handler-based tracing; precompute trace/state file paths. |
| src/Build/BackEnd/Components/Scheduler/SchedulableRequest.cs | Convert state/assertion messages to interpolated strings. |
| src/Build/BackEnd/Components/RequestBuilder/TaskHost.cs | Convert VerifyThrow messages to interpolated strings. |
| src/Build/BackEnd/Components/RequestBuilder/TaskBuilder.cs | Convert component-type assertion message to interpolated string. |
| src/Build/BackEnd/Components/RequestBuilder/TargetUpToDateChecker.cs | Convert dictionary invariant message to interpolated string. |
| src/Build/BackEnd/Components/RequestBuilder/TargetEntry.cs | Convert execution/state messages to interpolated strings. |
| src/Build/BackEnd/Components/RequestBuilder/TargetBuilder.cs | Convert several internal error/verify messages to interpolated strings. |
| src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs | Convert state messages to interpolated strings. |
| src/Build/BackEnd/Components/RequestBuilder/Lookup.cs | Convert scope mismatch message to interpolated string. |
| src/Build/BackEnd/Components/RequestBuilder/IntrinsicTasks/MSBuild.cs | Convert internal error/verify messages to interpolated strings. |
| src/Build/BackEnd/Components/RequestBuilder/IntrinsicTasks/IntrinsicTaskFactory.cs | Convert internal error messages to interpolated strings. |
| src/Build/BackEnd/Components/RequestBuilder/IntrinsicTask.cs | Convert internal error message to interpolated string. |
| src/Build/BackEnd/Components/ProjectCache/ProjectCacheService.cs | Convert plugin assertions / unknown plugin errors to interpolated strings. |
| src/Build/BackEnd/Components/Logging/LoggingServiceLogMethods.cs | Convert mapping invariant messages to interpolated strings. |
| src/Build/BackEnd/Components/Logging/LoggingServiceFactory.cs | Convert component-type assertion message to interpolated string. |
| src/Build/BackEnd/Components/Logging/LoggingService.cs | Convert packet-type/internal mapping messages to interpolated strings. |
| src/Build/BackEnd/Components/Logging/LoggingContext.cs | Convert invalid-context internal error to interpolated string. |
| src/Build/BackEnd/Components/Communications/TranslatorExtensions.cs | Convert constructor invariant to interpolated string. |
| src/Build/BackEnd/Components/Communications/TaskHostNodeManager.cs | Convert component-type assertion message to interpolated string. |
| src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs | Convert host-context and factory messages to interpolated strings. |
| src/Build/BackEnd/Components/Communications/NodeProviderOutOfProc.cs | Convert max-node/invalid node messages to interpolated strings. |
| src/Build/BackEnd/Components/Communications/NodeProviderInProc.cs | Convert timeout/shutdown messages to interpolated strings. |
| src/Build/BackEnd/Components/Communications/NodeManager.cs | Convert missing-provider and component-type assertion to interpolated strings. |
| src/Build/BackEnd/Components/Communications/NodeEndpointInProc.cs | Convert status invariants and waitId message to interpolated strings. |
| src/Build/BackEnd/Components/Caching/ResultsCache.cs | Convert internal error and component-type assertion to interpolated strings. |
| src/Build/BackEnd/Components/Caching/RegisteredTaskObjectCache.cs | Convert component-type assertion message to interpolated string. |
| src/Build/BackEnd/Components/Caching/ConfigCache.cs | Convert internal error and component-type assertion to interpolated strings. |
| src/Build/BackEnd/Components/BuildRequestEngine/BuildRequestEntry.cs | Convert state/assertion messages to interpolated strings. |
| src/Build/BackEnd/Components/BuildRequestEngine/BuildRequestEngine.cs | Convert trace + invariants to interpolated strings; add handler-based tracing. |
| src/Build/BackEnd/Components/BuildComponentFactoryCollection.cs | Convert factory replacement/internal error messages to interpolated strings. |
| src/Build/BackEnd/Client/MSBuildClientPacketPump.cs | Convert incomplete read/waitId messages to interpolated strings. |
| src/Build/BackEnd/BuildManager/LegacyThreadingData.cs | Convert registration/assertion messages to interpolated strings. |
| src/Build/BackEnd/BuildManager/BuildSubmission.cs | Convert config mismatch internal error to interpolated string. |
| src/Build/BackEnd/BuildManager/BuildManager.cs | Convert several invariant/internal error messages to interpolated strings. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review Summary — Interpolated String Handler Refactoring
Good design overall. The IsTrueInterpolatedStringHandler for VerifyThrow is a genuine performance win — deferred formatting on hot paths. The TraceInterpolatedStringHandler in BuildRequestEngine is also well-designed with the engine instance-based isEnabled pattern.
Issues Found
🔴 Bug (1)
- Stray space in
{ _buildManagerState}(BuildManager.cs:2396) — produces double-spaced error message
🟡 Moderate (3)
- ~10 stale PERF comments across BuildManager, LoggingService, LoggingServiceLogMethods, NodePacketFactory, BuildRequestEntry — these warn about boxing with
VerifyThrowbut the newIsTrueInterpolatedStringHandlereliminates that concern. Comments are now misleading and should be removed or the code should be migrated to the newVerifyThrowpattern. BUILDINGAPPXTASKSguard removal inShared/ErrorUtilities.cs— needs confirmation that no external project still compiles this file with that flag.ResourceUtilities.csin the same directory still uses the guards.- Tests weakened —
VerifyThrow1TruethroughVerifyThrow4Trueno longer exercise formatting behavior sincetruecondition prevents handler from formatting.
🟢 Minor (2)
- Inconsistent
.Box()— used in oneVerifyThrowcall (BuildRequestEngine.cs:449) but not in identical calls nearby. - Message content changes in CommandLineToolSwitch.cs — old
"InvalidType"with unused format arg now becomes"InvalidType: Boolean". Improvement, but intentional?
Design Notes
UnconditionalInterpolatedStringHandlerformats before method entry (no deferral) but allThrowInternalErrorcall sites are behind explicit guards, so this is acceptable.StringBuilderHelper.AppendFormatted<T>(T value)usesvalue?.ToString()on unconstrainedT— potential boxing in IL for value types, but only on error paths. Fine for current usage; flag if ever used on hot success paths.FrameworkErrorUtilitiesbecoming the canonical implementation withShared/ErrorUtilitiesdelegating to it is a clean architectural improvement.
Note
🔒 Integrity filter blocked 2 items
The following items were blocked because they don't meet the GitHub integrity level.
- #13556
pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved". - #13556
pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | noneGenerated by Expert Code Review (on open) for issue #13556 · ● 15.8M
Now that boxing is no longer a concern when calling ErrorUtilities.VerifyThrow, update several call sites to use it.
This helper existed to restrict boxing of BuildRequestEngineStatus enum values when formatting strings. This is no longer a concern and this helper can be removed.
Add coverage for interpolated string handler overloads.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There are several tracing and error methods that provide overloads for passing format strings and arguments. All of these allocate extra memory either by boxing args or creating params arrays. This can be avoided by using custom interpolated string handlers and passing in interpolated strings at the call sites.