-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
feat: worker pool that can execute tasks on all workers #32120
Conversation
845a318
to
4d4272a
Compare
4d4272a
to
bf4c08b
Compare
bf4c08b
to
ad42d34
Compare
ad42d34
to
6aff32a
Compare
expect(returnValue).toBeArrayOfSize(numWorkers) | ||
// turns sync function into async | ||
expect(returnValue).toSatisfyAll(isPromise) |
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.
Neat!
{ | ||
"name": "gatsby-worker", | ||
"description": "Utility to create worker pools", | ||
"version": "0.0.0-next.0", |
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 think we'll want to start this at 1.0.0?
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.
Early on I would prefer to keep it <1
as we might quickly introduce breaking changes as we start using worker pool and discover new needs or APIs (that's what <1 is for). 1 is when API becomes actually stable enough
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.
Curious how this will play with lerna minor bump - I think it should do 0.1.0-next.0
for the next branch cut? If so, this sounds good to me.
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.
https://github.com/gatsbyjs/gatsby/commits/master/packages/gatsby-plugin-graphql-config/package.json is example of 0.X.Y
package we have and it does bump minor for each release. We want gatsby-worker
to be actual dep only of gatsby
core package - if we will need it in gatsby-cli
for structured logs, we will import it from gatsby/internal there. Messaging (but this will only need messaging and not queueing up tasks)
packages/gatsby/src/utils/worker/__tests__/test-helpers/create-test-worker.ts
Show resolved
Hide resolved
16aab93
to
e48e671
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.
I think we must also replace this usage of JEST_WORKER_ID
:
process.env.FORCE_TEST_DATABASE_ID ?? process.env.JEST_WORKER_ID |
// if node is not the head then it will have .prev | ||
taskNode.prev!.next = taskNode.next |
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.
Should we also re-assign taskNode.next.prev
? Is it possible that it will retain the task through this link?
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.
Yes, you are correct - damn basic data structures ;)
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.
done in ca74834
c1ff0b5
to
2bf3fac
Compare
14741b9
to
ca74834
Compare
Similar as #32120 (comment) - we actually use |
…en tho we ourselves will be using gatsby-worker
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.
👍
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.
Really great stuff 👍
Description
This swaps
jest-worker
with our own worker pool that will be adding features we need for PQR. Initially it has feature parity withjest-worker
(for the features we do use) + ability to execute certain "tasks" on all workers (needed for bootstrapping workers for PQR -jest-worker
setup was too limiting for us - we need greater control).In future PRs there will be added support for messaging between main and workers.
Perf
Not hugely impactful for us, but I wanted to make sure worker pool performance wouldn't regress I used https://github.com/facebook/jest/tree/master/packages/jest-worker/src/__performance_tests__ and adjusted it a bit (https://gist.github.com/pieh/9c3f6dbdd3acafa5a8811418eca4d356) and compared
gatsby-worker
againstjest-worker@27.0.2
(latest as of writing of this comment - note that we currently use older version).Task that does nothing 100000 times
Task that calculate PI 10000 times in workers
With example benchmarks
gatsby-worker
seems a bit faster (~7% when calculating PI 10000 times and ~19% when doing no-op task 100000 times), but realistically we won't see impact of it in real life as our task are really heavy and we usually will have much less tasks than 10000 (so any worker pool overhead is dwarfed by actual tasks).Documentation
In new
gatsby-worker
utility packageREADME.md
Related Issues
[ch33219]