fix mangling collision with keep_fnames by alexlamsl · Pull Request #1431 · mishoo/UglifyJS

alexlamsl · 2017-01-20T09:30:47Z

Historically, AST_Scope.enclosed is used by AST_Scope.references() and AST_Scope.next_mangled(), with the former (now) unused within the code base.

Removing this line introduced years ago would ensure all child symbol references to be scanned by next_mangled() for possible name collision. Before this change, the scan would only work if all the children would rename itself after their parents settle down onto a mangled value, which cannot happen if the child is unmangleable, e.g. keep_fnames = true.

Fixes #1423

alexlamsl · 2017-01-20T09:40:50Z

One of the CI tests timed out - probably a fluke, re-opening to re-test.

alexlamsl · 2017-01-20T12:08:32Z

Just to leave a quick note to say that this may be sub-optimal as the parent scope's mangling will need to avoid all sub-scopes' variables, regardless of whether a sub-scope uses the parent scope's variable or not.

I have an idea of coming up with a proper solution, but need to pop out now and will work on this later on tonight.

kzc · 2017-01-20T16:26:14Z

@alexlamsl Thanks for creating this PR. A fix by removing code - impressive.

Your comments above make sense but scope handling is probably the thing I understand least about Uglify. I think we need to get the opinion of @mishoo and @rvanvelzen on this issue.

this may be sub-optimal as the parent scope's mangling will need to avoid all sub-scopes' variables, regardless of whether a sub-scope uses the parent scope's variable or not.

I'm not worried about mangle.keep_fnames=true being suboptimal, as the current uglify release does not generate correct code for this presently.

My main concern is whether regular default mangling (mangle.keep_fnames=false) will be negatively affected by this PR in some way - either the output or speed. As far as we know default mangling is correct in uglify-js releases up to this point due to the absence of bug reports. Will this PR change the default mangle behavior in any way?

(Edit: fixed mangle.keep_fnames=true in suboptimal comment)

kzc · 2017-01-20T16:53:53Z

Unfortunately this PR makes uglify significantly slower in default mangle mode:

Without fix:

$ time uglifyjs ember.prod.js -m -c warnings=0 | shasum
2418a432bf12e71650a33fe571af1a780d095e36  -

real	0m3.579s
user	0m4.011s
sys	0m0.105s

With fix:

$ time bin/uglifyjs ember.prod.js -m -c warnings=0 | shasum
2418a432bf12e71650a33fe571af1a780d095e36  -

real	0m6.478s
user	0m6.902s
sys	0m0.108s

The input file ember.prod.js is 1,695,992 bytes.

It's slower as the input file size increases. app.js below is 5,209,857 bytes:

Without fix:

$ time uglifyjs app.js -m -c warnings=0 | shasum
08690ccc3b1f3a90ecf20221ae0f8f9adfa2b786  -

real	0m10.577s
user	0m11.168s
sys	0m0.218s

With fix:

$ time bin/uglifyjs app.js -m -c warnings=0 | shasum
08690ccc3b1f3a90ecf20221ae0f8f9adfa2b786  -

real	0m31.206s
user	0m31.805s
sys	0m0.213s

Three times slower for a 5.2MB input file.

We can't use this fix in its present form without some adjustments.

On the plus side, all input files I've tried still produce the same shasum with this PR indicating the default mangling algorithm remains the same.

(Edit: I had the with/without timings reversed - fixed)

kzc · 2017-01-20T16:59:59Z

In light of these timings, I think it would be preferable to reintroduce the code removed by this PR and put them within if (!options.keep_fnames) blocks.

kzc · 2017-01-20T17:04:14Z

Unrelated side note: this project should put together a test suite of a large number of JS input files (of commonly used frameworks and applications) and their corresponding shasums to test uglifyjs -c -m timings before each release to spot speed regressions.

alexlamsl · 2017-01-20T17:08:12Z

@kzc the identical results with keep_fnames = false is what I expected. The performance impact is a surprise, however.

Luckily reference() is called only by AST_Scope.figure_out_scope(), which with your recent PR is now guaranteed to be supplied with mangle options 😉

I'll put up this minimal fix with sub-optimal impact to keep_fnames = true here, and open a separate PR for the more complete fix and we can test and pick the one we want.

kzc · 2017-01-20T17:15:22Z

The performance impact is a surprise, however.

It may be due to uglify's use of arrays and linear lookup scans. I made another fix a while back to replace an array with a map and it made it considerably faster: #1024

I'm not suggesting this PR alter a data structure, just something to be aware of.

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

lib/scope.js

    while (s) {
        push_uniq(s.enclosed, def);
-        if (s === def.scope) break;
+        if (s === def.scope && !options.keep_fnames) break;


Can you please add a comment above this line why the && !options.keep_fnames clause is important. Otherwise it may be inadvertently removed in the future.

kzc · 2017-01-20T17:31:32Z

lib/scope.js

    while (s) {
        push_uniq(s.enclosed, def);
-        if (s === def.scope) break;
+        // propagate unmangleable function names to top level


Could you please use this comment? (assuming it is accurate)

// Propagate unmangleable function names to top level. // Although `&& !options.keep_fnames` is not strictly needed, // it improves the speed of default mangling. // See: https://github.com/mishoo/UglifyJS2/pull/1431

Thanks for typing it out - included in code 😉

kzc · 2017-01-20T17:38:04Z

@alexlamsl With your latest revision the default mangle speed regression is now gone:

With latest fix:

$ time bin/uglifyjs ember.prod.js -m -c warnings=0 | shasum
2418a432bf12e71650a33fe571af1a780d095e36  -

real	0m3.652s
user	0m4.094s
sys	0m0.108s

$ time bin/uglifyjs app.js -m -c warnings=0 | shasum
08690ccc3b1f3a90ecf20221ae0f8f9adfa2b786  -

real	0m10.451s
user	0m11.066s
sys	0m0.212s

Great stuff!

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

LGTM.

Impressed with the elegance of this fix. Is the alternate "optimal" fix #1432 for keep_fnames=true needed?

kzc · 2017-01-20T18:11:39Z

I evaluated #1432 and it's good.

Since you say #1432 handles more corner cases than this PR with only minimal additional changes, could you please update this PR to match the fix in #1432 and then close #1432. The comments in this PR are useful to future maintainers.

fixes mishoo#1423

alexlamsl · 2017-01-20T18:18:48Z

@kzc thanks for testing and review as always 👍

So the reason why this new fix works on more cases is due to AST_SymbolRef.reference() would account for unused variables, which when combined with keep_fnames = true would cause name collisions.

Fun fact: if compress.unused = true is on, then those unused functions would get removed so even with keep_fnames = false the previous fix would have worked.

(just read your comments about array vs. object (id's) - will keep that in mind going forward)

kzc · 2017-01-20T18:25:41Z

LGTM

@alexlamsl Confirmed that latest PR produces identical default mangle results on the two large javascript input files above with no speed regressions.

Nice work!

kzc · 2017-01-20T18:30:17Z

Also worth noting that using -m keep_fnames=true -c keep_fnames=true does not slow down uglify significantly on same inputs tested above.

bring command-line in line with minify()

kzc · 2017-01-24T04:06:51Z

Thanks @alexlamsl. The CLI fix and new tests look good.

alexlamsl force-pushed the keep_fnames branch from 6758402 to 1d215e5 Compare January 20, 2017 09:33

alexlamsl closed this Jan 20, 2017

alexlamsl reopened this Jan 20, 2017

alexlamsl mentioned this pull request Jan 20, 2017

keep_fnames: true with morris.js #1423

Closed

alexlamsl force-pushed the keep_fnames branch from 1d215e5 to ccfab65 Compare January 20, 2017 17:13

kzc reviewed Jan 20, 2017

View reviewed changes

alexlamsl force-pushed the keep_fnames branch from ccfab65 to f328438 Compare January 20, 2017 17:25

kzc reviewed Jan 20, 2017

View reviewed changes

alexlamsl force-pushed the keep_fnames branch from f328438 to 61e9ff2 Compare January 20, 2017 17:43

alexlamsl mentioned this pull request Jan 20, 2017

fix mangling collision with keep_fnames (take 2) #1432

Closed

fix mangling collision with keep_fnames

4b4c7db

fixes mishoo#1423

alexlamsl force-pushed the keep_fnames branch from 61e9ff2 to 4b4c7db Compare January 20, 2017 18:14

alexlamsl mentioned this pull request Jan 20, 2017

Next release after v2.7.5 #1383

Closed

kzc mentioned this pull request Jan 23, 2017

pass mangle options to figure_out_scope() #1436

Closed

pass mangle options to figure_out_scope()

35da3fe

bring command-line in line with minify()

rvanvelzen merged commit 1eaa211 into mishoo:master Jan 26, 2017

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

alexlamsl mentioned this pull request Jan 27, 2017

fix crash in unsafe replacement of undefined #1443

Closed

Conversation

alexlamsl commented Jan 20, 2017

Uh oh!

alexlamsl commented Jan 20, 2017

Uh oh!

alexlamsl commented Jan 20, 2017

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

kzc commented Jan 20, 2017

Uh oh!

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

Uh oh!

Uh oh!

alexlamsl commented Jan 20, 2017

Uh oh!

kzc commented Jan 20, 2017

Uh oh!

kzc Jan 20, 2017

Choose a reason for hiding this comment

Uh oh!

alexlamsl Jan 20, 2017

Choose a reason for hiding this comment

Uh oh!

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

alexlamsl Jan 20, 2017

Choose a reason for hiding this comment

Uh oh!

kzc commented Jan 20, 2017

Uh oh!

kzc commented Jan 20, 2017

Uh oh!

kzc commented Jan 20, 2017

Uh oh!

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

Uh oh!

Uh oh!

kzc commented Jan 20, 2017

Uh oh!

kzc commented Jan 20, 2017

Uh oh!

kzc commented Jan 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kzc commented Jan 20, 2017 •

edited

Loading

kzc commented Jan 20, 2017 •

edited

Loading

kzc commented Jan 20, 2017 •

edited

Loading

kzc Jan 20, 2017 •

edited

Loading

alexlamsl commented Jan 20, 2017 •

edited

Loading