-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Allow NodeProviderOutOfProcTaskHost to manage multiple nodes instead of one per arch #12577
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow NodeProviderOutOfProcTaskHost to manage multiple nodes instead of one per arch #12577
Conversation
…it for combined key for taskHostNodeId
This reverts commit ce75443.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is well executed change, please address/discuss comments. Then lgtm.
src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs
Show resolved
Hide resolved
src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/Components/Communications/NodeProviderOutOfProcTaskHost.cs
Show resolved
Hide resolved
src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs
Show resolved
Hide resolved
src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs
Show resolved
Hide resolved
src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs
Outdated
Show resolved
Hide resolved
@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
nodeId
if the in-proc nodes (only when multi-threaded mode is on) intoNodeProviderOutOfProcTaskHost
nodeId
with 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
Message
tasks. ModifiedMessage
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 settingMSBUILDFORCEALLTASKSOUTOFPROC
.Notes