drop unused: toplevel, assign-only by alexlamsl · Pull Request #1450 · mishoo/UglifyJS

alexlamsl · 2017-01-29T16:07:33Z

Only works when assignment is on the same scope level as declaration for now (see new tests in drop-unused.js).

Run against angular.js and managed to shave one extra variable off without any measureable change in performance.

This can obviously be extended in the future, but given how long I spent reading up drop_unused() let's take one step at a time. Putting it here as I run out of tests to throw at it (for now).

kzc · 2017-01-29T16:26:00Z

How can your optimization be disabled for the collapse_vars tests because I don't want these variables to be dropped. This tests the flow analysis of collapse_vars.

Can your optimization be put behind a new option?

kzc · 2017-01-29T16:29:49Z

test/compress/drop-unused.js

+            var a;
+            a = 1;
+        }
+


Please remove the blank lines between the functions in this test so it's easier to compare the input to the output. Thanks.

kzc · 2017-01-29T16:32:00Z

Perhaps a new option drop_assign_only defaulting to true? That way it can be selectively disabled for tests.

alexlamsl · 2017-01-29T16:36:35Z

@kzc any suggestions with a shorter option name? It's a chore adding white spaces to all the other options... 😝

kzc · 2017-01-29T16:39:02Z

Feel free to come up with a shorter name. drop_unused or assign_only maybe?

kzc · 2017-01-29T16:40:05Z

Those suggested names are not the greatest. What's the longest option name currently?

alexlamsl · 2017-01-29T16:40:54Z

drop_debugger and collapse_vars are tied.

kzc · 2017-01-29T16:43:49Z

I'm stumped to come up with a better, shorter name.

alexlamsl · 2017-01-29T16:46:01Z

@kzc no worries - I'll think of something after/while updating the code and tests 😉

kzc · 2017-01-29T16:48:05Z

What if unused can be overloaded with an integer value 2 indicating to not use this optimization?

~~If it's any other true-ish value then it was as it was before.~~
If it's any other true-ish value then it will drop the unused var.

alexlamsl · 2017-01-29T16:50:30Z

@kzc sounds pretty cool actually, which makes this new feature off by default as well.

kzc · 2017-01-29T16:51:35Z

We can have the new feature enabled by default with true.
Setting unused=2 can disable the new feature.

alexlamsl · 2017-01-29T16:53:18Z

@kzc yes, I misread your previous comment (again)

kzc · 2017-01-29T16:53:58Z

People tend use default options. Not many people set collapse_vars=true, for example.

@kzc yes, I misread your previous comment (again)

You didn't misread it - I rewrote it. Sorry about that.

kzc · 2017-01-29T17:05:24Z

What if there was yet another option setting that can remove unused top level vars?

$ echo 'var a = foo();' | bin/uglifyjs -c
var a=foo();

var a was not dropped because it was top-level.

This should be off by default as some people expect this grandfathered behavior in uglified scripts in the wild.

kzc · 2017-01-29T17:07:47Z

test/compress/collapse_vars.js

        function f2(x) { var z = x, a = ++z; return z += a; }
        function f3(x) { var a = (x -= 3); return x + a; }
        function f4(x) { var a = (x -= 3); return x + a; }
-        function f5(x) { var w = e1(), v = e2(), c = v = --x; return (w = x) - c; }


Can you please set unused: 2 in the collapse_vars tests with different results?

I copied these original tests to the bottom with unused=2 - guess I should do it the other way round 😅

kzc · 2017-01-29T17:08:16Z

test/compress/collapse_vars.js

-            var a = 2;
            do {
-                fn(a = 7);
+                fn(7);


Can you please set unused: 2 in the collapse_vars tests with different results?

kzc · 2017-01-29T17:10:30Z

test/compress/collapse_vars.js

    }
 }
+
+keep_assign: {


I see what you've done. Please keep the original test names in the original place. Just change unused to 2 in the original tests that have different results.

You may add a new test below the original.

Would you like me to copy the whole section below each of the two original blocks of tests, or just the affected case?

The latest collapse_vars test changes look good - thanks!

kzc · 2017-01-29T17:15:13Z

lib/compress.js

+                    if (node instanceof AST_Assign && node.operator == "="
+                        && node.left instanceof AST_SymbolRef
+                        && scope === self
+                        && assign_as_unused) {


Would you mind making assign_as_unused the first condition in the if? It's faster if assign_as_unused is not in effect, and the same speed otherwise.

kzc · 2017-01-29T17:15:30Z

lib/compress.js

                    }
+                    if (node instanceof AST_Assign && node.operator == "="
+                        && node.left instanceof AST_SymbolRef
+                        && assign_as_unused) {


Would you mind making assign_as_unused the first condition in the if? It's faster if assign_as_unused is not in effect, and the same speed otherwise.

alexlamsl · 2017-01-29T17:20:52Z

test/compress/collapse_vars.js

 }
-
-collapse_vars_regexp: {
-    options = {


kzc · 2017-01-29T17:21:36Z

test/compress/collapse_vars.js

    }
 }
-
-collapse_vars_regexp: {


Why was this regex test dropped?

This test was added originally as result of a bug fix. Just because the implementation with respect to is_const changed does not mean the test should be dropped.

it's cool. i was just confused.

I was trying to remove my own tests at the end of the file, and forgot I've done so already 🙄

Tests restored.

kzc · 2017-01-29T17:27:35Z

There's a comment above that might have been lost in the shuffle: #1450 (comment)

Would welcome your thoughts on the best option to remove unused top level vars.

alexlamsl · 2017-01-29T17:32:49Z

@kzc yeah I am still chewing on that thought - the code change seems trivial _{(famous last words...)}

alexlamsl · 2017-01-29T17:33:17Z

Recycling PR state to re-run timed-out CI tests.

kzc · 2017-01-29T17:37:44Z

Dropping unused top-level vars may warrant its own option.

alexlamsl · 2017-01-29T17:51:30Z

@kzc I've given it a go with string-based option value. Worse case, we can just break both of these features back out as separate flags.

I was hoping the code is the more challenging part of this exercise...

kzc · 2017-01-29T23:15:21Z

So I guess the only outstanding things are top_retain RegExp support and implicitly setting -c toplevel=true when top_retain is used and toplevel is undefined.

alexlamsl · 2017-01-29T23:19:45Z

@kzc ah so it's just a CLI limitation then. On that note I did have an urge to rewrite that in commander like I did for html-minifier 😛

I've just finished updating the docs - will update code now and push in a moment.

kzc · 2017-01-30T00:06:54Z

Ah, I see now - this is how one enters a string argument into yargs from the CLI:

$ echo 'function y(){}' | bin/uglifyjs -c top_retain='"x,y,z"',toplevel
function y(){}

$ echo 'function y(){}' | bin/uglifyjs -c top_retain='"x,z"',toplevel
WARN: Dropping unused function y [-:1,9]

alexlamsl · 2017-01-30T00:09:46Z

So it's all a big eval()... ❗️

PTAL at the latest changes.

kzc · 2017-01-30T01:17:35Z

The CLI and the various options work perfectly. Uglify users will definitely benefit from this new functionality.

Great work!

- assign statement does not count towards variable usage by default - only works with assignments on the same scope level as declaration - can be disabled with `unused` set to "keep_assign" - `toplevel` to drop unused top-level variables and/or functions - `top_retain` to whitelist top-level exceptions

alexlamsl · 2017-01-30T05:21:03Z

@kzc thanks for all the help, as always 👍

- assign statement does not count towards variable usage by default - only works with assignments on the same scope level as declaration - can be disabled with `unused` set to "keep_assign" - `toplevel` to drop unused top-level variables and/or functions - `top_retain` to whitelist top-level exceptions closes mishoo#1450

When mishoo#1450 optimises `a=b=42`, it stops after the first variable even if both are unused. fixes mishoo#1539

When #1450 optimises `a=b=42`, it stops after the first variable even if both are unused. fixes #1539

similar case as mishoo#1578 but against mishoo#1450 instead closes mishoo#1580

- similar case as mishoo#1578 but against mishoo#1450 instead - fix `this` binding in mishoo#1450 as well closes mishoo#1580

- similar case as #1578 but against #1450 instead - fix `this` binding in #1450 as well closes #1580

kzc reviewed Jan 29, 2017

View reviewed changes

alexlamsl commented Jan 29, 2017

View reviewed changes

test/compress/collapse_vars.js

}

collapse_vars_regexp: {

options = {

Copy link
Copy Markdown

Collaborator Author

alexlamsl Jan 29, 2017

Choose a reason for hiding this comment

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

Ooops!

kzc reviewed Jan 29, 2017

View reviewed changes

alexlamsl force-pushed the unused-assign branch from 6cc2615 to 95ccf22 Compare January 29, 2017 17:28

alexlamsl closed this Jan 29, 2017

alexlamsl reopened this Jan 29, 2017

alexlamsl changed the title ~~[WIP] drop unused: toplevel, assign-only~~ drop unused: toplevel, assign-only Jan 30, 2017

alexlamsl force-pushed the unused-assign branch from d08a00d to cebff96 Compare January 30, 2017 05:21

This was referenced Feb 9, 2017

add benchmark & JetStream tests #1479

Closed

2.8.0 staging #1485

Merged

alexlamsl mentioned this pull request Feb 17, 2017

fix pure_funcs & improve side_effects #1494

Closed

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

add test for mishoo#1448 & mishoo#1450

391a25a

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

add test for mishoo#1448 & mishoo#1450

01b8313

alexlamsl mentioned this pull request Feb 21, 2017

@const creeping #1497

Closed

alexlamsl closed this in 148047f Feb 23, 2017

alexlamsl deleted the unused-assign branch February 24, 2017 00:18

This was referenced Feb 24, 2017

Improve `unused': Variable is not dropped, when it is hoisted. #426

Closed

multiple var assignment to same value with unused vars is compressed incorrectly #1539

Closed

fix chained assignment with unused #1540

Merged

alexlamsl added a commit to alexlamsl/UglifyJS that referenced this pull request Mar 2, 2017

fix chained assignment with unused

d8677f7

When mishoo#1450 optimises `a=b=42`, it stops after the first variable even if both are unused. fixes mishoo#1539

alexlamsl added a commit that referenced this pull request Mar 2, 2017

fix chained assignment with unused (#1540)

17b8135

When #1450 optimises `a=b=42`, it stops after the first variable even if both are unused. fixes #1539

alexlamsl added a commit to alexlamsl/UglifyJS that referenced this pull request Mar 8, 2017

scan RHS of dropped assignments

db5a348

similar case as mishoo#1578 but against mishoo#1450 instead closes mishoo#1580

This was referenced Mar 8, 2017

_dec is not defined #1580

Closed

scan RHS of dropped assignments #1581

Merged

alexlamsl added a commit to alexlamsl/UglifyJS that referenced this pull request Mar 8, 2017

scan RHS of dropped assignments

95d1c56

- similar case as mishoo#1578 but against mishoo#1450 instead - fix `this` binding in mishoo#1450 as well closes mishoo#1580

alexlamsl added a commit that referenced this pull request Mar 8, 2017

scan RHS of dropped assignments (#1581)

7e465d4

- similar case as #1578 but against #1450 instead - fix `this` binding in #1450 as well closes #1580

etpinard mentioned this pull request Mar 13, 2017

Compress compiled block using { unsued: "keep_assign" } scijs/cwise#13

Merged

Conversation

alexlamsl commented Jan 29, 2017

Uh oh!

kzc commented Jan 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kzc commented Jan 29, 2017

Uh oh!

alexlamsl commented Jan 29, 2017

Uh oh!

kzc commented Jan 29, 2017

Uh oh!

kzc commented Jan 29, 2017

Uh oh!

alexlamsl commented Jan 29, 2017

Uh oh!

kzc commented Jan 29, 2017

Uh oh!

alexlamsl commented Jan 29, 2017

Uh oh!

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

Uh oh!

Uh oh!

alexlamsl commented Jan 29, 2017

Uh oh!

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

Uh oh!

Uh oh!

alexlamsl commented Jan 29, 2017

Uh oh!

kzc commented Jan 29, 2017

Uh oh!

kzc commented Jan 29, 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!

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!

kzc commented Jan 29, 2017

Uh oh!

alexlamsl commented Jan 29, 2017

Uh oh!

alexlamsl commented Jan 29, 2017

Uh oh!

kzc commented Jan 29, 2017

Uh oh!

alexlamsl commented Jan 29, 2017

Uh oh!

kzc commented Jan 29, 2017

Uh oh!

alexlamsl commented Jan 29, 2017

Uh oh!

kzc commented Jan 30, 2017

kzc commented Jan 29, 2017 •

edited

Loading

kzc commented Jan 29, 2017 •

edited

Loading