Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

convert void 0 to a new var #2912

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

Skalman
Copy link
Contributor

@Skalman Skalman commented Feb 12, 2018

I have sprinkled the code with lots of TODOs and questions. The basic implementation works, using only within-same-function transforms as proposed by @kzc.

  • According to the benchmark:
    • Average uglified size improved by ~0.16%
    • Average gzip size worsened by ~0.17%

Do these results mean that we should abandon this idea? If we want this transform, it shouldn't be enabled by default, and information needs to be added to the README.

Results from node test/benchmark.js
BEFORE THIS CHANGE

https://code.jquery.com/jquery-3.2.1.js

  • parse: 1.107s
  • rename: 0.769s
  • compress: 8.914s
  • scope: 0.274s
  • mangle: 0.640s
  • properties: 0.000s
  • output: 0.539s
  • total: 12.243s

Original: 268039 bytes
Uglified: 86074 bytes
GZipped: 30058 bytes
SHA1 sum: 3e16e5ec39f60f779cf1cb407dcc0fb93c1216ab

https://cdnjs.cloudflare.com/ajax/libs/angular.js/1.6.4/angular.js

  • parse: 2.029s
  • rename: 1.359s
  • compress: 15.630s
  • scope: 0.598s
  • mangle: 1.171s
  • properties: 0.000s
  • output: 0.562s
  • total: 21.349s

Original: 1249863 bytes
Uglified: 173616 bytes
GZipped: 59988 bytes
SHA1 sum: 87f388ccba07cb8505ac03ff75cadb235c6a437a

https://cdnjs.cloudflare.com/ajax/libs/mathjs/3.9.0/math.js

  • parse: 4.141s
  • rename: 2.746s
  • compress: 23.607s
  • scope: 0.304s
  • mangle: 0.559s
  • properties: 0.000s
  • output: 0.430s
  • total: 31.788s

Original: 1590107 bytes
Uglified: 466740 bytes
GZipped: 118842 bytes
SHA1 sum: f8502b2990e245757a06d53ad670aaa710806e66

https://maxcdn.bootstrapcdn.com/bootstrap/3.3.7/js/bootstrap.js

  • parse: 0.524s
  • rename: 0.373s
  • compress: 3.588s
  • scope: 0.167s
  • mangle: 0.283s
  • properties: 0.000s
  • output: 0.222s
  • total: 5.157s

Original: 69707 bytes
Uglified: 36836 bytes
GZipped: 9582 bytes
SHA1 sum: 31b3a3cb53ed8dd5e92519891b92a60eb07babfc

https://unpkg.com/[email protected]/dist/react.js

  • parse: 1.962s
  • rename: 1.611s
  • compress: 14.967s
  • scope: 0.407s
  • mangle: 0.788s
  • properties: 0.000s
  • output: 0.501s
  • total: 20.237s

Original: 701412 bytes
Uglified: 205174 bytes
GZipped: 62273 bytes
SHA1 sum: 7a8ad6431a54c84e476a1a87aeba0a80a6f1dd06

http://builds.emberjs.com/tags/v2.11.0/ember.prod.js

  • parse: 4.303s
  • rename: 3.194s
  • compress: 19.328s
  • scope: 0.374s
  • mangle: 0.676s
  • properties: 0.000s
  • output: 0.790s
  • total: 28.665s

Original: 1852178 bytes
Uglified: 526064 bytes
GZipped: 129050 bytes
SHA1 sum: 22037ab14a33379b2ee9ba9270ce22e4ee970a2c

https://cdn.jsdelivr.net/lodash/4.17.4/lodash.js

  • parse: 1.183s
  • rename: 0.735s
  • compress: 14.983s
  • scope: 0.286s
  • mangle: 0.534s
  • properties: 0.000s
  • output: 0.401s
  • total: 18.122s

Original: 539590 bytes
Uglified: 69578 bytes
GZipped: 24877 bytes
SHA1 sum: 025d83a7526aa44dd4a175a5fabc3ef7c6992967

https://cdnjs.cloudflare.com/ajax/libs/d3/4.5.0/d3.js

  • parse: 2.478s
  • rename: 2.393s
  • compress: 18.957s
  • scope: 0.398s
  • mangle: 0.854s
  • properties: 0.000s
  • output: 0.681s
  • total: 25.761s

Original: 451131 bytes
Uglified: 211601 bytes
GZipped: 71051 bytes
SHA1 sum: 1636ad62c5338200bb99eb57fa3fd39f882502d2

https://raw.githubusercontent.com/kangax/html-minifier/v3.5.7/dist/htmlminifier.js

  • parse: 3.678s
  • rename: 3.485s
  • compress: 22.196s
  • scope: 0.302s
  • mangle: 0.647s
  • properties: 0.000s
  • output: 0.489s
  • total: 30.797s

Original: 1063075 bytes
Uglified: 508164 bytes
GZipped: 158013 bytes
SHA1 sum: ad6e5c94ba84dc6cb0227b57504f4c2e3abac535

WITH THIS CHANGE

https://code.jquery.com/jquery-3.2.1.js

  • parse: 1.204s
  • rename: 0.644s
  • compress: 8.808s
  • scope: 0.388s
  • mangle: 0.740s
  • properties: 0.000s
  • output: 0.478s
  • total: 12.263s

Original: 268039 bytes
Uglified: 85768 bytes
GZipped: 30113 bytes
SHA1 sum: 5a7980c8fc4be9cda00bdf23457b336d728021ff

https://cdnjs.cloudflare.com/ajax/libs/angular.js/1.6.4/angular.js

  • parse: 1.966s
  • rename: 1.255s
  • compress: 16.213s
  • scope: 0.789s
  • mangle: 1.462s
  • properties: 0.000s
  • output: 0.928s
  • total: 22.613s

Original: 1249863 bytes
Uglified: 173393 bytes
GZipped: 60045 bytes
SHA1 sum: 6cc222e3f8bc88192bba299f3504ccc3959fa940

https://cdnjs.cloudflare.com/ajax/libs/mathjs/3.9.0/math.js

  • parse: 4.036s
  • rename: 2.125s
  • compress: 26.278s
  • scope: 0.318s
  • mangle: 0.650s
  • properties: 0.000s
  • output: 0.508s
  • total: 33.916s

Original: 1590107 bytes
Uglified: 466184 bytes
GZipped: 119122 bytes
SHA1 sum: 660b9e954b1f85ba742e964bbb35ebad167664e3

https://maxcdn.bootstrapcdn.com/bootstrap/3.3.7/js/bootstrap.js

  • parse: 0.625s
  • rename: 0.336s
  • compress: 2.977s
  • scope: 0.105s
  • mangle: 0.279s
  • properties: 0.000s
  • output: 0.224s
  • total: 4.546s

Original: 69707 bytes
Uglified: 36830 bytes
GZipped: 9580 bytes
SHA1 sum: d6c3dab88ac5f13c1f5dd20c232b60e3efbe237b

https://unpkg.com/[email protected]/dist/react.js

  • parse: 2.177s
  • rename: 1.265s
  • compress: 15.991s
  • scope: 0.350s
  • mangle: 1.673s
  • properties: 0.000s
  • output: 0.803s
  • total: 22.259s

Original: 701412 bytes
Uglified: 204937 bytes
GZipped: 62343 bytes
SHA1 sum: a76418a746a313e3eab3db474811dbf280758922

http://builds.emberjs.com/tags/v2.11.0/ember.prod.js

  • parse: 3.680s
  • rename: 3.006s
  • compress: 21.636s
  • scope: 0.365s
  • mangle: 0.745s
  • properties: 0.000s
  • output: 0.481s
  • total: 29.913s

Original: 1852178 bytes
Uglified: 524078 bytes
GZipped: 129669 bytes
SHA1 sum: 1f19d7f41160f151fca54afe903b0a34d3098f1c

https://cdn.jsdelivr.net/lodash/4.17.4/lodash.js

  • parse: 1.173s
  • rename: 0.805s
  • compress: 14.316s
  • scope: 0.243s
  • mangle: 0.665s
  • properties: 0.000s
  • output: 0.444s
  • total: 17.646s

Original: 539590 bytes
Uglified: 69578 bytes
GZipped: 24877 bytes
SHA1 sum: 025d83a7526aa44dd4a175a5fabc3ef7c6992967

https://cdnjs.cloudflare.com/ajax/libs/d3/4.5.0/d3.js

  • parse: 2.650s
  • rename: 2.019s
  • compress: 21.041s
  • scope: 0.415s
  • mangle: 0.923s
  • properties: 0.000s
  • output: 0.583s
  • total: 27.631s

Original: 451131 bytes
Uglified: 211569 bytes
GZipped: 71062 bytes
SHA1 sum: 5c47a60b136f38376fcadff4129f0254ebca0d5e

https://raw.githubusercontent.com/kangax/html-minifier/v3.5.7/dist/htmlminifier.js

  • parse: 3.437s
  • rename: 2.979s
  • compress: 24.390s
  • scope: 0.312s
  • mangle: 0.657s
  • properties: 0.000s
  • output: 0.532s
  • total: 32.307s

Original: 1063075 bytes
Uglified: 507867 bytes
GZipped: 158078 bytes
SHA1 sum: 87d89bfcd4d713ed3e91f9bfecd7dc86c59cd00b

fixes #2585

@alexlamsl
Copy link
Collaborator

The uglified numbers look positive for starters.

I suspect the worse gzipped results are (partly) due to mangle – one way to assess is to run without it:

node test/benchnark.js -c

I haven't looked at the code yet as I'm about to doze off – will get back to you on that.

Thanks for working on this 👍

@Skalman
Copy link
Contributor Author

Skalman commented Feb 12, 2018

I think disabling mangling doesn't change the gzip results (~0.17% worse, like before). I'm also tired, so I might be mistaken.

Here's an idea that probably doesn't change anything, but could.

// with this transform
var existing=1,variables=2,undefined; ...

// idea
var undefined,existing=1,variables=2;...

@alexlamsl
Copy link
Collaborator

alexlamsl commented Feb 13, 2018

Re-reading #2585 (comment) it claims to produce post-gzip improvements, which is not observed anywhere (except bootstrap.js) - so I'm not sure how that was done.

@alexlamsl
Copy link
Collaborator

Here's a more restricted version which never introduces new variables, i.e. only group void 0s into an existing variable which happens to be undefined:

--- a/lib/compress.js
+++ b/lib/compress.js
@@ -58,6 +58,7 @@ function Compressor(options, false_by_default) {
         evaluate      : !false_by_default,
         expression    : false,
         global_defs   : {},
+        group_voids   : !false_by_default,
         hoist_funs    : false,
         hoist_props   : !false_by_default,
         hoist_vars    : false,
@@ -196,6 +197,11 @@ merge(Compressor.prototype, {
                 }
             }
         }
+        if (this.option("group_voids")) {
+            node.figure_out_scope(mangle);
+            node.reset_opt_flags(this);
+            node.group_voids(this);
+        }
         if (this.option("expression")) {
             node.process_expression(false);
         }
@@ -270,6 +276,28 @@ merge(Compressor.prototype, {
         return this.TYPE == node.TYPE && this.print_to_string() == node.print_to_string();
     });

+    AST_Toplevel.DEFMETHOD("group_voids", function(compressor) {
+        var void_sym;
+        this.transform(new TreeTransformer(function(node, descend) {
+            if (node instanceof AST_Scope) {
+                var old_sym = void_sym;
+                node.variables.each(function(def) {
+                    if (old_sym !== void_sym) return;
+                    var fixed = def.orig[0].fixed_value();
+                    if (fixed && is_undefined(fixed, compressor)) {
+                        void_sym = def.orig[0];
+                    }
+                });
+                descend(node, this);
+                void_sym = old_sym;
+                return node;
+            }
+            if (void_sym && is_undefined(node, compressor)) {
+                return make_node(AST_SymbolRef, void_sym, void_sym);
+            }
+        }));
+    });
+
     AST_Scope.DEFMETHOD("process_expression", function(insert, compressor) {
         var self = this;
         var tt = new TreeTransformer(function(node) {

This only makes a difference to math.js, but even so gzip still isn't a win:

node test/benchmark -mc group_voids=false

https://cdnjs.cloudflare.com/ajax/libs/mathjs/3.9.0/math.js
- parse: 0.875s
- rename: 0.562s
- compress: 5.350s
- scope: 0.203s
- mangle: 0.328s
- properties: 0.000s
- output: 0.234s
- total: 7.552s

Original: 1590107 bytes
Uglified: 466736 bytes
GZipped:  118836 bytes
SHA1 sum: 39105dfb8a9231469b83993c388735b9655cab2d

node test/benchmark -mc group_voids=true

https://cdnjs.cloudflare.com/ajax/libs/mathjs/3.9.0/math.js
- parse: 0.921s
- rename: 0.579s
- compress: 5.531s
- scope: 0.156s
- mangle: 0.328s
- properties: 0.000s
- output: 0.235s
- total: 7.750s

Original: 1590107 bytes
Uglified: 466571 bytes
GZipped:  118888 bytes
SHA1 sum: 6a6435c980dea291bd5b07bd1a34c131b9d19a38

So I reckon the initial analysis is flawed, which is sadly quite often the case when it comes to mangle and gzip.

At least it gives you an idea how scan and replace the AST with minimal number of rounds 😉

@Skalman
Copy link
Contributor Author

Skalman commented Feb 16, 2018

My idea to place the undefined variable first in the var declaration produces slightly worse results. I've tried a few variants to see if the placement of the new undefined declaration can make the gzip gods happier.

// Last in the first var (my initial implementation)
// Gzip worse by 0.174%
var a, b, undefined; var c, d; var e, f;

// First in the first var
// Gzip worse by 0.189%
var undefined, a, b; var c, d; var e, f;

// Last in the last var
// Gzip worse by 0.145%
var a, b; var c, d; var e, f, undefined;

// First in the last var
// Gzip worse by 0.158%
var a, b; var c, d; var undefined, e, f;

@alexlamsl's idea of never introducing new variables has a tiny effect compared to introducing new variables, so I don't think it would be worth it. If we're implementing any of these, the "last in the last var" should be used, since it produces the least bad gzip.

I also think we should benchmark introducing a var before deciding what to do.

@Skalman
Copy link
Contributor Author

Skalman commented Feb 16, 2018

Not creating a new var (like my initial impementation) improves uglification by 0.159% (as reported in my initial comment).

// Last in the last var (best from my comment above)
// Uglify better by 0.159%
// Gzip worse by 0.145%


// Last in the last var + create new `var` last in function if there is no existing `var`
// Uglify better by 0.165%
// Gzip worse by 0.162%


// Last in the last var + create new `var` last in function if there is no existing `var` only if there are more than two `void 0` uses
// Uglify better by 0.171%
// Gzip worse by 0.152%


// Last in the last var + create new `var` first in function if there is no existing `var`
// Uglify better by 0.164%
// Gzip worse by 0.152%


// Last in the last var + create new `var` first in function if there is no existing `var` only if there are more than two `void 0` uses
// Uglify better by 0.171%
// Gzip worse by 0.148%

Idea: introduce new var if there are more than three void 0 uses. 😄

@alexlamsl
Copy link
Collaborator

Introducing new variables tend to cause cascading effects on variable name assignments within mangle, which is why such a large gzip delta can be observed with a seemingly small change.

I haven't come up with any better ideas to try yet, but then we are in no great hurry either 😉

@alexlamsl
Copy link
Collaborator

And yes, putting that new variable first in the first var would shift the names of all subsequent declarations in the scope, so has a bigger effect than a trailing declaration (still not zero since it would cause intra-scope disruptions).

@alexlamsl
Copy link
Collaborator

FWIW, inline>1 also had to fight with gzip regressions back then via various tricks and hacks to make it profitable.

@Skalman
Copy link
Contributor Author

Skalman commented Feb 18, 2018

I've done a bunch of experiments varying how many voids are required before they are replaced. In all these experiments a new variable to an existing var is placed last, and a new var is placed first (which the above experiments show that Gzip likes best). I've compared the sum(uglified) with sum(gzip) for the benchmark test, not using a geometric mean (which might be more mathematically rigorous). In no case is the Gzip improved. 😞

I can't think of another way to improve the Gzip without the new variable crossing scopes. Where does that leave us? Abandon this project or include it anyway defaulted off?

If we include it, it should have as large an uglify improvement compared to the gzip worsening, while still making a significant difference. Choosing from the options below, I'd go for either {1, 2} or {2, 2}. What do you think?

Add variable to `var` when it replaces Add new `var` when it replaces Uglify improvement Gzip worsening Uglify-Gzip factor
1 void never 0.159 % 0.145 % 1.10
1 void 1 void 0.164 % 0.152 % 1.08
1 void 2 voids 0.171 % 0.148 % 1.16
1 void 3 voids 0.167 % 0.144 % 1.16
2 voids 2 voids 0.117 % 0.072 % 1.63
2 voids 3 voids 0.113 % 0.069 % 1.64
2 voids never 0.106 % 0.068 % 1.56
3 voids 3 voids 0.075 % 0.041 % 1.83
3 voids never 0.067 % 0.04 % 1.68
5 voids 5 voids 0.03 % 0.01 % 3
5 voids never 0.026 % 0.01 % 2.6

(Assuming I haven't messed up the implementation or the numbers.)

@alexlamsl
Copy link
Collaborator

IMHO the numbers look rather unappealing as it stands - I'd be happier if the uglified gain is much larger than the gzip loss.

@alexlamsl
Copy link
Collaborator

I think part of the issue here is with mangle, c.f. #2219

Introducing cross-scope variables for this feature probably won't do a lot of good, since it'd mess with variable name assignments pretty much the same way.

@alexlamsl
Copy link
Collaborator

@Skalman now that #2948 is in, please have another go at this and see if your previous strategies would be more compressible with the new mangle.

@Skalman
Copy link
Contributor Author

Skalman commented Feb 25, 2018

Sure, I'll have a look in a few days.

@Skalman
Copy link
Contributor Author

Skalman commented Mar 7, 2018

@Skalman now that #2948 is in, please have another go at this and see if your previous strategies would be more compressible with the new mangle.

I tried rebasing against the current master, but the numbers are no better.

A solution that'd likely improve also the Gzip numbers would be to do what @alexlamsl did in #2945. It should be possible to implement that during the compress stage, before mangle.

This would introduce captured variables. @kzc believes that this would cause runtime performance issue, and intuitively I'd agree. @alexlamsl, is this incorrect?

@alexlamsl
Copy link
Collaborator

@Skalman as I mentioned in #2945 (comment), no measurable performance difference is observed through JetStream.

In general we need something like 10x slowdown across a majority number of platforms to justify worrying about it, as JavaScript runtimes will receive continuous speed-ups and tuning against common code snippets with time.

@alexlamsl
Copy link
Collaborator

I'm actually surprised the numbers don't improve with the latest mangle - I'll take another look at the changes in this PR after some sleep to see what I have missed.

@alexlamsl
Copy link
Collaborator

alexlamsl commented Mar 8, 2018

Hmm - looks like the upper-level variables still don't like to be kicked around by those newly introduced void 0 substitutes:

--- a/lib/compress.js
+++ b/lib/compress.js
@@ -59,6 +59,7 @@ function Compressor(options, false_by_default) {
         evaluate      : !false_by_default,
         expression    : false,
         global_defs   : {},
+        group_voids   : false,
         hoist_funs    : false,
         hoist_props   : !false_by_default,
         hoist_vars    : false,
@@ -200,6 +201,9 @@ merge(Compressor.prototype, {
         if (this.option("expression")) {
             node.process_expression(false);
         }
+        if (this.option("group_voids")) {
+            node.group_voids(this);
+        }
         return node;
     },
     info: function() {
@@ -316,6 +320,79 @@ merge(Compressor.prototype, {
         self.transform(tt);
     });

+    AST_Scope.DEFMETHOD("get_void_var", function(compressor) {
+        var void_var;
+        this.variables.each(function(def) {
+            if (void_var) return;
+            var value = def.orig[0].fixed_value();
+            if (value && is_undefined(value, compressor)) {
+                void_var = def.orig[0];
+            }
+        });
+        if (!void_var) {
+            void_var = make_node(AST_SymbolVar, this, {
+                name: this.make_var_name("undefined"),
+                scope: this
+            });
+            this.enclosed.push(this.def_variable(void_var));
+            var var_stat;
+            this.walk(new TreeWalker(function(node) {
+                if (node instanceof AST_Scope && this.parent()) return true;
+                if (node instanceof AST_Var) {
+                    var_stat = node;
+                    return true;
+                }
+            }));
+            if (!var_stat) {
+                var_stat = make_node(AST_Var, this, {
+                    definitions: []
+                });
+                this.body.push(var_stat);
+            }
+            var_stat.definitions.push(make_node(AST_VarDef, this, {
+                name: void_var,
+                value: null
+            }));
+        }
+        return void_var;
+    });
+
+    AST_Toplevel.DEFMETHOD("group_voids", function(compressor) {
+        var scope, void_var;
+        this.transform(new TreeTransformer(function(node, descend) {
+            if (node instanceof AST_Scope) {
+                var save_scope = scope;
+                var save_var = void_var;
+                scope = node;
+                void_var = null;
+                descend(node, this);
+                void_var = save_var;
+                scope = save_scope;
+                return node;
+            }
+            if (is_undefined(node, compressor)) {
+                if (!void_var) void_var = scope.get_void_var(compressor);
+                return make_node(AST_SymbolRef, node, void_var);
+            }
+        }));
+    });
+
+    AST_Toplevel.DEFMETHOD("group_voids_cross_scope", function(compressor) {
+        var self = this, scope, void_var;
+        this.transform(new TreeTransformer(function(node, descend) {
+            if (node instanceof AST_Scope && node.parent_scope === self) {
+                scope = node;
+                descend(node, this);
+                scope = null;
+                void_var = null;
+                return node;
+            }
+            if (scope && is_undefined(node, compressor)) {
+                if (!void_var) void_var = scope.get_void_var(compressor);
+                return make_node(AST_SymbolRef, node, void_var);
+            }
+        }));
+    });
+
     (function(def){
         def(AST_Node, noop);

And yes, if you swap node.group_voids(this); with node.group_voids_cross_scope(this); you get better uglified and gzipped results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

void 0 can be optimized to an unassigned variable
2 participants