-
Notifications
You must be signed in to change notification settings - Fork 12k
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
fix(@angular/build): address prerendering in-memory ESM resolution in Node.js 22.2.0 and later #27721
Conversation
ff460e8
to
e2070bc
Compare
b6b744b
to
7a5c5e2
Compare
… 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
7a5c5e2
to
23575dd
Compare
Blocking as there are discussion about reverting the Node.js breaking change nodejs/node#53182. Revert PR nodejs/node#53183 |
@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? |
@GeoffreyBooth, the refactoring in this PR involves the main process forking a child process and applying the Before
Now
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.
|
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. |
@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. |
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? |
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
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 |
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 😄) |
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.
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. |
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. |
Node.js 22.2.0 introduced a breaking change affecting custom ESM resolution.
For more context, see: Node.js issue #53097
Closes: #27674