feat: add support for `parallelization` && `caching` (`options.parallel`) #77

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

package.json

    "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

package.json

-    "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

src/index.js

-      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

src/uglify/cache.js

+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

src/uglify/minify.js

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


buildCommentsFunction => buildComments || comments

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

src/uglify/minify.js

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


move the {Object} to L21 please :)

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

src/uglify/minify.js

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


buildDefaultUglifyOptions => buildOptions || defaults

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

src/uglify/index.js

+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

src/uglify/serialization.js

@@ -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

test/empty-options.test.js

@@ -16,14 +16,14 @@ describe('when applied with no options', () => {
    const compilerEnv = pluginEnvironment.getEnvironmentStub();
    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

test/es2015.test.js

@@ -17,6 +17,7 @@ describe('when applied with uglifyOptions.ecma', () => {
    });

    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

test/extract-option-set-to-a-single-file.test.js

@@ -11,6 +11,7 @@ describe('when applied with extract option set to a single file', () => {
    compilerEnv.context = '';

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


Same as #77 (comment)

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

test/invalid-options.test.js

@@ -10,6 +10,7 @@ describe('when applied with invalid options', () => {
  it('matches snapshot', () => {
    const compiler = createCompiler();
    new UglifyJsPlugin({
+      parallel: { cache: false, workers: 0 },


Same as #77 (comment)

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

test/invalid-options.test.js

    const pluginEnvironment = new PluginEnvironment();
    const compilerEnv = pluginEnvironment.getEnvironmentStub();
    compilerEnv.context = '';

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


Same as #77 (comment)

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

test/__snapshots__/invalid-options.test.js.snap

@@ -3,9 +3,9 @@
 exports[`errors 1`] = `
 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

test/__snapshots__/invalid-options.test.js.snap

  "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

test/all-options.test.js

@@ -17,6 +17,7 @@ describe('when applied with all options', () => {
    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

test/all-options.test.js

@@ -311,13 +325,14 @@ describe('when applied with all options', () => {
              compilationEventBindings = chunkPluginEnvironment.getEventBindings();
            });

-            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

test/all-options.test.js

@@ -365,28 +380,16 @@ describe('when applied with all options', () => {
              compilationEventBindings = chunkPluginEnvironment.getEventBindings();
            });

-            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

test/empty-options.test.js

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


Remove these too, please.

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

test/extract-comments-options.test.js

    const pluginEnvironment = new PluginEnvironment();
    const compilerEnv = pluginEnvironment.getEnvironmentStub();
    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

test/extract-comments-options.test.js

    const pluginEnvironment = new PluginEnvironment();
    const compilerEnv = pluginEnvironment.getEnvironmentStub();
    compilerEnv.context = '';

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


Remove please.

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

test/empty-options.test.js

    plugin.apply(compilerEnv);
    eventBindings = pluginEnvironment.getEventBindings();
  });

  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

test/empty-options.test.js

@@ -121,7 +123,7 @@ describe('when applied with no options', () => {
          }], () => {
            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 facebook/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

Open

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));

feat: add support for parallelization && caching (options.parallel) #77

feat: add support for parallelization && caching (options.parallel) #77

Conversation

aui commented Jul 9, 2017 • edited Loading

jsf-clabot commented Jul 9, 2017 • edited Loading

michael-ciniawsky commented Jul 9, 2017 • edited Loading

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

michael-ciniawsky commented Jul 9, 2017

hulkish commented Jul 9, 2017

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

aui Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikesherov commented Jul 9, 2017

michael-ciniawsky commented Jul 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hulkish Jul 19, 2017 • edited Loading

Choose a reason for hiding this comment

hulkish Jul 19, 2017 • edited Loading

Choose a reason for hiding this comment

hulkish Jul 19, 2017 • edited Loading

Choose a reason for hiding this comment

hulkish Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hulkish Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hulkish Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

hulkish Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

hulkish Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hulkish Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

hulkish Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

mikesherov left a comment

Choose a reason for hiding this comment

hulkish left a comment

Choose a reason for hiding this comment

hulkish Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

michael-ciniawsky left a comment

Choose a reason for hiding this comment

mikesherov commented Jul 20, 2017

scott-thrillist commented Jul 21, 2017 • edited Loading

feat: add support for `parallelization` && `caching` (`options.parallel`) #77

feat: add support for `parallelization` && `caching` (`options.parallel`) #77

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

scott-thrillist commented Jul 21, 2017 •

edited

Loading