Skip to content
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

New: Use woker-farm to handle webpack multi compile #570

Closed

Conversation

soda0289
Copy link

What did you implement:

Memory usage of this plugin is really high when using package individually. This is because we store the webpack stats/results from each function compile until all functions are finished compiling. These stats object contain all of the compiled source code for each module and chunk generated. We don't use many properties from webpack stats and this change adds a new function processWebpackStats() that only extracts the properties we need (cliOutput, errors, outputPath, and externalModules). This greatly reduces the memory footprint during compile and lets the garbage collector clean up a lot more memory.

Lowering the memory improves performance a little bit but there's still a chance of a memory leak if you are using many webpack plugins that do not cleanup properly. To fix this we must ensure memory is cleaned up after every function compile. To do this I added the npm package worker-farm that runs each function compile in a separate thread/process. This way we can end the process and clean up the memory after each compilation is done. This also lets us compile multiple functions at once and has a huge performance improvement. Almost 50% faster in some repositories I have tested this on. This is the same pattern that parallel-webpack uses to improve performance of webpack multi compile.

Closes #299

How did you implement it:

Refactored the code to create 2 different compilers. Single compiler that uses webpack() in main thread, same as current implementation. Multi compiler uses worker-farm to run each webpack compilation in a seperate thread. Since each worker is its own process we can only pass in serializable objects into the thread. This means we can pass webpack config file path or a JSON object that does not have functions or other non serializable objects. We use the V8 serialize function to test if a webpack config can be serialized using the HTML structured clone algorithm. This might be a breaking change but I don't think its possible to include a non serializable object into a serverless config. If it is possible it will throw an error letting the user know they should use webpack config file path instead.

How can we verify it:

  • Enable package individually
  • Run a build or deploy
  • Ensure every function is compiled correclty

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

@soda0289
Copy link
Author

Its failing since we build on NodeJS 6. V8 serialize function was added in NodeJS 8. I think we should bump up min version to NodeJS10 since 6 has been deprecated for a while. Let me know if that is ok.

@revgum revgum mentioned this pull request Apr 21, 2020
7 tasks
Copy link
Member

@miguel-a-calles-mba miguel-a-calles-mba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops. Looks like the build is failing. May you please investigate it? Thanks.

@j0k3r
Copy link
Member

j0k3r commented May 5, 2020

@miguel-a-calles-mba I agree with @soda0289, we should bump minimum version of node to 10.
We should only have build & support for:

  • Node 10
  • Node 12

AWS disabled ability to update functions running:

  • Node.js 6.10 since May 30 2019
  • Node.js 8.10 since February 3, 2020

@miguel-a-calles-mba
Copy link
Member

miguel-a-calles-mba commented May 13, 2020

@HyperBrain, may you please update the Travis CI and AppVeyor configurations to test against Node 10 and 12, please? Thanks!

@miguel-a-calles-mba miguel-a-calles-mba added this to the 5.4.0 milestone May 13, 2020
@miguel-a-calles-mba miguel-a-calles-mba changed the base branch from master to release/5.4.0 May 13, 2020 14:47
@j0k3r
Copy link
Member

j0k3r commented May 13, 2020

@miguel-a-calles-mba you can do it on your own by editing .travis.yml and .appveyor.yml.

@miguel-a-calles-mba
Copy link
Member

Thanks @j0k3r! I didn't know that.

@miguel-a-calles-mba
Copy link
Member

@soda0289 May you please merge master into your fork? I'm not able to do it for you. The latest version of master now runs unit tests against Node 8 and 10.

@miguel-a-calles-mba
Copy link
Member

Nevermind @soda0289. I figured out how to update your PR to update CI node version.

@miguel-a-calles-mba
Copy link
Member

@soda0289 May you fix the unit tests? I addressed the linting issues, but the test are still failing. Thanks.

@soda0289 soda0289 force-pushed the multi-thread-compiler branch from 5794da1 to 3782e91 Compare May 19, 2020 16:07
This improves memory footprint of compilations using package
individually. By only extracting the properties we need from webpack
stats/result we can clean up a lot more memory every compile. This also
uses seperate processes to compile each function which can clean up all
the memory related to compilations and build multiple functions at same
time.
@soda0289 soda0289 force-pushed the multi-thread-compiler branch from 3782e91 to c06d630 Compare May 19, 2020 16:12
@soda0289
Copy link
Author

@miguel-a-calles-mba Sorry for delayed response. I have fixed up the linting errors and test cases.

Another question I have is if we should add options to allow the user to customize how many threads they want to use or if they want to use multi thread compile at all. Currently it is used every time package individually is set but this might be undesired for some users.

@soda0289
Copy link
Author

Also should I reabse onto branch 'release/5.4.0' or should I change base branch to be master?

@miguel-a-calles-mba
Copy link
Member

@soda0289 Maybe there should be a default value of one thread?

We are targeting release/5.4.0.

@troyready
Copy link

Sane thread default is probably the number of CPU cores (as an example from another ecosystem, I believe that's the default for python's multiprocessing module - using what's returned by multiprocessing.cpu_count())

Having it be configurable would obviously be nice.

@coyoteecd
Copy link
Contributor

@soda0289 @miguel-a-calles-mba Any chance to get this fix in? The plugin is becoming nearly unusable for us

@miguel-a-calles-mba
Copy link
Member

@coyoteecd, we are working to get 5.3.3 released and will start on 5.4.0.

You can install this PR in the meantime.

npm install serverless-heaven/serverless-webpack#pull/570/head

@asprouse asprouse mentioned this pull request Jul 3, 2020
7 tasks
@coyoteecd
Copy link
Contributor

coyoteecd commented Jul 6, 2020

@soda0289 @miguel-a-calles-mba I did a dry-run of this pull request in our environment and hit an issue.

Previously, the plugin exposed entries and options through slsw.lib.entries and slsw.lib.options. See code here and functional description here. I am using a webpack.config that runs a particular plugin based on a command -line option passed to serverless. Now, slsw.lib.options is undefined, so I can't access those options anymore.

Is there a way to keep the existing functionality, or is there an alternative to get the command line options in the worker thread config?


Edit: tried to fix this issue in a fork and gave up. While I can pass the lib.options and make that available in child workers, can't do the same with the entire serverless instance, because of circular references. At this point I gave up and switched to #517, which works well and is backwards compatible.

@dominics
Copy link

dominics commented Jul 6, 2020

I tried this branch today, and it didn't work out for me. Probably because my webpack config isn't serializable? (It's something like https://gist.github.com/dominics/f9f24430789d1b4c4f0297a31b5b3ff7, so there's a function in there - not a common case.) But there were no errors; the bundled code just didn't have any externals (and the verbose logging just says "no external modules needed")

I'd prefer a nice loud error I guess.

@thomaschaaf
Copy link

For us this PR fixed the issue of memory running out :)

@bryantbiggs
Copy link

will this PR make it into 5.4.0?

@diogogvhenriques
Copy link

This PR also solves the issue for some of our projects.

@roni-frantchi
Copy link

This PR had also helped us overcome the OoM errors.
For us, it worked where #299 which is 5.4, did not.
We'd love to see this one make it into 5.4

@corollari corollari mentioned this pull request Dec 1, 2020
7 tasks
@j0k3r j0k3r closed this Jan 29, 2021
@j0k3r j0k3r deleted the branch serverless-heaven:master January 29, 2021 14:25
@bryantbiggs
Copy link

does this mean the change is slated to arrive in 5.4.0?!

@j0k3r j0k3r reopened this Jan 29, 2021
@j0k3r j0k3r changed the base branch from release/5.4.0 to master January 29, 2021 19:15
@j0k3r
Copy link
Member

j0k3r commented Jan 29, 2021

Sorry I made a mistake while merging release/5.4.0 into master and removed it. The removal automatically closed that PR. It should now be rebased against the master and fix conflicts.

@j0k3r j0k3r self-assigned this Jan 29, 2021
@j0k3r
Copy link
Member

j0k3r commented Feb 1, 2021

@soda0289 could you rebase against the master and fix conflicts? Thanks 🙏

@j0k3r j0k3r removed this from the 5.4.0 milestone Mar 4, 2021
@mikewolfd
Copy link

mikewolfd commented Apr 5, 2021

@j0k3r What's going on with this? I have a bug that will be fixed by #570

Edit: I meant a bug fixed by #661

@Enase
Copy link

Enase commented Apr 5, 2021

@mikewolfd I'm not sure, but looks like it was addressed by #517, see v5.4.0

@troyready
Copy link

I'll echo above, from what I can tell this can now be closed. The original issues are now solved for me by using the following serverless config:

package:
  individually: true

custom:
  webpack:
    serializedCompile: true

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JavaScript heap out of memory when packaging many functions