-
Notifications
You must be signed in to change notification settings - Fork 30k
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
module: have a single hooks thread for all workers #52706
module: have a single hooks thread for all workers #52706
Conversation
Review requested:
|
1025b03
to
fa24d02
Compare
fa24d02
to
ee16c17
Compare
@nodejs/loaders |
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.
Great work!
03f69a8
to
2fdf7b3
Compare
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.
Yahoo! 🙌 great work @dygabo. thank you for this!
a few nits, and a couple small tweaks requested.
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.
🙌
I added a testcase for a scenario where a worker thread import operation would call process.exit. I.e.
I think this is a change in behavior compared to the previous version. Is this scenario covered correctly now? Is this an acceptable change in behavior? |
bb59bc8
to
7f86cde
Compare
port2: toHooksThread, | ||
} = new MessageChannel(); | ||
if (!isInternal) { | ||
// This is not an internal hooks thread => it needs a channel to the hooks thread: |
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.
is the isInternal
flag only used by hooks thread?
if yes, the name should be likely changed (maybe in a followup).
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.
Currently but not inherently limited to
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 looks like a great improvement! cheers
Commit Queue failed- Loading data for nodejs/node/pull/52706 ✔ Done loading data for nodejs/node/pull/52706 ----------------------------------- PR info ------------------------------------ Title module: have a single hooks thread for all workers (#52706) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch dygabo:spawn-only-one-loader-thread -> nodejs:main Labels module, lib / src, needs-ci, loaders, commit-queue-squash Commits 13 - module: have a single hooks thread for all workers - Update lib/internal/modules/esm/loader.js - Apply suggestions from code review - Apply suggestions from code review - code review suggestions addressed - cache response message on exit - add test for process.exit for import running for worker thread - implement unregistration of workers from the hooks thread upon exit - address review finding - fix comment - fix or skip tests that are not fit for running on worker threads - fix flaky test with debug build - fixup! fix flaky test with debug build Committers 1 - Gabriel Bota PR-URL: https://github.com/nodejs/node/pull/52706 Reviewed-By: Geoffrey Booth Reviewed-By: Jacob Smith Reviewed-By: Antoine du Hamel Reviewed-By: Gerhard Stöbich ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/52706 Reviewed-By: Geoffrey Booth Reviewed-By: Jacob Smith Reviewed-By: Antoine du Hamel Reviewed-By: Gerhard Stöbich -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last approving review: ⚠ - module: have a single hooks thread for all workers ⚠ - Update lib/internal/modules/esm/loader.js ⚠ - Apply suggestions from code review ⚠ - Apply suggestions from code review ⚠ - code review suggestions addressed ⚠ - cache response message on exit ⚠ - add test for process.exit for import running for worker thread ⚠ - implement unregistration of workers from the hooks thread upon exit ⚠ - address review finding ⚠ - fix comment ⚠ - fix or skip tests that are not fit for running on worker threads ⚠ - fix flaky test with debug build ⚠ - fixup! fix flaky test with debug build ℹ This PR was created on Fri, 26 Apr 2024 13:22:51 GMT ✔ Approvals: 4 ✔ - Geoffrey Booth (@GeoffreyBooth) (TSC): https://github.com/nodejs/node/pull/52706#pullrequestreview-2025361284 ✔ - Jacob Smith (@JakobJingleheimer): https://github.com/nodejs/node/pull/52706#pullrequestreview-2026747841 ✔ - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/52706#pullrequestreview-2026755507 ✔ - Gerhard Stöbich (@Flarna): https://github.com/nodejs/node/pull/52706#pullrequestreview-2040506445 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2024-05-08T00:05:42Z: https://ci.nodejs.org/job/node-test-pull-request/59022/ - Querying data for job/node-test-pull-request/59022/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/8995057971 |
Landed in 22cb99d |
PR-URL: #52706 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
I've added dont-land-on-v18.x and dontt-land-on-v20.x given how breaking this is. |
PR-URL: nodejs#52706 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
PR-URL: nodejs#52706 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
This implements reusing the single esm loader hooks thread for all the subsequent workers.
It started as a trial to solve the test contributed by @GeoffreyBooth from #50752. The test content is taken over from that branch and only slightly modified to also support transitive worker threads (that are started by some worker and not by the main thread). This is probably a not very common use case but the interface supports it so I put in the test as well.
With this initial commit all tests are passing.
Let's start a discussion and if there are additional usages that are missed by the implementation or if something needs improvements, let me know. If the implementation matches the expectations, we could close the test-only PR and continue the discussion here. Let me know your thoughts.