perf(turbopack-node): optimize worker communication with pre-allocated TaskChannels #101
Conversation
Summary of ChangesHello @xusd320, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on a performance optimization within the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 introduces a performance optimization for worker communication by pre-allocating TaskChannels. This is a solid improvement that avoids repeated hash map lookups in the hot path of sending and receiving messages. The refactoring moves channel acquisition to a one-time operation per task, encapsulating the logic within a new TaskChannels struct which also handles cleanup via its Drop implementation. Additionally, the changes clarify the precedence of the worker_pool feature over process_pool when both are enabled. My review highlights a couple of areas where the use of blocking locks in an asynchronous context could still pose a performance concern under high contention, and I've suggested potential improvements.
| impl TaskChannels { | ||
| /// Create and register channels for a new task. | ||
| /// Channels are inserted into the global maps so JS workers can find them. | ||
| pub(crate) fn new(task_id: u32, worker_id: u32) -> Self { | ||
| let worker_channel = { | ||
| let mut map = WORKER_POOL_OPERATION.worker_routed_channel.lock(); | ||
| map.entry(worker_id) | ||
| .or_insert_with(|| Arc::new(MessageChannel::unbounded())) | ||
| .clone() | ||
| }; | ||
|
|
||
| let task_channel = { | ||
| let mut map = WORKER_POOL_OPERATION.task_routed_channel.lock(); | ||
| map.entry(task_id) | ||
| .or_insert_with(|| Arc::new(MessageChannel::unbounded())) | ||
| .clone() | ||
| }; | ||
|
|
||
| Self { | ||
| worker_channel, | ||
| task_channel, | ||
| task_id, | ||
| } | ||
| } |
There was a problem hiding this comment.
The new function for TaskChannels uses blocking lock() calls to access worker_routed_channel and task_routed_channel. Since this function is called from an async context (WorkerThreadPool::evaluate), these blocking calls can stall the executor's thread if the locks are contended. While this change moves the locking out of the hot send/recv path (which is a great performance improvement!), the blocking nature of the lock itself remains. For even better performance and responsiveness in a highly concurrent scenario, consider using a concurrent hash map (like dashmap) which would allow for non-blocking or more granularly locked access.
| impl Drop for TaskChannels { | ||
| fn drop(&mut self) { | ||
| // Only remove task channel, worker channel is shared across tasks | ||
| WORKER_POOL_OPERATION | ||
| .task_routed_channel | ||
| .lock() | ||
| .remove(&self.task_id); | ||
| } | ||
| } |
There was a problem hiding this comment.
The drop implementation for TaskChannels uses a blocking lock() on task_routed_channel. Since TaskChannels is part of WorkerOperation which is used in an async context, this drop can be called from within an async task. A blocking call here can stall the executor's thread if the lock is contended, which could impact performance and responsiveness. While this blocking behavior existed before this refactoring in WorkerOperation::drop, it's worth considering alternatives to avoid blocking in async code, such as using a concurrent hash map (like dashmap) for task_routed_channel.
…d TaskChannels - Make worker_pool take priority when both features are enabled - Change cfg conditions from #[cfg(feature = "process_pool")] to #[cfg(all(feature = "process_pool", not(feature = "worker_pool")))] - Pre-allocate TaskChannels to avoid HashMap lookups during send/recv - Remove unused #[allow(unused)] attributes on napi exports - Clean up unused helper functions (send_message_to_worker, recv_task_message, etc.)
xusd320
force-pushed
the
perf/turbopack-node-task-channels
branch
from
54dabf5 to
1a7fe0d
Compare
Sync optimization from vercel#86266