Skip to content

improve reduce_vars and fix a bug#1473

Closed
alexlamsl wants to merge 1 commit intomishoo:masterfrom
alexlamsl:reduce_vars-iife
Closed

improve reduce_vars and fix a bug#1473
alexlamsl wants to merge 1 commit intomishoo:masterfrom
alexlamsl:reduce_vars-iife

Conversation

@alexlamsl
Copy link
Copy Markdown
Collaborator

To improve upon the existing work of reduce_vars (released in 2.7.5) - to perform a rescan of SymbolDef.modified after each optimisation pass, and to treat IIFE invocation parameters as initialised variables.

Care has been taken against performance regressions, measured via test/benchmark.js from #1460.

@alexlamsl
Copy link
Copy Markdown
Collaborator Author

(Travis CI timed out.)

@alexlamsl alexlamsl closed this Feb 8, 2017
@alexlamsl alexlamsl reopened this Feb 8, 2017
@alexlamsl alexlamsl closed this Feb 8, 2017
@alexlamsl alexlamsl reopened this Feb 8, 2017
@kzc
Copy link
Copy Markdown
Contributor

kzc commented Feb 8, 2017

Just to confirm: modified was only set on nodes in figure_out_scope previously to be used solely by the reduce_vars optimization? So removing modified from figure_out_scope won't negatively affect any code?

@alexlamsl
Copy link
Copy Markdown
Collaborator Author

@kzc yup - I introduced it back in #1301, and I double-checked and made sure nobody else has decided to utilise this yet.

@kzc
Copy link
Copy Markdown
Contributor

kzc commented Feb 8, 2017

I'm positively surprised that reduce_vars handles parameters to IIFEs. Can you confirm this parameter optimization is limited to IIFEs and not functions in general?

This was successfully reduced:

$ echo '!function(a){console.log(a-1);}(42);' | bin/uglifyjs -c reduce_vars
!function(a){console.log(41)}(42);

but oddly this was not:

$ echo '!function(a){console.log(a);}(42);' | bin/uglifyjs -c reduce_vars
!function(a){console.log(a)}(42);

Edit: was it because the result is shorter?

nope:

$ echo '!function(ARGUMENT){console.log(ARGUMENT);}(42);' | bin/uglifyjs -c reduce_vars
!function(ARGUMENT){console.log(ARGUMENT)}(42);

@alexlamsl
Copy link
Copy Markdown
Collaborator Author

alexlamsl commented Feb 8, 2017

but oddly this was not:

That is a known quirk with evaluate:

$ echo 'const a=1;console.log(a+1)' | bin/uglifyjs -c
const a=1;console.log(2);

$ echo 'const a=1;console.log(a)' | bin/uglifyjs -c
const a=1;console.log(a);

Can you confirm this parameter optimization is limited to IIFEs and not functions in general?

I borrow the logic from negate_iife, so am fairly certain this covers only IIFEs - the normal form, and one which does new function(...){...}(...)

@alexlamsl
Copy link
Copy Markdown
Collaborator Author

alexlamsl commented Feb 8, 2017

Another two quirks I've discovered is to do with keep_fnames and keep_fargs, both of which I have an idea to improve upon in an upcoming PR 😉

Edit: #1476

@alexlamsl
Copy link
Copy Markdown
Collaborator Author

Discovered by @kzc while testing with #1479, the following won't work with reduce_vars on master (& probabaly 2.7.5 as well):

if (code == 16) {
  var bitsLength = 2, bitsOffset = 3, what = len;
} else if (code == 17) {
  var bitsLength = 3, bitsOffset = 3, what = (len = 0);
} else if (code == 18) {
  var bitsLength = 7, bitsOffset = 11, what = (len = 0);
}
var repeatLength = this.getBits(bitsLength) + bitsOffset;

Note the repeated var definitions of bitsOffset. I'm thinking of including the fix in this PR to reduce potential merge conflicts.

@kzc
Copy link
Copy Markdown
Contributor

kzc commented Feb 10, 2017

uglify-js@2.7.5 produces the correct last line with -c reduce_vars:

var repeatLength = this.getBits(bitsLength) + bitsOffset;

master 7f8d72d produces:

var repeatLength = this.getBits(bitsLength) + 11;

@kzc
Copy link
Copy Markdown
Contributor

kzc commented Feb 10, 2017

reduce_vars regression introduced in 0610c02

@alexlamsl
Copy link
Copy Markdown
Collaborator Author

Specifically, these lines.

But my new test case in f42a5db will cause even 2.7.5 to fail 😜

@kzc
Copy link
Copy Markdown
Contributor

kzc commented Feb 10, 2017

On the positive side, your new test/jetstream.js script is doing what it was designed to do.

@kzc
Copy link
Copy Markdown
Contributor

kzc commented Feb 10, 2017

Can you squash + change commit description to "improve reduce_vars and fix a bug" ?

@alexlamsl alexlamsl changed the title improve reduce_vars improve reduce_vars and fix a bug Feb 10, 2017
@kzc
Copy link
Copy Markdown
Contributor

kzc commented Feb 10, 2017

@alexlamsl Would you mind adding the test case from #1473 (comment) since it apparently exercises different logic than the test case you committed?

- update modified flag between compress() passes
- support IIFE arguments
- fix corner case with multiple definitions
@alexlamsl
Copy link
Copy Markdown
Collaborator Author

@kzc test added to squashed commit

@alexlamsl
Copy link
Copy Markdown
Collaborator Author

(Travis CI, please try again? Thank you.)

@alexlamsl alexlamsl closed this Feb 10, 2017
@alexlamsl alexlamsl reopened this Feb 10, 2017
@alexlamsl alexlamsl mentioned this pull request Feb 11, 2017
alexlamsl added a commit to alexlamsl/UglifyJS that referenced this pull request Feb 18, 2017
- update modified flag between compress() passes
- support IIFE arguments
- fix corner case with multiple definitions

closes mishoo#1473
@alexlamsl alexlamsl mentioned this pull request Feb 21, 2017
@alexlamsl alexlamsl closed this in a0f4fd3 Feb 23, 2017
@alexlamsl alexlamsl deleted the reduce_vars-iife branch February 24, 2017 00:23
@alexlamsl alexlamsl mentioned this pull request Mar 1, 2017
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.

2 participants