fix `pure_funcs` & improve `side_effects` by alexlamsl · Pull Request #1494 · mishoo/UglifyJS

alexlamsl · 2017-02-17T08:01:54Z

Added some tests for pure_funcs in general while I'm at this.

kzc · 2017-02-17T14:42:08Z

test/compress/pure_funcs.js

+    }
+    expect: {
+        function f(a, b) {
+            console.log(a());


If console.log is deemed to be pure and its return value is not used, shouldn't the expected result should be the following?

function f(a, b) { a(); }

That's because AST_SimpleStatement isn't smart enough at the moment.

I'll investigate if this is the only site that needs improvement for AST_Call optimisation. Hopefully I won't end up with nasty surprises like I did in #1477.

@alexlamsl Could you please add a [WIP] prefix to the title while this is sorted out?

I think the real problem is that the concepts of "side effects" and "pure function" are used interchangeably in the code, when they are different things. As we see above, a pure function can have arguments with side effects.

That isn't an issue per se, because it makes sense when asking AST_Node.has_side_effects() to consider the expression as a whole and return true if any of its components have side effects.

Optimisation of individual side-effect-free components are scattered across compress.js, and in this particular case, I think OPT(AST_SimpleStatement) needs to be augmented as it's the site where we know the evaluated value of its body is being discarded.

kzc · 2017-02-17T15:26:34Z

Can you add an additional test like the following and add unused: true in the options?

function foo() {
    var u = pure(1);
    var x = pure(2);
    var y = pure(x);
    var z = pure(pure(side_effect()));
    return pure(3);
}

it ought to compress to:

function foo() {
    side_effect();
    return pure(3);
}

alexlamsl · 2017-02-17T17:42:47Z

@kzc added test as suggested above. Right now it will return:

function foo() {
    pure(pure(side_effects()));
    return pure(3);
}

I'm contemplating on whether to open a separate PR for the further optimisation or group it all in this one.

kzc · 2017-02-17T17:58:13Z

I'm contemplating on whether to open a separate PR for the further optimisation or group it all in this one.

Might as well put it into this one.

kzc · 2017-02-17T18:15:53Z

FYI, one of the most frequent uses of pure_funcs in the wild is for Babel generated _classCallCheck:

$ echo 'class Foo {}' | babel

"use strict";
function _classCallCheck(instance, Constructor) { if (!(instance instanceof Constructor)) { throw new TypeError("Cannot call a class as a function"); } }
var Foo = function Foo() {
  _classCallCheck(this, Foo);
};

That probably should be a test case.

alexlamsl · 2017-02-17T19:31:16Z

@kzc took me a while, but here it is 😅

Please check the babel test under test/compress/pure_funcs.js to see if that matches what you expect?

kzc · 2017-02-17T19:38:56Z

lib/compress.js

        return self;
    });

+    (function(def){


Can you please put a comment above this line?

// drop_value()

Done with an extra line to try and explain what it does 😉

kzc · 2017-02-17T19:50:35Z

test/compress/pure_funcs.js

    }
    expect: {
        function f(a, b) {
-            Math.floor(c / b);


Shouldn't the expected result be the following?

function f(a, b) { c; }

Since c is global then its access could throw if not defined.

$ cat foo.js "use strict"; try { c; } catch (x) { console.log("caught exception: ", x); }

$ node ./foo.js caught exception: ReferenceError: c is not defined

Just confirming - in ECMAScript there are no "divide by zero" exceptions, correct? It would produce Infinity or NaN or whatever so no issue removing the divisions here.

Ah! I forgot the undefined error case.

Thanks for catching that!

kzc · 2017-02-17T19:51:51Z

test/compress/pure_funcs.js

+            if (!(instance instanceof Constructor))
+                throw new TypeError("Cannot call a class as a function");
+        }
+


please remove this empty line

kzc · 2017-02-17T19:53:43Z

Nice job.

The babel test looks correct. Your other PR for -c toplevel could reduce it further I think?

I noted a different test result that I had issue with.

alexlamsl · 2017-02-17T19:55:22Z

I noted a different test result that I had issue with.

Thanks for catching that - I'm having a slow/distracted day 😓

kzc · 2017-02-17T20:00:09Z

test/compress/drop-unused.js

+    }
+    expect: {
+        function foo() {
+            var b;


Would have expected to see the global c here:

c; c;

kzc · 2017-02-17T20:00:51Z

test/compress/pure_funcs.js

+        }
+    }
+    expect: {
+        function f(a, b) {


kzc · 2017-02-17T20:11:42Z

@alexlamsl The speed and quality of your code continues to impress me.

alexlamsl · 2017-02-17T20:17:44Z

The babel test looks correct. Your other PR for -c toplevel could reduce it further I think?

I lumped this PR on top of #1485 locally to test this theory. For starters, even without any extra options, function Foo() { got turned into function() {.

With toplevel on, I just get a blank output 😏

(Conflicts with #1448 and #1450)

alexlamsl · 2017-02-17T20:41:27Z

test/jetstream.js with default options passed.

alexlamsl · 2017-02-17T21:36:27Z

Run into a curious issue with math.js where negate_iife is being undone - investigating.

alexlamsl · 2017-02-17T22:38:14Z

Employed similar logic as #1451 to fix it.

kzc · 2017-02-17T23:53:29Z

These unmerged PRs are completely inter-tangled now.

kzc · 2017-02-18T03:43:37Z

Bug:

$ echo 'pure(a() ? 4 : 5)' | bin/uglifyjs -c --pure-funcs pure
WARN: Retaining side-effect statement [-:1,0]
undefined:3121
        self.right = self.right.transform(tw);
                               ^

TypeError: Cannot read property 'transform' of null

I think a distinct test is needed for every node type listed in drop_value().

kzc · 2017-02-18T03:44:38Z

lib/compress.js

+            return null;
+        }
+
+        function trim(nodes, compressor, first_in_statement) {


Can you please comment what this function does and what it returns?

kzc · 2017-02-18T03:57:53Z

This warning is not needed - it is too common:

$ echo 'x() && y()' | bin/uglifyjs -c
WARN: Retaining side-effect statement [-:1,0]
x()&&y();

kzc · 2017-02-18T04:13:16Z

Not optimal:

$ echo '(1, [2, f()], 3, {a:1, b:g()});' | bin/uglifyjs -c
[2,f()],{a:1,b:g()};

should be:

f(),g();

alexlamsl · 2017-02-18T05:56:01Z

This warning is not needed - it is too common:

Done.

alexlamsl · 2017-02-18T05:57:07Z

Not optimal: (1, [2, f()], 3, {a:1, b:g()});

Oh, I was hoping OPT(AST_Seq) would pick up the slack of that case. I'll investigate.

alexlamsl · 2017-02-18T06:07:06Z

Bug: pure(a() ? 4 : 5)

Fixed.

kzc · 2017-02-18T06:28:32Z

lib/compress.js

            return null;
        }

+        // drop side-effect-free elements an array of expressions


// Drop side-effect-free elements from an array of expressions. // Returns an array of expressions with side-effects or null // if all elements were dropped. Note: original array may be // returned if nothing changed.

Updated - thanks! 👍

kzc · 2017-02-18T06:48:34Z

Even without pure functions, the AST_SimpleStatement drop_value() optimization seems to be a nice win.

alexlamsl · 2017-02-18T06:56:15Z

Not optimal: (1, [2, f()], 3, {a:1, b:g()});

Improved.

Even without pure functions, the AST_SimpleStatement drop_value() optimization seems to be a nice win.

This is similar to what I wanted to do in #1451 (comment)

kzc · 2017-02-18T07:00:06Z

lib/compress.js

+            });
+        });
+    })(function(node, func){
+        node.DEFMETHOD("drop_value", func);


Would it bother you to change the name of the function to something more descriptive like drop_irrelevant or drop_side_effect_free?

drop_side_effect_free sounds good as it reduces the vocabulary surface area 👍

kzc · 2017-02-18T07:03:27Z

(1, [2, f()], 3, {a:1, b:g()});

Would you mind adding that under a new sequences test? It shows multi-level simplification.

alexlamsl · 2017-02-18T07:05:48Z

Would you mind adding that under a new sequences test?

It is added under test/compress/drop-unused.js - or do you want me to rename the test case?

kzc · 2017-02-18T07:06:31Z

It is added under test/compress/drop-unused.js - or do you want me to rename the test case?

I must have missed it - it's fine. Thanks.

kzc · 2017-02-18T07:11:25Z

test/compress/drop-unused.js

+        (1, [2, foo()], 3, {a:1, b:bar()});
+    }
+    expect: {
+        foo(), bar();


👍 super_awesome++

kzc · 2017-02-18T07:24:13Z

This PR alone shaved 4,201 bytes off a 2MB app bundle I had kicking around. Not bad at all.

alexlamsl · 2017-02-18T07:38:07Z

test/benchmark.js

Code	Time		Size
	master	#1494	master	#1494
jquery-3.1.1.js	0.788s	0.836s	identical
angular.js	1.175s	1.174s	174,174	174,175
math.js	17.442s	17.792s	470,543	470,528
bootstrap.js	0.220s	0.235s	36,920	36,909
react.js	1.202s	1.297s	207,922	207,339
ember.prod.js	2.354s	2.463s	530,719	530,714
lodash.js	0.720s	0.685s	identical
d3.js	1.590s	1.653s	identical

alexlamsl · 2017-02-18T07:42:35Z

Oh Shoo—

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 (UglifyJS2\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)

alexlamsl · 2017-02-18T07:47:23Z

Seems like this PR is interfering with OutputStream(options.max_line_len). The reason why I suspect that is:

when I removed 0=== from math.js only 3 bytes was saved, diff shows there's an extra \n
that same exception is thrown in add benchmark & JetStream tests #1479 (comment) when I mess with ~~karma~~ max_line_len

alexlamsl · 2017-02-18T08:48:26Z

node test/jetstream.js -b beautify=false,max_line_len=16000 -mc warnings=false completed successfully.

- only drops side-effect-free arguments - drop side-effect-free parts with discarded value from `AST_Seq` & `AST_SimpleStatement` closes mishoo#1494

- only drops side-effect-free arguments - drop side-effect-free parts with discarded value from `AST_Seq` & `AST_SimpleStatement`

alexlamsl · 2017-02-19T17:36:57Z

lib/compress.js

+                operator: "||",
+                left: this.condition,
+                right: alternative
+            }) : this.condition.drop_side_effect_free(compressor);


condition was returned without optimised for side-effects before

alexlamsl · 2017-02-19T17:37:44Z

lib/compress.js

+            });
+            var node = this.clone();
+            node.consequent = consequent;
+            node.alternative = alternative;


These were accidentally modifying this instead of node before.

alexlamsl · 2017-02-19T17:38:25Z

test/compress/pure_funcs.js

+    }
+}
+
+conditional: {


Tests beefed up to cover aforementioned changes.

- only drops side-effect-free arguments - drop side-effect-free parts with discarded value from `AST_Seq` & `AST_SimpleStatement` closes mishoo#1494

kzc reviewed Feb 17, 2017

View reviewed changes

alexlamsl changed the title ~~fix pure_funcs with side effects arguments~~ [WIP] fix pure_funcs with side effects arguments Feb 17, 2017

kzc reviewed Feb 17, 2017

View reviewed changes

kzc reviewed Feb 18, 2017

View reviewed changes

alexlamsl mentioned this pull request Feb 18, 2017

clean up max_line_len #1496

Closed

alexlamsl force-pushed the pure_funcs branch from c7616d6 to 9ab10d9 Compare February 18, 2017 10:51

alexlamsl changed the title ~~[WIP] fix pure_funcs with side effects arguments~~ fix pure_funcs & improve side_effects Feb 18, 2017

alexlamsl mentioned this pull request Feb 18, 2017

Next release after v2.7.5 #1383

Closed

alexlamsl added a commit to alexlamsl/UglifyJS that referenced this pull request Feb 18, 2017

fix pure_funcs & improve side_effects

d079089

- only drops side-effect-free arguments - drop side-effect-free parts with discarded value from `AST_Seq` & `AST_SimpleStatement` closes mishoo#1494

kzc mentioned this pull request Feb 18, 2017

2.8.0 staging #1485

Merged

fix pure_funcs & improve side_effects

6cd98c7

- only drops side-effect-free arguments - drop side-effect-free parts with discarded value from `AST_Seq` & `AST_SimpleStatement`

alexlamsl force-pushed the pure_funcs branch from 9ab10d9 to 6cd98c7 Compare February 19, 2017 17:33

alexlamsl commented Feb 19, 2017

View reviewed changes

test/compress/pure_funcs.js

}

}

conditional: {

Copy link
Copy Markdown

Collaborator Author

alexlamsl Feb 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests beefed up to cover aforementioned changes.

alexlamsl added a commit to alexlamsl/UglifyJS that referenced this pull request Feb 19, 2017

fix pure_funcs & improve side_effects

4c0abda

- only drops side-effect-free arguments - drop side-effect-free parts with discarded value from `AST_Seq` & `AST_SimpleStatement` closes mishoo#1494

alexlamsl closed this in 26fbeec Feb 23, 2017

alexlamsl deleted the pure_funcs branch February 24, 2017 00:27

alexlamsl mentioned this pull request Mar 7, 2017

Minify compressed if statement #920

Closed

Conversation

alexlamsl commented Feb 17, 2017

Uh oh!

kzc Feb 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kzc commented Feb 17, 2017

Uh oh!

alexlamsl commented Feb 17, 2017

Uh oh!

kzc commented Feb 17, 2017

Uh oh!

kzc commented Feb 17, 2017

Uh oh!

alexlamsl commented Feb 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kzc commented Feb 17, 2017

Uh oh!

alexlamsl commented Feb 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kzc commented Feb 17, 2017

Uh oh!

alexlamsl commented Feb 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexlamsl commented Feb 17, 2017

Uh oh!

alexlamsl commented Feb 17, 2017

Uh oh!

alexlamsl commented Feb 17, 2017

Uh oh!

kzc commented Feb 17, 2017

Uh oh!

kzc commented Feb 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kzc commented Feb 18, 2017

Uh oh!

kzc commented Feb 18, 2017

Uh oh!

alexlamsl commented Feb 18, 2017

Uh oh!

alexlamsl commented Feb 18, 2017

Uh oh!

alexlamsl commented Feb 18, 2017

Uh oh!

kzc Feb 17, 2017 •

edited

Loading

alexlamsl commented Feb 17, 2017 •

edited

Loading

kzc commented Feb 18, 2017 •

edited

Loading