Send task-host build process environment as delta#14126
Conversation
In -mt mode the task environment driver returns its backing dictionary by reference. Feeding that same instance back into SetEnvironment cleared it (Clear() followed by copy-from-self), wiping the environment. Guard the replace with a reference-equality check. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The build process environment is invariant for most tasks but was re-sent in full in every TaskHostConfiguration and echoed back in every TaskHostTaskComplete. Under negotiated packet version 5 it is sent in full once per connection; subsequent unchanged packets carry a one-byte "identical" marker and the receiver reconstructs it from the connection baseline, on both the forward (config) and return (complete) paths. Older peers keep the legacy full-dictionary format. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
On solution builds CurrentSolutionConfigurationContents (a large, build-invariant XML blob) was serialized into every TaskHostConfiguration's global properties. Carry it in its own wire field, excluded from the dictionary, and send it once per connection with the same full/identical delta as the environment. A task can still read it at runtime via IBuildEngine6.GetGlobalProperties(). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When a task host runs consecutive tasks whose environment is unchanged and the previous task did not mutate it, skip re-applying and restoring the process environment. Reset the cache at build boundaries and whenever a task blocks on a callback, so nested activity always forces a fresh apply. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
c80f9f2 to
abd77c4
Compare
There was a problem hiding this comment.
Pull request overview
Reduces out-of-proc task host IPC volume in -mt builds by introducing per-connection “full vs identical” transfer modes for build-invariant payloads (build process environment + CurrentSolutionConfigurationContents), and by avoiding redundant environment apply/restore work inside the task host.
Changes:
- Introduces packet-version-gated delta/identical wire forms for the task-host build process environment (forward + return paths) and for solution configuration contents (forward path).
- Adds
TranslateDictionaryExcludingKeyssupport to keep large invariant global properties out of the per-task global-properties dictionary payload. - Skips redundant per-task environment apply/restore in the task host when the next task’s environment is unchanged, and adds regression/unit tests covering the new wire format and aliasing edge cases.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Shared/TaskHostTaskComplete.cs | Adds environment “full vs identical” wire mode for the task completion packet. |
| src/Shared/TaskHostConfiguration.cs | Adds environment + solution-config delta fields/modes and excludes CurrentSolutionConfigurationContents from global-properties dictionary on the wire. |
| src/Shared/NodeEndpointOutOfProcBase.cs | Ensures outgoing packets use the negotiated (min) packet version when serializing. |
| src/MSBuild/OutOfProcTaskHostNode.cs | Reconstructs omitted invariants from per-connection baselines; skips redundant environment apply/restore between tasks. |
| src/Framework/MultiThreadedTaskEnvironmentDriver.cs | Guards against self-clearing when SetEnvironment is called with the driver’s own backing dictionary. |
| src/Framework/ITranslator.cs | Adds TranslateDictionaryExcludingKeys to the translator interface. |
| src/Framework/BinaryTranslator.cs | Implements TranslateDictionaryExcludingKeys in binary translators (write-side omits excluded keys). |
| src/Framework/BackEnd/NodePacketTypeExtensions.cs | Bumps packet version to 5 and documents the new delta capability. |
| src/Framework/BackEnd/CommunicationsUtilities.cs | Adds AreEnvironmentsEquivalent helper used to decide whether to send/apply/restore environment. |
| src/Build/Instance/TaskFactories/TaskHostTask.cs | Reconstructs “identical” returned environment from the sent config instead of re-receiving it. |
| src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs | Tracks per-connection baselines and marks outgoing configs as “identical” when invariants match. |
| src/Build.UnitTests/BackEnd/TranslationHelpers.cs | Adds helper to measure last serialization size for wire-size assertions. |
| src/Build.UnitTests/BackEnd/TaskHostTaskComplete_Tests.cs | Adds tests for v5 environment full/identical and legacy fallback behavior. |
| src/Build.UnitTests/BackEnd/TaskHostFactory_Tests.cs | Adds end-to-end regression tests for env reuse and env delta correctness in -mt. |
| src/Build.UnitTests/BackEnd/TaskHostConfiguration_Tests.cs | Adds tests for v5 environment delta and solution-config dedupe + legacy fallback. |
| src/Build.UnitTests/BackEnd/TaskEnvironment_Tests.cs | Adds regression test for the aliased-dictionary self-clear scenario. |
| src/Build.UnitTests/BackEnd/SetEnvironmentVariableTask.cs | Adds a test task that mutates environment inside the (possibly OOP) task process. |
| src/Build.UnitTests/BackEnd/ReadEnvironmentVariableTask.cs | Adds a test task that reads environment + reports PID for OOP validation. |
| src/Build.UnitTests/BackEnd/BinaryTranslator_Tests.cs | Adds unit tests proving excluded keys are omitted from the wire and round-trip behavior is correct. |
JanProvaznik
left a comment
There was a problem hiding this comment.
I think the Global properties should be deduplicated for the general case rather than special casing the one found in the solution build
… delta Deduplicate the (largely invariant) global properties on the out-of-proc task-host wire: send them in full once per connection, then a 1-byte ''identical'' marker when unchanged, and reconstruct them on the task host from the per-connection baseline. Gated on the negotiated packet version so legacy/net35 peers keep the full-dictionary wire format. Includes an end-to-end -mt regression test (TaskHostObservesGlobalPropertyAcrossConsecutiveTasks) and a generic AreDictionariesEquivalent helper shared with the environment delta path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Manual end-to-end tests passed: Scenario: a .NET Framework
1. Pin the sample to released 10.0.300
2. Set the host environment$env:DOTNET_HOST_PATH = "C:\Program Files\dotnet\dotnet.exe"
$env:DOTNET_ROOT = "C:\Program Files\dotnet"3. Run the sample with the Framework MSBuild.exe
$proj = ".\artifacts\bin\Microsoft.Build.Engine.UnitTests\Debug\net10.0\TestAssets\ExampleNetTask\TestNetTask\TestNetTask.csproj"
$asmLoc = ".\artifacts\bin\Microsoft.Build.Engine.UnitTests\Debug\net10.0"
& ".\artifacts\bin\bootstrap\net472\MSBuild\Current\Bin\MSBuild.exe" $proj `
-restore -v:n -nologo `
"-p:LatestDotNetCoreForMSBuild=net10.0" "-p:AssemblyLocation=$asmLoc"Result:
|
|
since this is a node communication change with nontrivial blast radius, I'd prefer a second positive review from @AR-May or @rainersigwald |
There was a problem hiding this comment.
Overall, I like the changes, this is a great PR. I have a couple of minor comments and one discussion point.
Since this change is risky, I'd also like to do an experimental insertion for it. As part of that insertion, I'd like to force the tasks to run in the task host to get additional test coverage. Assuming the tests pass, I'm approving this PR.
| // When the negotiated wire format supports it, send the (invariant) build process | ||
| // environment and global properties only when they changed; otherwise mark them | ||
| // as unchanged on the wire. | ||
| if (packet is TaskHostConfiguration taskHostConfiguration && writeTranslator.NegotiatedPacketVersion >= NodePacketTypeExtensions.EnvironmentDeltaMinVersion) |
There was a problem hiding this comment.
Placing the Full/Identical decision here in DrainPacketQueue has a bit of a layering smell for me. This loop is generic transport "write any packet's bytes to the pipe" but now it ends up knowing that a specific packet's payload can be delta-encoded.
I don't think it has to live here. The baseline is really just per-connection state, and the decision only needs to (a) use a per-connection baseline - so we do not mix different types of the task host. We need to be extra careful with that. And (b) happen before the config is serialized.
From another point of view, with this code here, we are extra sure we did not mix up baselines for the connection.
Option I'd consider: Compute the mode where the config is built/enqueued (e.g. in TaskHostTask ), backed by a per-connection baseline held in the provider keyed by TaskHostNodeKey . This puts the payload knowledge in the layer that owns the config, and keeps the drain loop pure transport.
@JanProvaznik what do you think?
There was a problem hiding this comment.
I agree on smell, but honestly I'm a little afraid to move it outside DrainPacketQueue. As you mentioned placing it here guarantees that we will not mix up baselines and it we can have it here without any locking
Fixes #14097
Context
Out-of-proc task hosts re-send the same build-invariant data in every
TaskHostConfiguration(and echo the environment back in everyTaskHostTaskComplete). Two payloads dominate this redundancy on large-mtbuilds:CurrentSolutionConfigurationContentsglobal property — an XML blob describing every project's configuration (~57 KB on OrchardCore) — shipped forward in every config. It is ~99% of the global-properties payload and ~48% of all task-host config bytes.Both are invariant for the vast majority of tasks, yet re-transmitted in full to connections that already received them.
Changes Made
Send the environment as a delta. Under a negotiated wire format (PacketVersion 5) the environment is sent in full only once per task-host connection; subsequent packets whose environment is unchanged carry a 1-byte "identical" marker instead of the full dictionary, on both the forward (config) and return (result) paths. The receiver reconstructs it from the last full environment seen on that connection.
Deduplicate the solution configuration blob. Solution (
.sln/.slnx) builds injectCurrentSolutionConfigurationContentsas a global property on every project. It is build-invariant (it describes the solution, not the project), but it is re-serialized into everyTaskHostConfiguration. It cannot simply be dropped — a task may read it at runtime viaIBuildEngine6.GetGlobalProperties()— so the same per-connection delta is applied (PacketVersion 65: the value is carried in its own field, excluded from the global-properties dictionary on the wire (so the large blob is never serialized twice), preceded by a 1-byte Full/Identical marker. It is sent once per connection, then only a marker; the task host reconstructs it from the connection baseline.Skip redundant per-task environment apply/restore in the task host when the next task's environment is unchanged from the previous one.
Bug fixes:
SetEnvironmentcleared it, because in-mtit was the driver's own backing dictionary. Fixed with a reference-equality guard.Testing
CurrentSolutionConfigurationContentsat PacketVersion 5-mtend-to-end regression tests: environment observed across consecutive task-host tasks, and an environment change made by one task-host task is observed by the next (verified failing before the aliasing fix, passing after).-mtRebuild succeeds.Measurements
Workload: OrchardCore (
OrchardCore.slnx),-t:Rebuild -mt— 17,975 task-host task invocations. Serialized payload sizes measured with ThProfile (write-side stream-position deltas; deterministic, single run). Baseline = upstreammain; patched = this PR.CurrentSolutionConfigurationContents(forward)Per direction:
TaskHostConfigurationtotal (forward)CurrentSolutionConfigurationContentsTaskHostTaskCompletetotal (return)The environment was ~4.6% of every config packet and ~29.7% of every result packet;
CurrentSolutionConfigurationContentswas ~48% of every config packet. After the change each carries only a 1-byte "unchanged" marker once the connection baseline is established. The remaining config payload is dominated by per-task task parameters, which are not invariant and so are not deduplicated.Wall-clock / overhead
Sending the environment as a delta, deduplicating the solution configuration, and skipping the redundant per-task environment apply/restore did not measurably change total wall-clock time or task-host overhead in my measurements — the IPC savings are below the run-to-run noise floor on my machine. This is an IPC-volume optimization (fewer bytes serialized/transmitted/deserialized), not a critical-path one.
I measured TaskHost overhead on OrchardCore after restore on my machine three times and took the average, using this guide to compute task host overhead. (Note: in this guide all
MSBuildMultiThreadableTaskparameters were deliberately deleted; I kept them, of course.)The measurement showed no significant improvement on total time for task and overhead:


Baseline:
Patched version:
Both deltas are likely noise (the two metrics point in opposite directions, and
execute_ms— task work this change cannot affect — moved by a similar magnitude, indicating machine-load drift rather than a real effect).