Allow NodeProviderOutOfProcTaskHost to manage multiple nodes instead of one per arch by surayya-MS · Pull Request #12577 · dotnet/msbuild

surayya-MS · 2025-09-29T13:13:02Z

Context

Currently, for multithreaded node we cannot force all tasks to run in task hosts by setting MSBUILDFORCEALLTASKSOUTOFPROC=1. It crashes. The problem is that NodeProviderOutOfProcTaskHost is one per process, and manages up to 4 nodes for specific architecture.

Changes Made

Propagated nodeId if the in-proc nodes (only when multi-threaded mode is on) into NodeProviderOutOfProcTaskHost
Used nodeId with the combination of (int)HanshakeOptions (is originally used as key for _nodeContexts), to generate a unique id for task host node

Testing

Manually tested by building this project that references other projects with Message tasks. Modified Message task to show the process id it is running on.
msbuild.exe .\MainProject.proj - m with combination of with/without -mt switch and with/without setting MSBUILDFORCEALLTASKSOUTOFPROC.

Notes

…it for combined key for taskHostNodeId

This reverts commit ce75443.

Copilot

Pull Request Overview

This PR modifies the NodeProviderOutOfProcTaskHost to manage multiple task host nodes instead of being limited to one node per architecture. The change enables better support for multi-threaded builds where multiple in-process nodes need to distinguish their task hosts.

Introduces a new task host node ID generation system that combines scheduled node ID with architecture/runtime information
Updates the NodeProviderOutOfProcTaskHost to use ConcurrentDictionary and support unlimited nodes
Threads the scheduled node ID through the task execution pipeline from BuildRequest to TaskHostTask

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
TaskHostTask.cs	Adds scheduledNodeId parameter and GenerateTaskHostNodeId method for unique task host identification
AssemblyTaskFactory.cs	Passes scheduledNodeId parameter when creating TaskHostTask instances
TaskExecutionHost.cs	Threads scheduledNodeId through task initialization and instantiation
BuildRequest.cs	Adds ScheduledNodeId property to track which node a request is assigned to
InProcNode.cs	Sets ScheduledNodeId on build requests and stores node ID
TaskBuilder.cs	Passes scheduledNodeId from build request to task execution host
NodeProviderOutOfProcTaskHost.cs	Replaces HandshakeOptions-based mapping with concurrent node ID mapping
NodeProviderInProc.cs	Passes nodeId when creating InProcNode instances
NodeManager.cs	Extracts multi-threaded mode check to local variable
TaskExecutionHost_Tests.cs	Updates test calls to include scheduledNodeId parameter
AssemblyTaskFactory_Tests.cs	Updates test calls to include scheduledNodeId parameter

src/Build/Instance/TaskFactories/TaskHostTask.cs

src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs

JanProvaznik

I think this is well executed change, please address/discuss comments. Then lgtm.

src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs

src/Build/Instance/TaskFactories/TaskHostTask.cs

AR-May

Looks overall good for me. I wonder about 2 things:

Can we hide using the task node id instead of handshake casted to int behind a change wave
Can we make a test that would catch the initial bug - that task host for different nodes should be different?

src/Build/BackEnd/Shared/BuildRequest.cs

src/Build/Instance/TaskFactories/TaskHostTask.cs

src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs

surayya-MS · 2025-10-01T13:40:39Z

Can we hide using the task node id instead of handshake casted to int behind a change wave

@AR-May I can make it so that task host node id is only used for multithreaded mode.

…r _nodeContexts only in case of multi-threaded mode

…/surayya-MS/msbuild into out-of-proc-task-host-second-try

surayya-MS · 2025-10-02T15:50:09Z

Can we make a test that would catch the initial bug - that task host for different nodes should be different?

@AR-May I'll try to do that in the follow-up PR.
For now, the testing was manual. Tested on combination of - with/without -mt and with/without setting MSBUILDFORCEALLTASKSOUTOFPROC. With MSBUILDFORCEALLTASKSOUTOFPROC=1 making sure that process ids are the same for the same project

AR-May · 2025-10-03T09:50:26Z

Can we make a test that would catch the initial bug - that task host for different nodes should be different?

@AR-May I'll try to do that in the follow-up PR. For now, the testing was manual. Tested on combination of - with/without -mt and with/without setting MSBUILDFORCEALLTASKSOUTOFPROC. With MSBUILDFORCEALLTASKSOUTOFPROC=1 making sure that process ids are the same for the same project

Ok, works for me!

src/MSBuild/Resources/Strings.resx

src/Build/Instance/TaskFactories/TaskHostTask.cs

src/Build/BackEnd/Shared/BuildRequest.cs

src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs

) ### Context There seem to be a merge bug introduced in #12577. The scheduled node id should be passed to the `TaskHostTask` in both instances of the task creation. ### Changes Made 1. Passed scheduled node id to the task host task constructor 2. Removed the default value in constructor to avoid such errors in future. ### Testing None

Fixes #12610 ### Context Follow-up to #12577. The taskhost node identification used bit-packing to combine `scheduledNodeId` (8 bits) and `HandshakeOptions` (9 bits) into a single int, limiting multithreaded builds to 256 nodes. This approach was fragile and hard to maintain. ### Changes Made - **New `TaskHostNodeKey` record struct** in `CommunicationsUtilities.cs` combining `HandshakeOptions` and `NodeId` as a clean, type-safe key - **Updated `NodeProviderOutOfProcTaskHost`** with optimized dictionary structure: - `_nodeContexts` keyed by `TaskHostNodeKey` for checking if a node with given (HandshakeOptions, scheduledNodeId) exists (acquisition logic) - `_nodeIdToNodeKey` for O(1) reverse lookup during node termination - `_nodeIdToPacketFactory` and `_nodeIdToPacketHandler` keyed by int for O(1) packet routing performance - `_activeNodes` tracks node IDs (int) for shutdown handling - Added atomic `_nextNodeId` counter for unique communication node ID generation - **Simplified `TaskHostTask`** - removed `GenerateTaskHostNodeId` method and associated bit-packing constants - **Improved error handling** - Added `VerifyThrow` assertions in `DisconnectFromHost` and `CreateNode` to verify context existence and prevent duplicate nodes ```csharp // Before: bit-packed int with 256-node limit _taskHostNodeId = GenerateTaskHostNodeId(_scheduledNodeId, _requiredContext); // (scheduledNodeId << 9) | ((int)handshakeOptions & 0x1FF) // After: clean record struct, no limit _taskHostNodeKey = new TaskHostNodeKey(_requiredContext, _scheduledNodeId); ``` This gives the best of both worlds: - **Maintainability**: Type-safe Keys for managing node lifecycle and acquisition - **Performance**: Direct Integer lookups for the high-frequency packet routing hot path ### Testing - Added 7 unit tests for `TaskHostNodeKey` (equality, hash codes, dictionary keys, large node IDs) - All 86 existing TaskHost tests pass ### Notes - Removes the `NODE_ID_MAX_VALUE_FOR_MULTITHREADED` (255) constraint entirely - `SendData(int, INodePacket)` throws `NotImplementedException` as it's required by `INodeProvider` interface but not used for task hosts (similar to `TaskHostNodeManager`)  <details> <summary>Original prompt</summary> > > ---- > > *This section details on the original issue you should resolve* > > <issue_title>[multithreaded] Refactor Node IDs for taskhost</issue_title> > <issue_description>followup to #12577 > Refactor src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs , HandshakeOptions to use record struct as a key to nodes to increase maintanability and remove limitation to 256 nodes.</issue_description> > > ## Comments on the Issue (you are @copilot in this section) > > <comments> > </comments> > </details> - Fixes #12610  --- 💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: JanProvaznik <25267098+JanProvaznik@users.noreply.github.com> Co-authored-by: Jan Provazník <janprovaznik@microsoft.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

surayya-MS and others added 4 commits September 29, 2025 15:09

print message task process id

ce75443

propagate in-proc node id into NodeProviderOutOfProcTaskHost and use …

55eb1d4

…it for combined key for taskHostNodeId

Revert "print message task process id"

d8dc7cd

This reverts commit ce75443.

Merge branch 'main' into out-of-proc-task-host-second-try

1a99dee

surayya-MS mentioned this pull request Sep 29, 2025

Allow NodeProviderOutOfProcTaskHost to manage multiple nodes instead of one per arch #12521

Closed

small refactorings

0c64714

surayya-MS marked this pull request as ready for review September 30, 2025 14:15

surayya-MS requested review from AR-May, JanProvaznik and Copilot September 30, 2025 14:15

Copilot AI reviewed Sep 30, 2025

View reviewed changes

src/Build/Instance/TaskFactories/TaskHostTask.cs Outdated Show resolved Hide resolved

src/Build/Instance/TaskFactories/TaskHostTask.cs Show resolved Hide resolved

src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs Outdated Show resolved Hide resolved

JanProvaznik approved these changes Sep 30, 2025

View reviewed changes

AR-May approved these changes Oct 1, 2025

View reviewed changes

surayya-MS mentioned this pull request Oct 1, 2025

Design multi-threaded build for Visual Studio scenarios #11939

Open

surayya-MS and others added 6 commits October 1, 2025 15:43

use generated taskHostId (from nodeId and handshakeOptions) as key fo…

90bc6c7

…r _nodeContexts only in case of multi-threaded mode

address comments

34e1c3a

Merge branch 'main' into out-of-proc-task-host-second-try

42440e1

remove AvailableNodes check

9f4d3dc

Merge branch 'out-of-proc-task-host-second-try' of https://github.com…

03f66d6

…/surayya-MS/msbuild into out-of-proc-task-host-second-try

Merge branch 'main' into out-of-proc-task-host-second-try

e451149

Merge branch 'main' into out-of-proc-task-host-second-try

94083c3

JanProvaznik and others added 4 commits October 3, 2025 12:14

Merge branch 'main' into out-of-proc-task-host-second-try

3bf932a

fix

5fca3f8

comment in HandshakeOptions

086b537

add recommendation on -mt switch about max 256 threads

41d8885

surayya-MS requested a review from rainersigwald October 3, 2025 11:05

JanProvaznik reviewed Oct 3, 2025

View reviewed changes

surayya-MS and others added 8 commits October 6, 2025 14:39

use named argument scheduledNodeId: 1 in tests

f6c31c8

avoid double lookup

006520a

use better syntax for list init

60322a8

Merge branch 'main' into out-of-proc-task-host-second-try

6e21988

use named argument in TaskExecutionHost_Tests

089a26a

change mt help message to be less than 80 chars long per line

a67d5dc

remaining localized strings

fbdc267

remove assignig default value to _taskHostNodeId

e1813b3

AR-May approved these changes Oct 6, 2025

View reviewed changes

rainersigwald approved these changes Oct 6, 2025

View reviewed changes

src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs Show resolved Hide resolved

JanProvaznik mentioned this pull request Oct 6, 2025

[multithreaded] Refactor Node IDs for taskhost #12610

Closed

help message formatting

71770a5

JanProvaznik enabled auto-merge (squash) October 6, 2025 16:11

surayya-MS disabled auto-merge October 7, 2025 06:49

surayya-MS enabled auto-merge (squash) October 7, 2025 06:50

surayya-MS merged commit e328bd0 into dotnet:main Oct 7, 2025
9 checks passed

surayya-MS mentioned this pull request Oct 7, 2025

[thread-safety] - TypeLoader, NodeProviderOutOfProcTaskHost #12188

Closed

2 tasks

This was referenced Oct 9, 2025

[PerfStar] - Writing New Tests for Multithread #12265

Closed

Fix bug with not passing scheduled node id to the task host task #12639

Merged

dotnet-maestro bot mentioned this pull request Oct 20, 2025

[release/10.0.2xx] Source code updates from dotnet/msbuild dotnet/dotnet#3001

Merged

Copilot AI mentioned this pull request Nov 25, 2025

Refactor Node IDs for taskhost to use record struct #12830

Merged

Conversation

surayya-MS commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Changes Made

Testing

Notes

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!

JanProvaznik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AR-May left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

surayya-MS commented Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

surayya-MS commented Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AR-May commented Oct 3, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

surayya-MS commented Sep 29, 2025 •

edited

Loading

surayya-MS commented Oct 1, 2025 •

edited

Loading

surayya-MS commented Oct 2, 2025 •

edited

Loading