| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
… Node.js 22.2.0 and later Node.js 22.2.0 introduced a breaking change affecting custom ESM resolution. For more context, see: [Node.js issue #53097](nodejs/node#53097) Closes: #53097
This update ensures that we can test prerendering with the ESM loader breaking change. For more details, see: nodejs/node#53097
|
Blocking as there are discussion about reverting the Node.js breaking change nodejs/node#53182. Revert PR nodejs/node#53183 |
Sorry, something went wrong.
|
@alan-agius4 Do you mind explaining the approach taken in this PR? It seems like you refactored from using worker threads to using child processes? |
Sorry, something went wrong.
|
@GeoffreyBooth, the refactoring in this PR involves the main process forking a child process and applying the --import flag. The child process will then be responsible for creating the thread workers. Before [main process] -> [thread workers --import] Now [main process] -> [forked process --import] -> [thread workers] The above, will only work for Node.js 22.2 as the ESM loaders in workers are not inherited from the main process on older versions of Node.js, hence for older versions we have another code path. [main process] -> [forked process --import (noop)] -> [thread workers --import] |
Sorry, something went wrong.
Thanks. Do you have a way to measure the performance impact of using child processes instead of workers? So you understand the context, we’re wondering about the necessity of supporting different hooks for different threads, as doing so adds a large amount of complexity to a feature that’s already quite complex. |
Sorry, something went wrong.
|
@GeoffreyBooth, I can run some tests and provide you with some numbers tomorrow, as I am based in Europe. However, the performance impact will vary depending on the application's size and complexity. In a simple "hello world" application, the performance regression is negligible. But as the application grows, the additional IPC layer introduced by the forked process might become a bottleneck, especially when transferring large amounts of data between the main process and the thread worker through the forked process and vice-versa. |
Sorry, something went wrong.
Of course, I was just hoping to get a general idea of the difference for an app of nontrivial size. Once you're using child processes, is there a reason to use worker threads? |
Sorry, something went wrong.
Yes, we use worker threads to prerender an N number of routes in parallel. Additionally, we need to render the application in isolation to ensure that globalThis and the global scopes of the forked process remain unaltered by the rendering process, which could otherwise be affected and modified by running the application.
Actually, this highlighted a bug in the above implementation, which I haven't had time to fix yet. The issue is that in big apps we are sending a large amount of data from the forked process to the main process via a single message, causing an Invalid string length error. I suspect that there will be an extra latency introduced by the additional IPC layer in the forked process. Previously, the worker process sent data directly to the main process. However, now the data flows through an intermediate forked process, adding complexity to the communication. |
Sorry, something went wrong.
Right, but why not use child processes for this purpose? What advantages do worker threads provide? (Sincere question.) Along those lines, why spawn X number of child processes which then spawn Y number of worker threads? Why not just fork Y number of child processes directly? I was assuming that threads would be faster and the only question was how much, but then I did some searching and found this: https://github.com/orgs/nodejs/discussions/44264. I’m curious to see if your test case shows worker threads to be faster, once you resolve the issues on this branch. If this branch is faster, would you want to keep this new implementation even if worker threads became an option again? If not, why not? (All sincere questions 😄) |
Sorry, something went wrong.
Since we transfer large data between the main and "worker" process and vice-versa which can easily several megabytes. This should make them faster than child processes, From my understanding thread workers are lightweight, generally faster, and consume fewer resources than forked processes. Furthermore, libraries like Piscina streamline the management of worker threads, offering a more user-friendly pool management APIs.
You are correct, an alternative solution might be more optimal, but my primary goal was to minimize changes introduced in a patch version.
I downloaded the benchmark and they are pretty close. > node-workers node:worker_threads benchmark: Processing 3000 files with 6 workers Average time to process 3000 files: 8.062776568001508 Benchmark elapsed time: 40.31395083197951 > node-forks > node node/run-forks.mjs node:child_process.fork benchmark: Processing 3000 files with 6 forks Average time to process 3000 files: 8.928233420807123 Benchmark elapsed time: 44.641243622004986
That's a good question, if forked process turns out to be faster, likely we'll go with them. After all, our goal is always to reduce build times. |
Sorry, something went wrong.
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Node.js 22.2.0 introduced a breaking change affecting custom ESM resolution.
For more context, see: Node.js issue #53097
Closes: #27674