That's a great hack! Never would have thought of that.

I think we'll hold off merging this as phantomjs is a pretty heavyweight dev dependency. We can apply this PR patch and test with it from time to time.

I had limited success running some of its tests locally against node. I could not find its source repo?? My google-fu is weak today.

By the way, when I run this PR against master with node test/jetstream.js on Mac it gets stuck at:

uglifyjs http://browserbench.org/JetStream/Octane2/mandreel.js

for a few minutes with 0% CPU.

wait... as I write this:

mandreel: ERROR: http://browserbench.org/JetStream/#quick:4: TypeError: undefined is not an object (evaluating '__suite.RunStep')
  T (http://localhost:8080/http%3A%2F%2Fbrowserbench.org%2FJetStream%2FJetStreamDriver.js:1)

  phantomjs://code/jetstream.js:29 in onError

Mind you, I don't know if it runs correctly on phantomjs without minification.

@alexlamsl Does it run to completion successfully on your machine? What's your OS and node version?

It does run successfully the whole day until just now when I was checking it out against #1450.

Then, as I was pulling my hair out, you just reported the exact same breakage but against master 😅

So I think that site might be under maintenance. You know, Murphy's Law.

you just reported the exact same breakage but against master

Doesn't necessarily mean master is correct either. ;-)

I just run against master and it works - please do try on your end if you have some spare CPU cycles.

mandreel.js is 5MB, and I was worried about my script not working when I first run it over that thing as well 😉

Doesn't necessarily mean master is correct either. ;-)

FYI, I'm running against all my unmerged PRs listed in #1383 - got stuck at #1450 as I mentioned earlier. But now that the quirk seems to have cleared itself, let me try again...

I think it's a time-out on the HTTP GET for that large file - guess I should handle the error event to make sure.

@kzc I've found the root cause - it's to do with me forgetting to flush stderr where bin/uglify-js is busily writing out those warnings by default.

So OSX / macOS have a shallower buffer than Windows 😜

@kzc please try again to see if this works?

I think we'll hold off merging this as phantomjs is a pretty heavyweight dev dependency. We can apply this PR patch and test with it from time to time.

Indeed I don't expect this to merge, at least not immediately. I'll add [WIP] for clarity.

With the latest change it now appears to work against master:

real	2m59.810s
user	2m52.183s
sys	0m7.563s

$ echo $?
0

That's pretty awesome.

The script test/jetstream.js can be checked in, but do not change package.json due to the size of phantomjs. Instead could the script check for the existence of node_modules/phantomjs-prebuilt and if not found print that npm install phantomjs-prebuilt@2.1.14 should be run first. Or just have the script simply run npm install phantomjs-prebuilt@2.1.14 first - it only wastes an additional 5 seconds if already installed. That way only the people who run it would incur the overhead of installing it.

That is a great idea - in fact what I did for html-minifier is to have a section in package.json called benchmarkDependencies and have the benchmark script pick those up and npm install on them.

But let's just hard-code this one dependency within test/jetstream.js for now 👍

@alexlamsl Your script works so well that I'm not sure uglify is really being run. ;-)

Could the script print uglify compress stats for each js file - original size and uglify size?

Or perhaps just save the original js files to test/jetstream/original/, and uglified files to test/jetstream/uglified/ so they can be manually inspected? Would probably need a .gitignore rule for test/jetstream/.

Right, let me do this npm install and add --stats and pipe out stderr, then I'll tackle writing out to the file system 😉

I did not know that uglifyjs had a --stats option...

Timing information (compressed 1 files):
- parse: 0.005s
- scope: 0.002s
- squeeze: 0.008s
- mangle: 0.000s
- generate: 0.001s

Having said that - it needs before/after size info and total uglify time!
:-P

Having said that - it needs before/after size info!

My thought exactly. Hint for a new PR has been well taken. 😜

Finished running this through all my PRs mentioned in #1383 (because it's easier to do git checkout then git clone, and I'm lazy at 5am...)

I'll go catch some sleep now.

Edit: I run this on #1460 as well

@alexlamsl FWIW, here are some test results with different compress options on Mac...

$ node -v
v6.9.0

        // tested against master 7f8d72d9d37396f2da05d5d824f74bd414c30119
        var uglifyjs = fork("bin/uglifyjs",
            [ "-mc", "warnings=false" ], // SUCCESS
            // [ "-mc", "passes=2,warnings=false" ], // SUCCESS
            // [ "-mc", "passes=3,warnings=false" ], // FAIL: mandreel: TypeError: undefined is not an object
            // [ "-mc", "collapse_vars,passes=2,warnings=false" ], // SUCCESS
            // [ "-mc", "collapse_vars,reduce_vars,warnings=false" ], // SUCCESS
            // [ "-mc", "collapse_vars,warnings=false" ], // SUCCESS
            // [ "-mc", "collapse_vars,passes=3,warnings=false" ], // UNKNOWN - script does not return
            // [ "-mc", "reduce_vars,warnings=false" ], // FAIL in pdfjs.js
            // [ "-mc", "collapse_vars,reduce_vars,passes=3,warnings=false" ], // FAIL in pdfjs.js
            // [ "-mc", "collapse_vars,reduce_vars,passes=2,warnings=false" ], // FAIL in pdfjs.js
            // [ "-mc", "collapse_vars,reduce_vars,warnings=false" ], // FAIL in pdfjs.js
            // [ "-mc", "reduce_vars,passes=2,warnings=false" ], // FAIL in pdfjs.js
            // [ "-mc", "unsafe,collapse_vars,reduce_vars,passes=2,warnings=false" ], // FAIL in pdfjs.js
            { silent: true }
        );

There might be genuine bugs in uglify in some compress options or some flakey I/O with node or phantomjs as some tests that failed above occasionally succeeded. Would be curious if you posted your results for Windows.

@kzc thanks for testing (as always) 👍

The socket time-outs (which is 2 minutes by default) is most likely the cause for mandreel.js failures. It has now been disabled.

The script can also take in custom cli options just like test/benchmark.js, e.g. node test/jetstream.js -mc collapse_vars,reduce_vars,unsafe,passes=3,sequences=1000000000

Edit: that particular set of options ends with Error: out of stack space for mandreel.js on master, but passes with #1460 🎉

Can you try running the options that led to the pdfjs.js failures?

Can you try running the options that led to the pdfjs.js failures?

Going through your list, one at a time 😎

Your errors may be different because you're running Chakra Node?

That's probably true, especially when it comes to things like out of stack space. I just managed to crash the pdf.js test with some complex set of options for -c, -m and -b BTW..

Perhaps pdf.js gets its input(s) in an async fashion that could fail. There may be a non-deterministic factor in there.

Could test/jetstream.js emit the uglify options at the beginning and end of the output?

Better still - instead of emitting:

uglifyjs http://browserbench.org/JetStream/SunSpiderPayload.js

What do you think about emitting the following?

curl http://browserbench.org/JetStream/SunSpiderPayload.js | uglifyjs -m -c ...options...

or emit:

uglifyjs JetStream/SunSpiderPayload.js -m -c ...options...

That last one is probably better in terms of reading on a (narrow?) console output window.

The parallel option is great for speed, but not so much for output. It's now jumbled.

The parallel option is great for speed, but not so much for output. It's now jumbled.

How so? By parallel I mean running multiple instances of test/jetstream.js, one in each console window. Or are you utilising some form of background task running in bash or something?

Perhaps I'm mistaken - I thought you'd see each command followed by the uglify stats?

uglifyjs Octane2/base.js -c unsafe,collapse_vars,reduce_vars,passes=3 -m -c warnings=false --stats
uglifyjs Octane2/code-load.js -c unsafe,collapse_vars,reduce_vars,passes=3 -m -c warnings=false --stats
Timing information (compressed 1 files):
- parse: 0.025s
- scope: 0.022s
- squeeze: 0.118s
- mangle: 0.002s
- generate: 0.011s
Timing information (compressed 1 files):
- parse: 0.030s
- scope: 0.020s
- squeeze: 0.054s
- mangle: 0.001s
- generate: 0.008s

Your new script runs in 3 minutes. The older one took 3:30.

Oh you mean multiple instances of bin/uglifyjs being run simultaneously under a single instance of test/Jetstream.js?

That is a zero-day "feature" due to the nature of Node.js async IO. Let me fix that by cumulating stdout and stderr instead of piping directly to parent process.

Oh you mean multiple instances of bin/uglifyjs being run simultaneously under a single instance of test/Jetstream.js?

I'm only running a single node test/jetstream.js command.

I'm only running a single node test/jetstream.js command.

Yup - but that one instance of test/Jetstream.js will spawn bin/uglifyjs asynchronously, whenever a HTTP GET request is made from phantomjs.

I'll now work to serialise the console output so as to improve readability.

stdout and stderr output is broken in early 6.x versions of node on UNIX upon error. Could you please start your benchmark scripts with:

// workaround for tty output truncation upon process.exit()
[process.stdout, process.stderr].forEach(function(stream){
    if (stream._handle && stream._handle.setBlocking)
        stream._handle.setBlocking(true);
});

as per https://github.com/mishoo/UglifyJS2/blob/master/tools/node.js#L1-L5

Now that I keep typing out those parameters, wouldn't -c !keep_fargs be a nice short-hand for -c keep_fargs=false? 😏

wouldn't -c !keep_fargs be a nice short-hand for -c keep_fargs=false? 😏

Perhaps when we replace yargs.

Instead of false I tend to use -c keep_fargs=0

Perhaps when we replace yargs.

That is actually on my TODO list - I've done some work utilising commander before.

Uh oh:

> node test\jetstream.js -b max_line_len=false
...
got result: 188.73446289999853
uglifyjs Octane2/code-load.js -b max_line_len=false --stats
Timing information (compressed 1 files):
- parse: 0.016s
- generate: 0.007s

uglifyjs Octane2/base.js -b max_line_len=false --stats
Timing information (compressed 1 files):
- parse: 0.013s
- generate: 0.013s

Fatal Windows exception, code 0xc0000005.
PhantomJS has crashed. Please read the bug reporting guide at
<http://phantomjs.org/bug-reporting.html> and file a bug report.
Error: JetStream failed!
   at Anonymous function (\test\jetstream.js:50:23)
   at emitTwo (events.js:106:5)
   at emit (events.js:191:7)
   at _handle.onexit (internal/child_process.js:215:7)

max_line_len is introduced in 8d233c3. I'm guessing this is a workaround for some browser bug, but I couldn't find any corresponding issues or PRs to be certain.

OT: #304 still an issue

Unrelated max_line_len issue aside, I think this PR is good to go. It's already proven its usefulness in finding bugs and helps to fill in the gaps in the unit tests. Well done!

A minor suggestion would be to print a success or fail message as the very last line of output. Perhaps something like "SUCCESS - 25/25 tests passed", or "FAIL - 23/25 tests passed".

Currently it will throw an error message of JetStream failed! upon any detection of failures, alongside a non-zero exit code. I'll add the success message now.

The non-zero error code upon failure is good as it will aid git bisect run and automated tests of different uglify options.