optimise binary operands with evaluate() by alexlamsl · Pull Request #1427 · mishoo/UglifyJS

alexlamsl · 2017-01-17T11:27:38Z

This one evolves out of #1425 (comment) - adding [WIP] in title so @kzc and others can throw corner cases at this and see what sticks 😉

kzc · 2017-01-17T15:11:48Z

Regarding the changes to the functionality of AST_Node.is_constant(). Is this because the optimizations are done elsewhere? Is the compress option evaluate=false no longer respected?

The AST_Node method is_constant was renamed to is_node_constant but were all occurrences of its use replaced? I still see is_constant being called elsewhere. Not sure how the tests passed - were there two methods of the same name?

If you're changing AST_Node.is_constant() could you please add if (this instanceof AST_RegExp) return false; as the first line of the method? It's not really a constant and can be problematic in corner cases. If this change is made then instanceof AST_RegExp can be removed from collapse_single_use_vars().

Not sure that the hoisting of evaluate for ll and rr to the top of OPT(AST_Binary) is correct. It does not appear to take into account the side effects of the reverse() function which conditionally swaps left and right.

Be aware that the tests do not cover all the code paths, so any change to these optimizations could result in potential failures if not careful. You'll be the new authority on compress and will likely have to support it after release.

alexlamsl · 2017-01-17T16:04:51Z

@kzc there should be no logic changes to AST_Node.is_constant(). The change is to introduce a new AST_Node.is_node_constant() which checks if a given node is "constant" without invoking any optimisations, i.e. evaluate(). So all existing usages of is_constant() should be left untouched.

(I'll think about the other points in your comment and get back to you in a moment.)

alexlamsl · 2017-01-17T16:45:18Z

@kzc actually, would you mind sharing the cases where RegExp and is_constant() is in conflict?

The two places is_constant() is being used are collapse_vars and y ? x : x => x

kzc · 2017-01-17T17:02:20Z

It's just code cleanup regarding RegExp. Moving the regexp check into is_constant is the correct thing to do as it is mutable and there's also instance identity (===) implications with its substitution:

        (function(){
            var result, rx = /ab*/g;
            while (result = rx.exec('acdabcdeabbb'))
                console.log(result[0]);
        })();

kzc · 2017-01-17T17:05:38Z

there should be no logic changes to AST_Node.is_constant().

It appears to be calling evaluate() indirectly in this PR which has implications for respecting the compress option evaluate=false.

+        AST_Node.DEFMETHOD("is_constant", function(compressor){
+            return this.is_node_constant() || this.evaluate(compressor).length > 1;
          });

alexlamsl · 2017-01-17T17:08:59Z

It appears to be calling evaluate() indirectly in this PR which has implications for respecting the compress option evaluate=false .

Isn't it already calling evaluate() in master?

kzc · 2017-01-17T17:19:41Z

Isn't it already calling evaluate() in master?

My bad. You are correct.

alexlamsl · 2017-01-17T17:19:41Z

@kzc started afresh with a commit that should address the concerns mentioned in your first comment.

is_constant() now rejects AST_RegExp and no longer calls evaluate() internally, which in the worst case would prohibit some cases from optimisation.

I realise that making extensive changes to compress.js is going to flag me up for the next bunch of bug reports for upcoming releases. Meanwhile, I hope this new version is less intrusive and easier to review.

kzc · 2017-01-17T17:24:52Z

I think your latest changes are safer. Sorry for the misinterpretation of past compress behavior.

alexlamsl · 2017-01-17T17:24:59Z

Apologies in advance for being redundant - calling evaluate() when evaluate=false is essentially a noop

kzc · 2017-01-17T17:25:33Z

calling evaluate() when evaluate=false is essentially a noop

Good to know.

alexlamsl · 2017-01-17T17:25:55Z

@kzc thanks for the review as always 👍

kzc · 2017-01-17T17:26:46Z

No problem. You have as good or better understanding of the code base than I do.

kzc · 2017-01-17T17:59:56Z

I realise that making extensive changes to compress.js is going to flag me up for the next bunch of bug reports for upcoming releases.

@alexlamsl The code base could indeed use a clean up which is what you were doing - and that's a good thing from both a long term maintenance and code efficiency point of view. Because Uglify is used by a lot of projects I personally try to err on the side of caution and make fairly conservative changes. But if you're willing to support the code base after making sweeping improvements, please go ahead. I just don't have as much time to commit to this project going forward.

alexlamsl · 2017-01-17T18:18:29Z

@kzc thanks for the kind words. I also think we should err on the conservative side and make incremental changes. So if/when regressions pop up they would be easier to track down.

Please don't hesitate to point out potential areas of improvement as I'm not familiar with the development history of this code base.

kzc · 2017-01-17T19:26:28Z

lib/compress.js

-                        compressor.warn("Condition left of && always false [{file}:{line},{col}]", self.start);
-                        return maintain_this_binding(compressor.parent(), self, ll[0]);
-                    }
+        if (compressor.option("conditionals")) switch (self.operator) {


I originally wrote that condition if (compressor.option("conditionals")) and it's incorrect. I later learned that the term "conditional" in a javascript context means "ternary operator" which has nothing to do with short-circuiting conditions. The "booleans" option is not appropriate either because it deals specifically with the true/false -> !0/!1 transform. I suggest to simply remove if (compressor.option("conditionals")) altogether and unconditionally run the body. Might impact some tests though.

@kzc will investigate 👌

If too many test lines are changed, don't worry about it. "conditionals" is enabled by default and nobody noticed this issue.

@kzc looks pretty straightforward - I've separated the tests out to a new file (and removed conditionals flag in those tests) for clarity.

That works. Thanks.

It being governed solely by the compress option "evaluate" makes more sense.

@kzc sure, though my "no_evaluate" tests need revision then

@alexlamsl It's your call - leave as you now have it or put it under "evaluate".

@kzc evaluate as requested - I wouldn't let a few tests stop us from doing The Right Thing 😎

That's the spirit! Thanks again.

- remove call to evaluate() in is_constant() and let nested optimize() does its job instead - reject RegExp in is_constant() and remove special case logic under collapse_vars - operands to conditionals optimisation are now always evaluate()-ed - throw error in constant_value() instead of returning undefined to catch possible bugs, similar to make_node_from_constant() - optimise binary boolean operators under `evaluate` instead of `conditionals`

rvanvelzen · 2017-01-26T11:16:45Z

Looking good once again.

alexlamsl force-pushed the binary branch from 5d0d437 to d884e4e Compare January 17, 2017 16:21

alexlamsl force-pushed the binary branch from d884e4e to 339df4b Compare January 17, 2017 17:12

alexlamsl force-pushed the binary branch from 0665240 to ded26a4 Compare January 17, 2017 19:15

kzc reviewed Jan 17, 2017

View reviewed changes

alexlamsl force-pushed the binary branch from bf23c67 to 677fec8 Compare January 17, 2017 20:02

alexlamsl mentioned this pull request Jan 19, 2017

make sure expression is AST_Binary #1430

Closed

alexlamsl force-pushed the binary branch from 4d70c2e to 05a9c02 Compare January 19, 2017 16:13

alexlamsl changed the title ~~[WIP] optimise binary operands for possible constants under evaluate()~~ optimise binary operands with evaluate() Jan 19, 2017

alexlamsl mentioned this pull request Jan 20, 2017

Next release after v2.7.5 #1383

Closed

rvanvelzen merged commit 0610c02 into mishoo:master Jan 26, 2017

alexlamsl deleted the binary branch January 26, 2017 11:35

alexlamsl mentioned this pull request Feb 1, 2017

[WIP] minor tweak to sequence manipulations #1458

Closed

alexlamsl mentioned this pull request Mar 14, 2017

minor clean-ups #1600

Merged

Conversation

alexlamsl commented Jan 17, 2017

Uh oh!

kzc commented Jan 17, 2017

Uh oh!

alexlamsl commented Jan 17, 2017

Uh oh!

alexlamsl commented Jan 17, 2017

Uh oh!

kzc commented Jan 17, 2017

Uh oh!

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

Uh oh!

Uh oh!

alexlamsl commented Jan 17, 2017

Uh oh!

kzc commented Jan 17, 2017

Uh oh!

alexlamsl commented Jan 17, 2017

Uh oh!

kzc commented Jan 17, 2017

Uh oh!

alexlamsl commented Jan 17, 2017

Uh oh!

kzc commented Jan 17, 2017

Uh oh!

alexlamsl commented Jan 17, 2017

Uh oh!

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

Uh oh!

Uh oh!

kzc commented Jan 17, 2017

Uh oh!

alexlamsl commented Jan 17, 2017

Uh oh!

kzc Jan 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!

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!

rvanvelzen commented Jan 26, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kzc commented Jan 17, 2017 •

edited

Loading

kzc commented Jan 17, 2017 •

edited

Loading

kzc Jan 17, 2017 •

edited

Loading