big perf regression in collapse_vars

[kzc]

Bug report or feature request?

perf regression

JavaScript input

$ bin/uglifyjs -V
uglify-js 3.4.0

$ curl -s https://raw.githubusercontent.com/mlwilkerson/terser-collapse-vars-perf-regression/master/index.js > slow.js

$ /usr/bin/time bin/uglifyjs slow.js -c collapse_vars=1 | wc -c
       38.35 real        38.41 user         0.07 sys
     286

$ /usr/bin/time bin/uglifyjs slow.js -c collapse_vars=0 | wc -c
        0.54 real         0.57 user         0.04 sys
     286

More details in original report: terser/terser#50

[kzc]

In the course of looking into this issue, I came across some odd behavior as illustrated by using this uglify-js 3.4.0 patch with the JS input below:

--- a/lib/compress.js
+++ b/lib/compress.js
@@ -1363,6 +1363,7 @@ merge(Compressor.prototype, {
                     extract_candidates(expr.consequent);
                     extract_candidates(expr.alternative);
                 } else if (expr instanceof AST_Definitions) {
+                    console.error("*** AST_Definitions expr:", expr.print_to_string());
                     expr.definitions.forEach(extract_candidates);
                 } else if (expr instanceof AST_DWLoop) {
                     extract_candidates(expr.condition);

$ echo 'function f(){var x=0; var o = {a:1, b:2}; console.log(x);}' | bin/uglifyjs -c
*** AST_Definitions expr: var x=0,o_a=1,o_b=2
*** AST_Definitions expr: var x=0,o_a=1,o_b=2
function f(){console.log(0)}

Notice that the properties of var o were hoisted into vars of their own despite o not being otherwise referenced. They're dropped by default compress of course, but it appears to be unnecessary work.

[kzc]

Possible collapse_vars speed fix - only consider the last 64 variable definition candidates. Passes all tests.

--- a/lib/compress.js
+++ b/lib/compress.js
@@ -1363,7 +1363,12 @@ merge(Compressor.prototype, {
                     extract_candidates(expr.consequent);
                     extract_candidates(expr.alternative);
                 } else if (expr instanceof AST_Definitions) {
-                    expr.definitions.forEach(extract_candidates);
+                    var len = expr.definitions.length;
+                    var i = len - 64;
+                    if (i < 0) i = 0;
+                    for (; i < len; i++) {
+                        extract_candidates(expr.definitions[i]);
+                    }
                 } else if (expr instanceof AST_DWLoop) {
                     extract_candidates(expr.condition);
                     if (!(expr.body instanceof AST_Block)) {

Also verified using smaller AST_Definitions candidate limits (2, 3, 4, etc) and confirmed that all the expect_stdout test results are correct (ignoring the generated code which would be different).

With patch above:

$ /usr/bin/time bin/uglifyjs slow.js -c
function alpha(){"use strict";var obj42={alpha:"42",beta:"42",gamma:[42,42,[],"42","42"]},obj43={alpha:"43",beta:"43",gamma:[43,43,[],"43","43"]},obj44={alpha:"44",beta:"44",gamma:[44,44,[],"44","44"]};console.log(obj42.alpha),console.log(obj43.alpha),console.log(obj44.alpha)}alpha();
        0.55 real         0.59 user         0.05 sys

[jack-guy]

Just wanted to call a little more attention to this issue - this has huge performance implications, especially for the folks over at @angular/cli. I went from being unable to complete the uglify portion of a build to done in 10 seconds with this patch. Thanks for the investigation and reporting @kzc :)

[kzc]

The proposed fix above was incorporated into Terser terser/terser#51 and will be in its next release.

[kzc]

No point keeping this ticket open.

[mlwilkerson]

Thanks for the quick fix on that, @kzc