feat: add support for `parallelization` && `caching` (`options.parallel`) by aui · Pull Request #77 · webpack-contrib/uglifyjs-webpack-plugin

aui · 2017-07-09T10:35:47Z

Use multiple processes to perform tasks in parallel.
Use the file caching system to skip files that have not changed.

Build time contrast:

| cache | workers | time(ms) |
| ------| ------- | -------- |
| false |       1 |    35216 |
| false |      31 |    16345 |
| true  |      31 |     3384 |

Demo: https://travis-ci.org/aui/uglifyjs-webpack-plugin-demo

jsf-clabot · 2017-07-09T10:35:48Z

All committers have signed the CLA.

michael-ciniawsky · 2017-07-09T15:52:22Z

Awesome 😊 I will review as soon as possible 😛

michael-ciniawsky · 2017-07-09T15:56:33Z

+    "find-cache-dir": "^1.0.0",
    "source-map": "^0.5.6",
-    "uglify-es": "^3.0.21",
+    "uglify-es": "^3.0.24",


Already included in #74 I will merge that PR

#74 merged, this should also update some of the snapshots you updated aswell :)

michael-ciniawsky · 2017-07-09T15:57:49Z

-    "test": "jest",
-    "test:coverage": "jest --collectCoverageFrom='src/**/*.js' --coverage",
-    "test:watch": "jest --watch",
+    "test": "jest --env node",


Why is that needed and please revert it in any case here, since we handle that in a project called webpack-defaults separately across the organisation

Jest default configuration is --env = jsdom, it can simulate the browser environment, I worry that it takes up extra memory

Actually, Jest uses less memory than Karma does, by quite a bit. That said, this change if needed belongs in webpack-defaults.

All of our libs run the exact same projects setup. So for the scope of this pull request, this needs to be set back to what it was.

If you feel that strongly about this particular configuration, open a pull request in https://github.com/webpack-contrib/webpack-defaults/issues so the change can be applied to all of our libs in the next patch release of defaults.

michael-ciniawsky · 2017-07-09T16:01:54Z

-      ie8,
-    };
+    this.options.uglifyOptions = this.options.uglifyOptions || {};
+    this.options.maxWorkers = this.options.maxWorkers || os.cpus().length;


this.options.parallel = { cache: '{Boolean}' || 'path/to/cache', workers: '{Number}' }

parallelmight not be the best name, open for suggestions here :)

@mikesherov Should this be a +1 for options.cpus ? 🙃

length - 1

michael-ciniawsky · 2017-07-09T16:04:03Z

+import crypto from 'crypto';
+import cacache from 'cacache';
+
+const hashAlgorithm = 'sha512';


hashAlgorithm =>hash || algorithm || sha512

michael-ciniawsky · 2017-07-09T16:26:21Z

compute-cluster seems to be fairly 'outdated' and is on v0.0.9 with failing tests. Could you elaborate on your choice here please ? :). What about worker-farm ?

hulkish · 2017-07-09T16:28:04Z

i agree with @michael-ciniawsky, i really like worker-farm for this also.

michael-ciniawsky · 2017-07-09T16:39:08Z

+  };
+};
+
+const buildCommentsFunction = (options, uglifyOptions, extractedComments) => {


buildCommentsFunction => buildComments || comments

michael-ciniawsky · 2017-07-09T16:40:42Z

@@ -0,0 +1,108 @@
+import uglify from 'uglify-es';
+
+const defaultUglifyOptions = {


move the {Object} to L21 please :)

michael-ciniawsky · 2017-07-09T16:42:15Z

+  },
+};
+
+const buildDefaultUglifyOptions = ({ ecma, warnings, parse = {}, compress = {}, mangle, output, toplevel, ie8 }) => {


buildDefaultUglifyOptions => buildOptions || defaults

michael-ciniawsky · 2017-07-09T16:45:42Z

+import ComputeCluster from 'compute-cluster';
+import { get, put } from './cache';
+import { encode } from './serialization'; // eslint-disable-line import/newline-after-import
+const uglifyVersion = require(require.resolve('uglify-es/package.json')).version; // eslint-disable-line import/no-dynamic-require


const version = { uglify: require(require.resolve('uglify-es/package.json')).version, plugin: require('../../package.json').version // require.resolve() here aswell ? }

Maybe consider a separate file src/uglify/versions.js ?

michael-ciniawsky · 2017-07-09T16:48:37Z

@@ -0,0 +1,46 @@
+const toType = value => (Object.prototype.toString.call(value).slice(8, -1));


Please give an example, why this is needed here

Since RegExp or Function can not be serialized, they need special handling:

worker.send({ extractComments: /foo/ });

// worker process.on('message', (message) => { console.log(message); // -> { extractComments: {} } 😢 })

Use serialization :

worker.send(encode({ extractComments: /foo/ }));

// worker process.on('message', (message) => { message = decode(message); console.log(message); // -> { extractComments: /foo/ } 😊 })

I don't think we can legitimately do this for functions, though.

var a = 1; function toBeSerialized() { console.log(a); }

in the case above, this results in non-obvious errors when serializing and deserializing in a different context. Is there any way we can avoid passing functions or regexes down to the worker and instead post process once comments are sent back to us?

Thanks for reminding me. I now think of three viable options:

When parallel.workers> 0, users are not allowed to use Function or RegExp

Before serializing, use ESlint to check the Function that contains context-dependent

At the run-time stage, catch the error and tell the user why the error occurred

I am currently inclined to the third program because it is friendly and simple. :-)

@mikesherov I refactored the serialization.js, and now the user can know the details of the error :-)

Error: options serialization failed: "test" parse failed: 'console' is not defined. 1| function toBeSerialized() { >> 2| console.log(a); 3| }

mikesherov · 2017-07-09T18:34:47Z

Great work here, a couple of notes:

Is workerization faster when CPU count is 1? Same question for when number of files to uglify is less than CPU count?
Separately, we should use worker-farm to manage processes.
Please max sure maxWorkers is maxed out at CPU count - 1. Having more workers than CPUs - 1 will introduce jank and slowdown.

michael-ciniawsky · 2017-07-19T01:02:01Z

@aui Are there any options definitely known to not work in when options.parallel is enabled? I need to update the docs PR (#61) with the changes here accordingly, so anything thats needs attention in terms of documentation would be appreciated if you could give me a brief sum up on that 😛

hulkish · 2017-07-19T21:52:00Z

    compilerEnv.context = '';

-    const plugin = new UglifyJsPlugin();
+    const plugin = new UglifyJsPlugin({ parallel: { cache: false, workers: 0 } });


Please move all options related changes in this test to it's own test file, or to parallel-option.test.js. The purpose of this test is to literally have empty options.

hulkish · 2017-07-19T21:58:31Z

    });

    new UglifyJsPlugin({
+      parallel: { cache: false, workers: 0 },


Same comment I made for empty-options.test> It's important that these options for these tests are not changed in order for us to really understand that it is not breaking those use cases.

hulkish · 2017-07-19T21:59:50Z

    compilerEnv.context = '';

    const plugin = new UglifyJsPlugin({
+      parallel: { cache: false, workers: 0 },


Same as #77 (comment)

hulkish · 2017-07-19T22:00:08Z

  it('matches snapshot', () => {
    const compiler = createCompiler();
    new UglifyJsPlugin({
+      parallel: { cache: false, workers: 0 },


Same as #77 (comment)

hulkish · 2017-07-19T22:00:25Z

    compilerEnv.context = '';

    const plugin = new UglifyJsPlugin({
+      parallel: { cache: false, workers: 0 },


Same as #77 (comment)

hulkish · 2017-07-20T00:11:27Z

 Array [
  "Error: main.0c220ec66316af2c1b24.js from UglifyJs
-DefaultsError: \`invalid-option\` is not a supported option",
+\`invalid-option\` is not a supported option",


Where did DefaultsError: go?

uglify-es error object does not have a msg field.

src/index.js > buildError() :

} else if (err.msg) { return new Error(`${file} from UglifyJs\n${err.msg}`); } return new Error(`${file} from UglifyJs\n${err.stack}`); // -> ...DefaultsError: `invalid-option` is not a supported option"

} else if (err.message) { return new Error(`${file} from UglifyJs\n${err.message}`); // -> ...`invalid-option` is not a supported option" } return new Error(`${file} from UglifyJs\n${err.stack}`);

can you try changing this line to:

const str = exports.removeCWD(error.message)

hulkish · 2017-07-20T00:11:46Z

+\`invalid-option\` is not a supported option",
  "Error: manifest.6afe1bc6685e9ab36c1c.js from UglifyJs
-DefaultsError: \`invalid-option\` is not a supported option",
+\`invalid-option\` is not a supported option",


Again about DefaultsError

hulkish · 2017-07-20T00:12:20Z

    compilerEnv.context = '';

    const plugin = new UglifyJsPlugin({
+      parallel: { cache: false, workers: 0 },


This one's fine, since this is an all-options test.

hulkish · 2017-07-20T00:14:44Z

            });

-            it('should get all warnings', () => {
+            it('should get all warnings', (done) => {


Remove, because this is a synchronous test (ensured via PluginEnvironment). This should not use a callback. or return a promise. This will also help to make sure the parallel feature being added does not break this expected behavior.

You can instead basically replicate this test inside parallel-option.test.js - this way we can have coverage for both single-core and multicore environments.

hulkish · 2017-07-20T00:17:56Z

            });

-            it('should get no warnings', () => {
+            it('should get no warnings', (done) => {


Please remove all of these for any existiing tests. So that we can keep visibility on the synchronous behavior.

hulkish · 2017-07-20T00:18:42Z

            files: ['test', 'test.js'],
          }], () => {
            expect(Object.keys(compilation.assets).length).toBe(4);
+            done();


Remove these too, please.

hulkish · 2017-07-20T00:23:25Z

    compilerEnv.context = '';

    const plugin = new UglifyJsPlugin({
+      parallel: { cache: false, workers: 0 },


This one is ok, because it's helpful to test this against es2015 feature.

hulkish · 2017-07-20T00:23:38Z

    compilerEnv.context = '';

    const plugin = new UglifyJsPlugin({
+      parallel: { cache: false, workers: 0 },


Remove please.

hulkish · 2017-07-20T00:39:29Z

  it('matches snapshot', () => {
    const compiler = createCompiler();
-    new UglifyJsPlugin().apply(compiler);
+    new UglifyJsPlugin({ parallel: { cache: false } }).apply(compiler);


Please remove this change here and just make a new test inside parallel-option.test.ja. Something like:

it('matches snapshot', () => { const compiler = createCompiler(); new UglifyJsPlugin({ parallel: { cache: false, workers: 0 } }).apply(compiler); return compile(compiler).then((stats) => { const errors = stats.compilation.errors.map(cleanErrorStack); const warnings = stats.compilation.warnings.map(cleanErrorStack); expect(errors).toMatchSnapshot('errors'); expect(warnings).toMatchSnapshot('warnings'); for (const file in stats.compilation.assets) { if (Object.prototype.hasOwnProperty.call(stats.compilation.assets, file)) { expect(stats.compilation.assets[file].source()).toMatchSnapshot(file); } } }); });

mikesherov

I think we're finally ready to land this!

hulkish

Looks great, good stuff.

hulkish · 2017-07-20T14:05:24Z

          }], () => {
            expect(compilation.errors.length).toBe(1);
            expect(compilation.errors[0]).toBeInstanceOf(Error);
-            expect(compilation.errors[0].message).toEqual(expect.stringContaining('TypeError'));


Just making note of this for when #80 is rebased on top of this. This should remain a TypeError - but I think the options schema validation should cover this. Nothing needing done for this pr i believe.

michael-ciniawsky

@aui Thx very much fo putting up this PR, this is very good work :)

mikesherov · 2017-07-20T23:27:22Z

Wooo!

scott-thrillist · 2017-07-21T17:00:36Z

Benchmarks vs webpack-parallel-uglify-plugin

webpack-parallel-uglify-plugin
23.18 seconds
22.61 seconds
22.19 seconds
22.51 seconds

uglifyjs-webpack-plugin
25.96 seconds
18.41 seconds
16.14 seconds
16.18 seconds

uglifyjs-webpack-plugin (parallel: false)
51.89 seconds

👍 👍 👍

aui added 8 commits July 8, 2017 13:51

refactor: minify()

b9ff34b

add: Use multi-process and cache to speed up the build

43208ec

add: maxWorkers、cache、cacheDirectory

754037c

test: Disable caching

f3f0656

Use multi-process and cache to speed up the build

04ea42b

fix: cache bug

7e9c080

test: Disable caching

acd748b

test: uglify

aca16f4

michael-ciniawsky changed the title ~~Improve the build speed~~ perf: improve the build speed Jul 9, 2017

michael-ciniawsky added this to the 1.0.0 milestone Jul 9, 2017

michael-ciniawsky added priority: 3 (required) pr_action: Rebase status: Pending pr_action: Review semver: Minor type: Feature and removed pr_action: Rebase labels Jul 9, 2017

michael-ciniawsky requested review from hulkish and michael-ciniawsky July 9, 2017 15:55

michael-ciniawsky suggested changes Jul 9, 2017

View reviewed changes

michael-ciniawsky assigned hulkish and michael-ciniawsky Jul 9, 2017

michael-ciniawsky changed the title ~~perf: improve the build speed~~ perf: add support parallelization && caching (options.parallel`) Jul 9, 2017

michael-ciniawsky changed the title ~~perf: add support parallelization && caching (options.parallel`)~~ perf: add support for parallelization && caching (options.parallel) Jul 9, 2017

michael-ciniawsky suggested changes Jul 9, 2017

View reviewed changes

aui added 4 commits July 19, 2017 10:03

refactor: rename variable

29ec4b1

test: remove private API

7051b8e

refactor: safeguard to not spawn more workers

cef8262

�add: options.parallel

3a68612

samouri mentioned this pull request Jul 19, 2017

Build: Remove Custom Bundler Automattic/wp-calypso#16379

Closed

6 tasks

hulkish suggested changes Jul 19, 2017

View reviewed changes

hulkish suggested changes Jul 20, 2017

View reviewed changes

aui added 2 commits July 20, 2017 11:05

test: clean up

3aebef6

refactor: buildError

e53ae99

mikesherov approved these changes Jul 20, 2017

View reviewed changes

hulkish approved these changes Jul 20, 2017

View reviewed changes

alexander-akait approved these changes Jul 20, 2017

View reviewed changes

hulkish reviewed Jul 20, 2017

View reviewed changes

michael-ciniawsky approved these changes Jul 20, 2017

View reviewed changes

michael-ciniawsky added pr_action: Merge ready status: Approved and removed priority: 3 (required) pr_action: Review status: Pending labels Jul 20, 2017

michael-ciniawsky merged commit ee16639 into webpack-contrib:master Jul 20, 2017

michael-ciniawsky removed the pr_action: Merge ready label Jul 20, 2017

viankakrisna mentioned this pull request Jul 25, 2017

add WebpackUglifyParallel react/create-react-app#2799

Closed

zjlovezj mentioned this pull request Aug 9, 2017

面试分享：一年经验初探阿里巴巴前端社招 jawil/blog#22

Open

michael-ciniawsky removed this from the 2.0.0 milestone Oct 29, 2017

aui mentioned this pull request Dec 5, 2017

[前端招聘] 在海边写代码 aui/blog#2

Closed

snyk-bot mentioned this pull request Jun 2, 2020

[Snyk] Security upgrade serialize-javascript from 1.5.0 to 3.1.0 A-Mikhail/uglifyjs-webpack-plugin#2

Open

		@@ -0,0 +1,108 @@
		import uglify from 'uglify-es';

		const defaultUglifyOptions = {

		@@ -0,0 +1,46 @@
		const toType = value => (Object.prototype.toString.call(value).slice(8, -1));

Uh oh!

Uh oh!

Conversation

aui commented Jul 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsf-clabot commented Jul 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michael-ciniawsky commented Jul 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky commented Jul 9, 2017

Uh oh!

hulkish commented Jul 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aui Jul 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikesherov commented Jul 9, 2017

Uh oh!

michael-ciniawsky commented Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hulkish Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hulkish Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hulkish Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hulkish Jul 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

aui commented Jul 9, 2017 •

edited

Loading

jsf-clabot commented Jul 9, 2017 •

edited

Loading

michael-ciniawsky commented Jul 9, 2017 •

edited

Loading

aui Jul 11, 2017 •

edited

Loading

michael-ciniawsky commented Jul 19, 2017 •

edited

Loading

hulkish Jul 19, 2017 •

edited

Loading

hulkish Jul 19, 2017 •

edited

Loading

hulkish Jul 19, 2017 •

edited

Loading

hulkish Jul 20, 2017 •

edited

Loading

hulkish Jul 20, 2017 •

edited

Loading

hulkish Jul 20, 2017 •

edited

Loading

hulkish Jul 20, 2017 •

edited

Loading

hulkish Jul 20, 2017 •

edited

Loading

hulkish Jul 20, 2017 •

edited

Loading

hulkish Jul 20, 2017 •

edited

Loading

hulkish Jul 20, 2017 •

edited

Loading