Fix telemetry allocation regression: per-engine collector ownership by JanProvaznik · Pull Request #13516 · dotnet/msbuild

JanProvaznik · 2026-04-09T14:34:50Z

Context

PR #13413 fixed thread-safety in telemetry by creating a local WorkerNodeTelemetryData (with 2 Dictionary<> instances) per build request, populating it with all targets/tasks, then merging under a lock into a shared singleton. This introduced a ~200MB allocation regression on large solutions, affecting both MT and non-MT modes (the pattern was applied unconditionally).

Architecture: before, after #13413, and now

Before #13413 (broken in MT mode):

TelemetryForwarderProvider  [Singleton, shared across all engines]
  +-- TelemetryForwarder
        +-- WorkerNodeTelemetryData  [one shared dict, NO locking]

RequestBuilder1 --AddTarget/AddTask--> shared dict  <-- RACE
RequestBuilder2 --AddTarget/AddTask--> shared dict  <-- RACE

Engine1.FinalizeProcessing() -> sends ALL data
Engine2.FinalizeProcessing() -> sends ALL data again  <-- Nx DUPLICATION

After #13413 (correct but 200MB regression):

TelemetryForwarderProvider  [Singleton, shared across all engines]
  +-- TelemetryForwarder
        +-- WorkerNodeTelemetryData  [shared dict, with lock]
        +-- FinalizeProcessing: swap-and-send (fixes duplication)

RequestBuilder (per request):
  new WorkerNodeTelemetryData()  <-- 2 fresh Dictionary<> per request
  populate with ~200 targets, ~60 tasks (6-7 resize steps each)
  forwarder.MergeWorkerData(localData)  <-- merge under lock, discard local

  ~30KB alloc churn x ~1000 requests/node x N nodes = ~200MB

This PR (correct, zero per-request allocations):

TelemetryForwarderProvider  [Singleton, but just a factory now]
  +-- CreateForwarder() -> new TelemetryForwarder per engine

BuildRequestEngine1                    BuildRequestEngine2
  +-- TelemetryForwarder1               +-- TelemetryForwarder2
        +-- data (no lock needed)             +-- data (no lock needed)

RequestBuilder --AddTarget/AddTask--> own engine forwarder
                                      NO local dicts, NO singleton contention

Engine1.FinalizeProcessing() -> sends only Engine1 data
Engine2.FinalizeProcessing() -> sends only Engine2 data
InternalTelemetryConsumingLogger merges all events (unchanged)

No locking is needed because the engine one-active-builder-at-a-time invariant guarantees single-threaded access (see #13531 for hardening the yield/reacquire protocol this depends on).

Measured regression (OrchardCore, 233 projects)

Metric	Value
Avg targets/project	~199 (range 112-593)
Build requests/node	~1000 (inner builds, metaprojects)
Per-request alloc overhead	~30 KB (Entry[], Int32[], TaskExecutionStats)
Per-node regression	~30 MB
x multiple nodes	~200 MB total

Commit structure

Functional change - move telemetry ownership to per-engine (keeps old Forwarder names)
Pure rename - TelemetryForwarder -> TelemetryCollector (no behavioral changes)

Testing

All 32 telemetry unit tests pass
Full build succeeds with 0 warnings, 0 errors
Red-teamed from 5 perspectives (thread safety, data completeness, duplication, node lifecycle, consumer compatibility)

Copilot

Pull request overview

Refactors worker-node telemetry collection to be owned per BuildRequestEngine (per-engine collectors) to eliminate the large per-request allocation churn introduced by the prior thread-safety fix, while preserving correctness in /m /mt scenarios.

Changes:

Replace the singleton TelemetryForwarderProvider/ITelemetryForwarder model with a singleton factory that creates per-engine ITelemetryCollector instances.
Wire the per-engine collector through NodeLoggingContext to RequestBuilder, removing per-request local WorkerNodeTelemetryData allocations and merge steps.
Update component registration (BuildComponentType), project file includes, and telemetry unit tests/mocks accordingly.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
src/Build/TelemetryInfra/TelemetryForwarderProvider.cs	Removes old singleton forwarder/provider implementation.
src/Build/TelemetryInfra/TelemetryCollectorProvider.cs	Adds singleton provider that creates per-engine telemetry collectors and sends events on finalize.
src/Build/TelemetryInfra/ITelemetryForwarder.cs	Removes old forwarder interface.
src/Build/TelemetryInfra/ITelemetryCollector.cs	Adds new per-engine collector interface used by request builders and engine shutdown.
src/Build/Microsoft.Build.csproj	Updates compilation includes to new telemetry files.
src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs	Sends target/task telemetry directly to the engine’s collector (no per-request local dictionaries).
src/Build/BackEnd/Components/Logging/NodeLoggingContext.cs	Stores the per-engine `ITelemetryCollector` reference for builders to use.
src/Build/BackEnd/Components/IBuildComponentHost.cs	Renames component enum value from `TelemetryForwarder` to `TelemetryCollector`.
src/Build/BackEnd/Components/BuildRequestEngine/BuildRequestEngine.cs	Creates a per-engine collector at build start and finalizes it at build cleanup.
src/Build/BackEnd/Components/BuildComponentFactoryCollection.cs	Registers the new telemetry collector provider component.
src/Build.UnitTests/Telemetry/Telemetry_Tests.cs	Updates telemetry test to use collector API (`AddTarget`/`FinalizeProcessing`).
src/Build.UnitTests/BackEnd/MockHost.cs	Updates mock host wiring to provide the new telemetry collector provider.

JanProvaznik · 2026-04-10T10:42:53Z

/review

github-actions · 2026-04-10T10:43:40Z

✅ Expert Code Review (command) completed successfully!

github-actions

Expert Review — PR #13516: Fix telemetry allocation regression

24-dimension review complete. Clean internal refactor — well-structured with good commit separation.

#	Dimension	Verdict
1	Backwards Compatibility	✅ LGTM
2	ChangeWave Discipline	✅ LGTM
3	Performance & Allocation Awareness	✅ LGTM
4	Test Coverage & Completeness	✅ LGTM
5	Error Message Quality	✅ LGTM
6	Logging & Diagnostics Rigor	✅ LGTM
7	String Comparison Correctness	✅ LGTM
8	API Surface Discipline	✅ LGTM
9	Target Authoring Conventions	✅ LGTM
10	Design Before Implementation	✅ LGTM
11	Cross-Platform Correctness	✅ LGTM
12	Code Simplification	⚪ 2 NITs
13	Concurrency & Thread Safety	✅ LGTM
14	Naming Precision	✅ LGTM
15	SDK Integration Boundaries	✅ LGTM
16	Idiomatic C# Patterns	✅ LGTM
17	File I/O & Path Handling	✅ LGTM
18	Documentation Accuracy	✅ LGTM
19	Build Infrastructure Care	✅ LGTM
20	Scope & PR Discipline	✅ LGTM
21	Evaluation Model Integrity	✅ LGTM
22	Correctness & Edge Cases	✅ LGTM
23	Dependency Management	✅ LGTM
24	Security Awareness	✅ LGTM

All dimensions clean or NIT-only

Key observations

Performance ✅ — Eliminates per-request WorkerNodeTelemetryData allocation and merge. Direct AddTarget/AddTask into the per-engine collector is strictly better.

Concurrency ✅ — Traced the full thread timeline: InitializeForBuild (engine thread) → ActionBlock dispatch → StartBuilderThread (barrier) → builder threads call AddTarget/AddTask under lock (_lock) → WaitForDeactivateCompletion joins all builders → FinalizeProcessing runs under lock. Clean happens-before chain throughout.

Correctness ✅ — Multiple WorkerNodeTelemetryEventArgs events (one per engine) instead of one merged event is handled correctly by InternalTelemetryConsumingLogger.EventSource5_WorkerNodeTelemetryLogged, which calls _workerNodeTelemetryData.Add() to merge each event additively.

Backwards Compatibility ✅ — All types are internal. The old public nested classes were effectively internal (bounded by internal containing class). No public API changes, no output format changes.

NITs — Two inline comments for stale "forwarder" naming left over from the rename.

Generated by Expert Code Review (command) for issue #13516 · ● 35.6M

Eliminates ~200MB allocation regression from PR dotnet#13413 by removing per-request WorkerNodeTelemetryData dictionary allocations entirely. Each BuildRequestEngine creates its own ITelemetryForwarder (via TelemetryForwarderProvider.CreateForwarder) and passes it to builders through NodeLoggingContext: - TelemetryForwarder owns WorkerNodeTelemetryData (no locking needed - the engine's one-active-builder-at-a-time invariant guarantees single-threaded access, see dotnet#13531) - RequestBuilder calls AddTarget/AddTask directly - BuildRequestEngine calls FinalizeProcessing at CleanupForBuild - TelemetryForwarderProvider is the factory, not a data-holding singleton Co-authored-by: Copilot <[email protected]>

Pure rename - no behavioral changes. Co-authored-by: Copilot <[email protected]>

Co-authored-by: Copilot <[email protected]>

Updated [Microsoft.Build](https://github.com/dotnet/msbuild) from 18.6.3 to 18.7.1. <details> <summary>Release notes</summary> _Sourced from [Microsoft.Build's releases](https://github.com/dotnet/msbuild/releases)._ ## 18.7.1 ## What's Changed * Fix TraceEngine file contention deadlock in multithreaded mode by @JanProvaznik in dotnet/msbuild#13446 * Remove duplicate test cases in MultithreadableTaskAnalyzer by @Youssef1313 in dotnet/msbuild#13483 * Ensure ThreadSafeTaskAnalyzer.Tests is considered as a unit test project by @Youssef1313 in dotnet/msbuild#13481 * Fix MSBuildTask0002 analyzer warnings in already-migrated tasks by @JanProvaznik in dotnet/msbuild#13466 * Fix race conditions in task host path resolution by @AR-May in dotnet/msbuild#13485 * Migrate ToolTask and Al task to TaskEnvironment API by @OvesN in dotnet/msbuild#13423 * Bump main to 18.7, add vs18.6 to merge flow by @MichalPavlik in dotnet/msbuild#13472 * Avoid allocations in GetHashCode implementations by @DustinCampbell in dotnet/msbuild#13475 * Add PATs rotation to agentic workflow(s) by @JanKrivanek in dotnet/msbuild#13496 * Fix ASP.NET WebSite projects to copy netstandard.dll facade when required by @JanProvaznik in dotnet/msbuild#13058 * Migrate AspNetCompiler to TaskEnvironment API by @OvesN in dotnet/msbuild#13424 * Add review workflow by @JanKrivanek in dotnet/msbuild#13503 * Strengthen reviewer skill: add step-back analysis dimensions by @JanProvaznik in dotnet/msbuild#13504 * Add 'Request Speedometer Perf Run' to VS experimental insertion build policies by @Copilot in dotnet/msbuild#13505 * Remove duplicate @ prefix from issueAuthor in GitOps by @akoeplinger in dotnet/msbuild#13492 * Improve review aw by @JanKrivanek in dotnet/msbuild#13510 * Migrates unit tests to use RoslynCodeTaskFactory to enable running tests under .NET Core by @jankratochvilcz in dotnet/msbuild#13500 * Fix cross-AppDomain TaskItem modifier cache regression by @DustinCampbell in dotnet/msbuild#13493 * Discourage review agent from approving PRs by @JanKrivanek in dotnet/msbuild#13512 * Stop trying to deploy ValueTuple by @rainersigwald in dotnet/msbuild#13507 * Ad-hoc re-sign bootstrap dotnet on macOS to prevent SIGKILL by @jankratochvilcz in dotnet/msbuild#13513 * RoslynCodeTaskFactory: Log MSB3753 when task class does not implement ITask by @jankratochvilcz in dotnet/msbuild#13517 * Update gh-aw (upon mcp policy changes) by @JanKrivanek in dotnet/msbuild#13526 * Eliminate XmlChildNodes allocations in GetXmlNodeInnerContents by @nareshjo in dotnet/msbuild#13509 * Fix telemetry allocation regression: per-engine collector ownership by @JanProvaznik in dotnet/msbuild#13516 * Migrate to xunit.v3 by @Youssef1313 in dotnet/msbuild#13482 * Fix stray brace in HandleBuildCancel trace string causing MSB1025 by @Copilot in dotnet/msbuild#13535 * Bumping to 10.0.4 runtime packages by @MichalPavlik in dotnet/msbuild#13533 * Remove early return in GetCanonicalForm, always call System.IO.Path by @OvesN in dotnet/msbuild#13532 * Do not overwrite GetCopyToOutputDirectoryItemsDependsOn, just add new… by @snechaev in dotnet/msbuild#13474 * Migrate GetReferenceAssemblyPaths task to TaskEnvironment API by @OvesN in dotnet/msbuild#13495 * Stabilize ToolTaskThatTimeoutAndRetry test by @rainersigwald in dotnet/msbuild#13489 * [automated] Merge branch 'vs18.6' => 'main' by @github-actions[bot] in dotnet/msbuild#13506 * Add extra test assertions around tests by @Youssef1313 in dotnet/msbuild#13536 * Add static eval for repo skills/agents via skill-validator by @JanKrivanek in dotnet/msbuild#13537 * Migrate SGen task to Task environment API by @OvesN in dotnet/msbuild#13457 * Fix TerminalLogger assert failure for metaproj files and cached project eval ID by @OvesN in dotnet/msbuild#13480 * Filter out approving review from pr-reviewer agent by @JanKrivanek in dotnet/msbuild#13553 * Use a unique task name per invocation to tabilize RoslynCodeTaskFactory_ReuseCompilation test by @huulinhnguyen-dev in dotnet/msbuild#13551 * Brief doc on feedback/logging/data systems by @rainersigwald in dotnet/msbuild#13554 * Localized file check-in by OneLocBuild Task: Build definition ID 9434: Build ID 13881982 by @dotnet-bot in dotnet/msbuild#13437 * Stage 3: Forward BuildProjectFile* callbacks from OOP TaskHost to worker node by @JanProvaznik in dotnet/msbuild#13350 * Enable TaskHost Callbacks by default by @JanProvaznik in dotnet/msbuild#13579 * Remove unactionable info from reviewer agent by @JanKrivanek in dotnet/msbuild#13578 * Enlighten RequiresFramework35SP1Assembly task for multithreaded mode by @jankratochvilcz in dotnet/msbuild#13575 * Make SdkResolver-provided environment variables take precedence over ambient environment by @Copilot in dotnet/msbuild#12655 * Add dotnet/skills marketplace and enable plugins by @Evangelink in dotnet/msbuild#13582 * The skills/agents check filters-in only touched files by @JanKrivanek in dotnet/msbuild#13586 * Fix skill-validation workflow failing when agents directory is deleted by @JeremyKuhne in dotnet/msbuild#13592 ... (truncated) Commits viewable in [compare view](dotnet/msbuild@v18.6.3...v18.7.1). </details> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

JanProvaznik force-pushed the fix/telemetry-allocation-regression branch from 580cdf1 to a0f92c8 Compare April 10, 2026 08:45

JanProvaznik changed the title ~~Fix ~200MB allocation regression from per-request telemetry dictionaries~~ Fix ~200MB allocation regression: move telemetry to per-engine ownership Apr 10, 2026

JanProvaznik force-pushed the fix/telemetry-allocation-regression branch from a0f92c8 to 973953e Compare April 10, 2026 08:59

JanProvaznik changed the title ~~Fix ~200MB allocation regression: move telemetry to per-engine ownership~~ Fix ~200MB telemetry allocation regression: per-engine forwarder ownership Apr 10, 2026

JanProvaznik force-pushed the fix/telemetry-allocation-regression branch 3 times, most recently from 914c257 to 80584a9 Compare April 10, 2026 09:42

JanProvaznik marked this pull request as ready for review April 10, 2026 10:28

Copilot AI review requested due to automatic review settings April 10, 2026 10:28

Copilot started reviewing on behalf of JanProvaznik April 10, 2026 10:28 View session

Copilot AI reviewed Apr 10, 2026

View reviewed changes

JanProvaznik force-pushed the fix/telemetry-allocation-regression branch from 80584a9 to 2e98fa6 Compare April 10, 2026 10:40

JanProvaznik requested a review from rainersigwald April 10, 2026 10:50

github-actions Bot reviewed Apr 10, 2026

View reviewed changes

Comment thread src/Build.UnitTests/Telemetry/Telemetry_Tests.cs Outdated

Comment thread src/Build/BackEnd/Components/BuildRequestEngine/BuildRequestEngine.cs Outdated

JanProvaznik force-pushed the fix/telemetry-allocation-regression branch from 2e98fa6 to 6d04e6b Compare April 10, 2026 11:27

JanProvaznik changed the title ~~Fix ~200MB telemetry allocation regression: per-engine forwarder ownership~~ Fix telemetry allocation regression: per-engine collector ownership Apr 10, 2026

JanProvaznik self-assigned this Apr 10, 2026

rainersigwald approved these changes Apr 10, 2026

View reviewed changes

Comment thread src/Build/TelemetryInfra/TelemetryCollectorProvider.cs Outdated

AR-May reviewed Apr 13, 2026

View reviewed changes

Comment thread src/Build/TelemetryInfra/TelemetryCollectorProvider.cs Outdated

This was referenced Apr 13, 2026

Add information about binlog to stdout #12638

Closed

Rewrite resolveprojectreferences MSBuild calls to reduce the amount of batching done #12631

Closed

separate the tracking parts of TL into a reusable base, so that TL becomes a 'renderer' #12587

Closed

JanProvaznik force-pushed the fix/telemetry-allocation-regression branch from 44ee0b5 to ce5fd68 Compare April 13, 2026 12:04

JanProvaznik and others added 2 commits April 13, 2026 14:24

Rename TelemetryForwarder to TelemetryCollector

b134b5c

Pure rename - no behavioral changes. Co-authored-by: Copilot <[email protected]>

JanProvaznik force-pushed the fix/telemetry-allocation-regression branch from a25cd82 to b134b5c Compare April 13, 2026 12:30

AR-May approved these changes Apr 13, 2026

View reviewed changes

Update doc comments to reference BuildRequestEngine explicitly

584611d

Co-authored-by: Copilot <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix telemetry allocation regression: per-engine collector ownership#13516

Fix telemetry allocation regression: per-engine collector ownership#13516
JanProvaznik merged 4 commits into
dotnet:mainfrom
JanProvaznik:fix/telemetry-allocation-regression

JanProvaznik commented Apr 9, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JanProvaznik commented Apr 10, 2026

Uh oh!

github-actions Bot commented Apr 10, 2026 •

edited

Loading

Uh oh!

github-actions Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

JanProvaznik commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Architecture: before, after #13413, and now

Measured regression (OrchardCore, 233 projects)

Commit structure

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JanProvaznik commented Apr 10, 2026

Uh oh!

github-actions Bot commented Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Expert Review — PR #13516: Fix telemetry allocation regression

Key observations

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JanProvaznik commented Apr 9, 2026 •

edited

Loading

github-actions Bot commented Apr 10, 2026 •

edited

Loading