add multiprocess support to uglify optimizer

[liucougar]

specify multiprocess in a build profile like the following: multiprocess: N,

where N is a number: 0 or other falsy value disables multiprocess (default) positive number would use that number of processes negative number would start one process for each CPU core

in addition to uglify-js 1.x, add support for 2.x

[liucougar]

in the patch, multiprocess is by default disabled, which is the backward compatible behavior.

however, i am leaning towards actually set the default to -1 (use as many processes as there are cpu cores)

in addition, by default uglify-js 2.x would generate a lot of warnings, like the following:

WARN: Dropping unused function argument gotoStart [/tp/front_end/dojo/fx.js:209,64]
WARN: Dropping unused function argument delay [/tp/front_end/dojo/fx.js:209,44]
WARN: Dropping unused function argument gotoEnd [/tp/front_end/dojo/fx.js:228,48]
WARN: Dropping unused function argument arg [/tp/front_end/dojo/fx.js:76,79]
WARN: Dropping unused function argument arg [/tp/front_end/dojo/fx.js:79,77]
WARN: Dropping unused function argument arg [/tp/front_end/dojo/fx.js:98,77]
WARN: Dropping unused function argument arg [/tp/front_end/dojo/fx.js:131,76]

do we want to by default disable showing warnings when using uglify-js 2.x? I vote for disable them (warning is enabled in the current patch)

[neonstalwart]

is there a reason not to use the existing maxOptimizationProcesses option instead of adding a new multiprocess option to the profile?

also, maybe we could adjust the architecture so that we centralize the code for handling multiple processes and then each type of optimizer could be controlled by that code rather than repeating the same concept in every optimizer.

[csnover]

If I had to guess, I’d say maxOptimizationProcesses was never documented so nobody knows it exists, which is why there is a new option here now.

IPC between Node.js children and Java is pretty different, and I don’t think that it’s worth the effort of trying to normalise how the two work.

[wkeese]

I tried the patch but no success running uglify. Worse yet the error messages aren't useful because they no longer list the file name. The commands I used were:

$ cd /ws/trunk/util
$ git checkout -b liucougar-uglify-multiprocess master
$  git pull git://github.com/liucougar/util.git uglify-multiprocess
$ npm install uglify-js
$ export NODE_PATH=/ws/trunk/util/node_modules
$ cd buildscripts
$ ./build.sh optimize=uglify release=true profile=standard

Tail of the output is:

WARN: Dropping side-effect-free statement [null:9,12]
WARN: Dropping side-effect-free statement [null:1,0]
WARN: Dropping side-effect-free statement [null:7,0]
WARN: Dropping side-effect-free statement [null:26,0]
{ message: 'Unexpected token: punc ({)',
  line: 2,
  col: 17,
  pos: 18,
  stack: 'Error\n    at new JS_Parse_Error (/ws/trunk/util/node_modules/uglify-js/lib/pars
...

Having said that, it doesn't work before the patch either, as hundreds of errors are reported. Seems like uglify 2 just doesn't work (or perhaps our code has lots of dodgy parts that aren't compatible with uglify 2). A snippet of the error messages from before the patch are:

error(356) The optimizer threw an exception; the module probably contains syntax errors. module identifier: dojo/store/Observable; exception: TypeError: object is not a function
error(356) The optimizer threw an exception; the module probably contains syntax errors. module identifier: dojo/store/api/Store; exception: TypeError: object is not a function
error(356) The optimizer threw an exception; the module probably contains syntax errors. module identifier: dojo/store/util/QueryResults; exception: TypeError: object is not a function
error(356) The optimizer threw an exception; the module probably contains syntax errors. module identifier: dojo/store/util/SimpleQueryEngine; exception: TypeError: object is not a function

[wkeese]

Thanks. OK, my problem was that my dojo directory had requirejs/ embedded in it as a submodule, which for some reason flummoxes uglify-js. I removed that and then benchmarked on my machine which has 8 processors (on one chip), as reported by require('os').cpus().length. Results were:

maxOptimizationProcesses       time
 1                             43s
 2                             30s
 4                             25s
 8                             21s
16                             21s
-1                             21s

[neonstalwart]

If I had to guess, I’d say maxOptimizationProcesses was never documented so nobody knows it exists, which is why there is a new option here now.

i see... well if it's doing the same thing then let's use the existing option. i've been using it for quite a while so i suspect there are probably other people who use it too and a new option that does the same thing doesn't make much sense

IPC between Node.js children and Java is pretty different, and I don’t think that it’s worth the effort of trying to normalise how the two work.

looking at build/transforms/optimizer/sendJob i don't immediately see why we couldn't fairly easily come up with some abstraction that eliminated the duplication of code but since i'm not volunteering to do it myself - it is what it is - i just thought i'd suggest looking into it.

also, based on @wkeese's results it looks like the default of maxOptimizationProcesses: 5 is a reasonable default.

[csnover]

also, based on @wkeese's results it looks like the default of maxOptimizationProcesses: 5 is a reasonable default.

I don’t understand this statement. Those test results show that using the same number of processes as number of CPUs results in the fastest optimizer time, as would be expected (-1). Not sure how “5” would be a reasonable default.

[liucougar]

just added another commit to change multiprocess to maxOptimizationProcesses.

currently, maxOptimizationProcesses is by default set to 5. i changed that to -1 in the new commit as well

trying to merge sendJob and this uglify_worker is not trivial: sendJob has to use a more general API to call java subprocesses, while uglify_worker uses a more friendly API to interact with nodejs subprocess.

[neonstalwart]

currently, maxOptimizationProcesses is by default set to 5. i changed that to -1 in the new commit as well

didn't you just break sendJob when the build is being run with java rather than node?

EDIT: ugh... nevermind, I shouldn't be reading code on a phone

[liucougar]

another commit to suppress warnings by default

if no-one has any more comments or objections, I'd like to land this tomorrow.

[neonstalwart]

i have no more objections

[wkeese]

No objections from me. I wish our code didn't have all those lint errors, but given that it does, suppressing the warnings seems like the practical route, at least for Dojo 1.x.

[liucougar]

fixed the issue csnover raised

[liucougar]

landed in e5994839cee5b8a1b1c917b37f53399fa751d1ab