bazel-contrib / rules_nodejs

NodeJS toolchain for Bazel.
https://bazelbuild.github.io/rules_nodejs/
Apache License 2.0
733 stars 523 forks source link

terser runs across chunks in serial #822

Closed alexeagle closed 5 years ago

alexeagle commented 5 years ago

An app with many code-splits spends a lot of time doing terser.

Bazel's model of parallelizing work requires that actions know their inputs and outputs. Code splitting has indeterminate outputs because of synthetic/common chunks. So we have to run a single terser action.

Node 10 now has the worker_threads API, read more at https://blog.logrocket.com/node-js-multithreading-what-are-worker-threads-and-why-do-they-matter-48ab102f8b10/ which means that even in a single action, we could at least parallelize across more cores on the machine. The typical build graph shape will have little parallelism at this point, so the other cores are probably idle.

Note: need to limit how many threads we start.

alexeagle commented 5 years ago

Almost ready to do this in the new terser rule - it just needs to accept TreeARtifact in its inputs first

Toxicable commented 5 years ago

Looks like we can go ahead with this again now that the new rollup and terser rules are in along with the node_module linker.

I think using this for some inspiration is a good start: https://github.com/webpack-contrib/terser-webpack-plugin/tree/master/src However we'll have to write a terser_wrapped again, I don't think we can use the command line forwarding approach when doing this. However we could implement a CLI for terser_wrapped that matches and extends tersers own CLI

alexeagle commented 5 years ago

Exactly, plus I just realized on my bike there is nothing Bazel-specific about this, so really we should have a terser package that minifies directories, and just depend on that. terser-wrapped.js in our own repo is limiting the market for such a feature.

According to https://github.com/terser/terser/issues/75 they don't want to accept that feature upstream, so we should write one (unless I get a reply there or one of us discovers such a thing already exists)

I made a start here https://github.com/alexeagle/rules_nodejs/tree/terser and I suppose we could proceed with the terser-directory-wrapper living in rules_nodejs, but we'd want to move it out eventually, and now seems like a good time :)

alexeagle commented 5 years ago

It's not ideal that we would have to parse the command-line options and call the Terser.minify API either. https://github.com/terser/terser/blob/master/bin/terser has a bunch of code that we want to re-use. Maybe we need a refactoring PR so that the command-line parser can be called in isolation?

Toxicable commented 5 years ago

terser now runs in parallel however it's using child_process to do this

I think we can close this as a blocker for 1.0 and open a new one to do it with worker_threads instead

alexeagle commented 5 years ago

Yes agreed