feat(rollup): run terser in threads #833

Toxicable · 2019-06-08T03:41:36Z

PR Checklist

Please check if your PR fulfills the following requirements:

Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature (please, look at the "Scope of the project" section in the README.md file)
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

Yes
No

Other information

alexeagle

cool! need any help to get the CI green?

alexeagle · 2019-06-10T14:43:11Z

internal/rollup/terser-wrapped.js

-const DEBUG = false;
+// worker_threads is only avilable on version > 10.5.0
+const [major, minor] = process.version.replace('v', '').split('.');
+const WORKER_THREADS_AVILABLE = major > 10 || major == 10 && minor > 5


nit: why not just try/catch on the require call?

should also be minor >= 5

Total guess here, but I would assume that doing arithmetic is faster than recovering from an exception.
However, in hindsight, it's probably the smallest amount of performance difference here.

alexeagle · 2019-06-10T14:43:49Z

internal/rollup/terser-wrapped.js

+  console.warn(
+      `WARNING: worker_threads not are not avilable on your version of Nodejs: ${process.version}
+    Running Terser in serial mode
+    Upgrade your Nodejs version to 10.5.0 minimum for worker_threads to be avilable`)


we should be more specific here, to update the /WORKSPACE file, otherwise users will update the nodejs on their machine and it still won't work

alexeagle · 2019-06-10T14:44:56Z

internal/rollup/terser-wrapped.js

+ * @param {boolean} debug
+ */
+async function runTerserParallel(files, debug) {
+  // TODO: do some concurrency limiting here


do we have to finish the TODO before we merge/release this?

We should. context switching will destroy performance gains.
os.cpus().length - 1 is typically a good cap.

I think a worker pool would also make sense here. There's no need to tear down and bring up new threads and re-initialize terser for each file.

alexeagle · 2019-06-10T14:45:44Z

internal/rollup/terser-wrapped.js

+} else {
+  console.warn(
+      `WARNING: worker_threads not are not avilable on your version of Nodejs: ${process.version}
+    Running Terser in serial mode


"serial mode" is a bit obtuse for a novice user, how about just "optimizing one file at a time"

clydin · 2019-06-10T16:16:24Z

internal/rollup/rollup_bundle.bzl

@@ -334,6 +334,9 @@ def _run_terser(ctx, input, output, map_output, debug = False, comments = True,
        args.add("--debug")
        args.add("--beautify")

+    # we need this arg to enable worker_threads
+    args.add("--node_options=--experimental-worker")


If on Node 12+ this shouldn't be added

Fair, not sure how to get the nodejs version in in skylark though.
@alexeagle You mentioned there's a way to do it the other day?

Once we have toolchains in it would actually be possible to add the node version to the toolchain and then one could get it in starlark. Not sure though how it would be possible currently though.

If problematic, node 12 currently won't error with the option present. So this could be addressed moving forward.

Toxicable

Thanks for the review.
I do have a few points I'm not 100% on, I've commented on them below.
If we could resolve those then I think we're good to go here.

Toxicable · 2019-06-11T09:06:16Z

internal/rollup/package.json

@@ -12,6 +12,7 @@
        "shelljs": "0.8.3",
        "source-map-explorer": "^1.7.0",
        "terser": "^3.16.1",
+        "p-limit": "^2.2.0",


This dosen't actually work, how do we add internal npm modules?

Toxicable · 2019-06-11T09:07:32Z

internal/rollup/terser-wrapped.js

+      return {inputFile, outputFile, sourceMapFile: outputFile + '.map'};
+    })
+
+    // TODO: allow config to force serial workers here


@Globegitter Suggested this one.
It would be a good addition but probably not apart of this first cut.

Toxicable · 2019-06-11T09:08:02Z

internal/rollup/terser-wrapped.js

+ * @param {boolean} debug
+ */
+async function runTerserParallel(files, debug) {
+  // TODO: make this configurable


Same here, would be a good candidate for config, but we'll worry about it later I think

Toxicable · 2019-06-11T09:09:14Z

internal/rollup/terser-wrapped.js

+  if (result.error) {
+    // TODO: figure out error reporting here
+    // scroll up a little from this: https://github.com/terser-js/terser#minify-options
+    throw result.error;


Im not certain of the format of this error object, we need to do some trial and error I think to figure out what kind of errors we get from Terser.
However, I'm not familiar enough with Terser to know how to cause an error :P

clydin · 2019-06-11T16:35:57Z

internal/rollup/terser-wrapped.js


-  if (DEBUG) console.error(`Running node ${args.join(' ')}`);
+  const terserOptions = buildTerserConfig(sourceMapFile, debug)
+  const inputCode = (await fs.promises.readFile(inputFile)).toString();


Are character encoding and BOM presence relevant here? This is currently assuming UTF-8.

I really don't know. We could add a trim() call which would remove the BOM if it is actually encoded as that.
I don't know the impact of stripping the BOM and re-writing it as plain UTF8 once we're done

Toxicable · 2019-09-23T06:02:48Z

replaced with #1172

feat(rollup): run terser in threads

d1d3058

Toxicable requested review from alexeagle, gregmagolan and kyliau as code owners June 8, 2019 03:41

googlebot added the cla: yes label Jun 8, 2019

alexeagle reviewed Jun 10, 2019

View reviewed changes

clydin reviewed Jun 10, 2019

View reviewed changes

feat(rollup): limit number of concurrent workers

bbe6310

Toxicable commented Jun 11, 2019

View reviewed changes

clydin reviewed Jun 11, 2019

View reviewed changes

fix(rollup): strip BOM from input

2f52e42

Toxicable mentioned this pull request Jul 20, 2019

Forked processes are unable to resolve node_modules/ #312

Closed

Toxicable closed this Sep 23, 2019

resublime mentioned this pull request Mar 8, 2021

[Snyk] Security upgrade chokidar from 2.0.4 to 3.0.0 LivingMatrix/rules_nodejs#4

Open

DanielWisenhoffDebricked mentioned this pull request Mar 8, 2021

[Snyk] Security upgrade chokidar from 2.0.4 to 3.0.0 DanielWisenhoffDebricked/rules_nodejs#32

Open

DanielWisenhoffDebricked mentioned this pull request Dec 19, 2023

[Snyk] Security upgrade chokidar from 2.0.4 to 3.0.0 DanielWisenhoffDebricked/rules_nodejs#174

Open

DanielWisenhoffDebricked mentioned this pull request Mar 22, 2024

[Snyk] Security upgrade chokidar from 2.0.4 to 3.0.0 DanielWisenhoffDebricked/rules_nodejs#189

Open

resublime mentioned this pull request Mar 22, 2024

[Snyk] Security upgrade chokidar from 2.0.4 to 3.0.0 LivingMatrix/rules_nodejs#19

Open

DanielWisenhoffDebricked mentioned this pull request May 14, 2024

[Snyk] Security upgrade chokidar from 2.0.4 to 3.0.0 DanielWisenhoffDebricked/rules_nodejs#207

Open

resublime mentioned this pull request May 14, 2024

[Snyk] Security upgrade chokidar from 2.0.4 to 3.0.0 LivingMatrix/rules_nodejs#20

Open

resublime mentioned this pull request May 22, 2024

[Snyk] Security upgrade chokidar from 2.0.4 to 3.0.0 LivingMatrix/rules_nodejs#21

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rollup): run terser in threads #833

feat(rollup): run terser in threads #833

Toxicable commented Jun 8, 2019 •

edited

Loading

alexeagle left a comment

alexeagle Jun 10, 2019

clydin Jun 10, 2019

Toxicable Jun 11, 2019

alexeagle Jun 10, 2019

alexeagle Jun 10, 2019

clydin Jun 10, 2019

clydin Jun 11, 2019

alexeagle Jun 10, 2019

clydin Jun 10, 2019

Toxicable Jun 11, 2019

Globegitter Jun 11, 2019 •

edited

Loading

clydin Jun 11, 2019

Toxicable left a comment

Toxicable Jun 11, 2019

Toxicable Jun 11, 2019

Toxicable Jun 11, 2019

Toxicable Jun 11, 2019 •

edited

Loading

clydin Jun 11, 2019 •

edited

Loading

Toxicable Jun 14, 2019

Toxicable commented Sep 23, 2019

feat(rollup): run terser in threads #833

feat(rollup): run terser in threads #833

Conversation

Toxicable commented Jun 8, 2019 • edited Loading

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

alexeagle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Globegitter Jun 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Toxicable left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Toxicable Jun 11, 2019 • edited Loading

Choose a reason for hiding this comment

clydin Jun 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Toxicable commented Sep 23, 2019

Toxicable commented Jun 8, 2019 •

edited

Loading

Globegitter Jun 11, 2019 •

edited

Loading

Toxicable Jun 11, 2019 •

edited

Loading

clydin Jun 11, 2019 •

edited

Loading