Allow NodeProviderOutOfProcTaskHost to manage multiple nodes instead of one per arch - #12577
#12577Allow NodeProviderOutOfProcTaskHost to manage multiple nodes instead of one per arch#12577surayya-MS merged 30 commits intodotnet:maindotnet/msbuild:mainfrom surayya-MS:out-of-proc-task-host-second-trysurayya-MS/msbuild:out-of-proc-task-host-second-tryCopy head branch name to clipboard
Conversation
…it for combined key for taskHostNodeId
This reverts commit ce75443.
There was a problem hiding this comment.
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 |
JanProvaznik
left a comment
There was a problem hiding this comment.
I think this is well executed change, please address/discuss comments. Then lgtm.
AR-May
left a comment
There was a problem hiding this comment.
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?
@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
@AR-May I'll try to do that in the follow-up PR. |
Ok, works for me! |
Fixes #12552
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
nodeIdif the in-proc nodes (only when multi-threaded mode is on) intoNodeProviderOutOfProcTaskHostnodeIdwith the combination of(int)HanshakeOptions(is originally used as key for_nodeContexts), to generate a unique id for task host nodeTesting
Manually tested by building this project that references other projects with
Messagetasks. ModifiedMessagetask to show the process id it is running on.msbuild.exe .\MainProject.proj - mwith combination of with/without-mtswitch and with/without settingMSBUILDFORCEALLTASKSOUTOFPROC.Notes