[PerfFix] Avoid separate thread for MP executor shm spin (take 2) #28319
Conversation
Signed-off-by: Nick Hill <[email protected]> (cherry picked from commit c9f66da)
Signed-off-by: Nick Hill <[email protected]>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the multiprocessing executor to remove the I/O thread pool, aiming to fix a performance regression. The core change involves a new threadless FutureWrapper and a manual future queue to manage asynchronous RPC calls. While this is a clever way to avoid thread overhead, the new FutureWrapper implementations in both multiproc_executor.py and ray_utils.py have dropped support for timeouts, which is a functional regression from the standard Future API. Furthermore, the interaction between KVOutputAggregator and the new FutureWrapper in multiproc_executor is complex, tightly coupled, and introduces a critical bug that will cause a crash if a timeout is used. I've provided detailed comments and suggestions to address these issues.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Signed-off-by: Nick Hill <[email protected]>
Signed-off-by: Nick Hill <[email protected]>
njhill
changed the title
…lm-project#28319) Signed-off-by: Nick Hill <[email protected]> Signed-off-by: xuebwang-amd <[email protected]>
…lm-project#28319) Signed-off-by: Nick Hill <[email protected]>
2 participants
This is a re-apply of #28012 which was reverted in #28289 due to a bug related to aggregating kv connector outputs which broke for example Nixl P/D for TP > 1.
The second commit is a fix for that issue. The original PR was itself a fix for a significant performance regression.
As a follow-on I will likely refactor this a little more and improve the test coverage.