-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Allow NodeProviderOutOfProcTaskHost
to manage multiple nodes instead of one per arch
#12521
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
Conversation
previously _nodeContext was dict with key of HandshakeOptions, now it is nodeId, previously NodeProviderOutOfProcTaskHost allowed max 4 nodes depending on the arch and etc., now it is unlimited
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 refactors the NodeProviderOutOfProcTaskHost
class to support managing multiple nodes dynamically rather than being limited to exactly 4 nodes. The change removes the hardcoded MaxNodeCount = 4
limitation and introduces proper node ID management.
Key Changes
- Replaced
HandshakeOptions
enum-based node identification with integer-based node IDs - Removed the hardcoded maximum node count limitation of 4 nodes
- Updated the node management system to use
ConcurrentDictionary
with proper locking mechanisms
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
File | Description |
---|---|
TaskHostTask.cs | Updated to use integer node IDs instead of HandshakeOptions for communication with task host provider |
NodeProviderOutOfProcTaskHost.cs | Refactored node management system to support unlimited nodes with proper concurrent access and ID generation |
src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs
Outdated
Show resolved
Hide resolved
src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs
Outdated
Show resolved
Hide resolved
src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs
Outdated
Show resolved
Hide resolved
src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs
Outdated
Show resolved
Hide resolved
Changed Message task to append Process id for manual validation (did't commit this change)
|
NodeProviderOutOfProcTaskHost
to manage multiple nodes instead of one per arch
src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs
Outdated
Show resolved
Hide resolved
Let's also check that taskhosts are reused properly during the build - project which has multiple message tasks, during the build with /mt /m should print have same process ID. |
I added second message to each of the projects. The result is different process ids for messages from the same project. I then checked same without -mt, and the result is the same. But in main without -mt, the result in is same process id for tasks from the same project.
|
src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs
Outdated
Show resolved
Hide resolved
From the previous results: Before the multi-threaded mode was introduced, there was one node per process , and the node to reuse in We need to somehow pass on nodeId (in-proc) to |
{ | ||
// node already exists, so "creation" automatically succeeded | ||
nodeCreationSucceeded = true; | ||
nodeId = _nextNodeId++; |
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.
consider https://learn.microsoft.com/en-us/dotnet/api/system.threading.interlocked.increment?view=net-9.0 instead of the lock
private HandshakeOptions _requiredContext = HandshakeOptions.None; | ||
private HandshakeOptions _requiredHandshakeOptions = HandshakeOptions.None; | ||
|
||
private int _requiredNodeId = -1; |
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.
this naming is confusing, if you want to express that a specific taskhosttask has a mapping to a nodeId, then I'd just say something like _taskHostNodeId
Closing this in favor of #12577 |
Fixes #12552
Context
Changes Made
Testing
Notes