-
Notifications
You must be signed in to change notification settings - Fork 417
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
Use jest worker to parallelize compilation #859
Use jest worker to parallelize compilation #859
Conversation
@janicduplessis I switched the PR as ready for review by mistake I thought the message said to approve the PR so the test can run. I misread, sorry. |
All good, yea I think the best is start with #858 then come back to this one. |
e7e0458
to
103e9df
Compare
Ok I had another look at this and rebased. I think it is good. Some minor changes that might not be obvious:
Also verified that using One potential question is do we want to add an option to configure this? Personally I don't think it is needed and concurrency can already be disabled by setting the existing option to 1. |
@j0k3r Any chance you could have a look at this? |
Sorry for being late to reply.
It seems that
Do you mean that setting |
The problem is everything passed to jest-worker needs to be serializable and
Yea it will still use jest-worker, but execute only one build at a time. I guess the question was more if we need a way to disable executing in a worker. Maybe if we don't have a way to access lib.serverless in the worker then it could be a use case to execute outside of it. |
I see. |
103e9df
to
a351f73
Compare
@j0k3r Turns out the config object is also a class instance so what I did is log the serverless object and copy manually the serializable parts that seem useful into a regular object. This covers the use case you listed. Edit: Copied only props documented in the serverless typescript type definitions and updated our lib typedef to only include props that we copy instead of using the type directly from serverless. |
de4b91e
to
27dec4d
Compare
27dec4d
to
0a9c2d7
Compare
Looks like people are using more elements which aren't defined in the serverless typescript definition and/or not documented, like:
I don't know how we can properly handle such case and warn people that the element doesn't exist. |
What did you implement:
Improve build performance by parallelizing webpack compilation using worker threads.
Builds on top of #858, (check only commit 2a776b8). Note there still might be some cleaning up to do and useWorkerThreads option should probably be configurable as it is only usable with node 11+. This also uses async / await which afaik wasn't used in the codebase, but should be fine since it is supported since node 8.
How did you implement it:
Use jest-worker which implements a simple promise based api to execute modules on worker threads.
Most of the changes were needed to avoid passing the webpack config directly, since worker inputs and outputs need to be serializable and webpack configs aren't. What we do to fix this is pass some of the config that we modify (based on the code in validate.js) and merge it with the config that we load again in the worker.
Also uses rechoir and interpret to allow loading webpack configs in other languages like typescript (https://webpack.js.org/configuration/configuration-languages/), this is based on what the webpack cli does. Not sure why it works in the main process without any special config, but not in worker threads so had to add this.
How can we verify it:
Tested in a large project with package: individually on a 2.8 GHz Quad-Core Intel Core i7 Macbook Pro.
Before: builds in ~1000s
After: builds in ~500s
Todos:
Is this ready for review?: NO
Is it a breaking change?: POSSIBLY