-
Notifications
You must be signed in to change notification settings - Fork 1.2k
improve mangle
#2948
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
improve mangle
#2948
Conversation
@kzc I think I'll do 1MFuzz on this locally, and if everything's green this is good to go. |
Seems to provide some general mangle improvement.
Edit: gzip improvement: 1288, or 0.194% If you don't mind, could you briefly describe how the algorithm was changed? |
Take the following input: var a;
function f(b) {
var c;
} Before this PR, var n1;
function n2(n3) {
var n4;
} This PR assigns them from the innermost var n3;
function n4(n1) {
var n2;
} The previous approach is easier to implement efficiently, especially when it comes to namespace utilisation, i.e. ensuring no "holes" between shortest and longest However, this would suffer from avalanche effect, where addition of a variable would often cause many unrelated scopes to lose contiguous name assignments, which in turn harms This is basically another attempt at #2219, with all the bells and whistles untangled and (seemingly) functionally correct this time round. |
That makes sense. Thanks for the explanation. By the way, I fixed a typo in my post above. This PR provides close to a 0.2% improvement. |
Good to know 👍 FWIW, I tried to improve |
Taking care of all those weird corner cases, most of them involving |
I noticed a 0.45% regression with this PR in |
Yeah - I compared the output size-by-size and all I can say is there is room for improvement for our current scheme of |
This is an impressive overall Gzip win! |
Normally any collision on these names won't be an issue but the Safari on iOS 7/8/9 will yield with a SyntaxError. The problem was discuessed and (partially) fixed in mishoo#179 but function definition mangling still breaks: ```js $ echo 'function aaa(){function foo(bar){};foo.name=2;return foo}' | ./node_modules/.bin/uglifyjs --compress --mangle function aaa(){function n(n){}return n.name=2,n} ``` Since symbol names are now [mangled from within](mishoo#2948 (comment)), the mangled argument names can now be appended to `names` cache to make sure the later mangled function name won't collide with them.
Normally any collision on these names won't be an issue but the Safari on iOS 7/8/9 will yield with a SyntaxError. The problem was discuessed and (partially) fixed in mishoo#179 but function definition mangling still breaks: ```js $ echo 'function aaa(){function foo(bar){};foo.name=2;return foo}' | ./node_modules/.bin/uglifyjs --compress --mangle function aaa(){function n(n){}return n.name=2,n} ``` Since symbol names are now [mangled from within](mishoo#2948 (comment)), the mangled argument names can now be appended to `names` cache to make sure the later mangled function name won't collide with them.
Usually the function name is dropped if the function is a function expression. But when `ie8` option is enabled, the function name is kept and mangled which may break legacy Safari for said reason. Since symbol names are now [mangled from within](mishoo#2948 (comment)), this fix adds argument names to existing names when mangling the name of a function expression.
Usually the function name is dropped if the function is a function expression. But when `ie8` option is enabled, the function name is kept and mangled which may break legacy Safari for said reason. Since symbol names are now [mangled from within](mishoo#2948 (comment)), this fix adds argument names to existing names when mangling the name of a function expression.
Usually the function name is dropped if the function is a function expression. But when `ie8` option is enabled, the function name is kept and mangled which may break legacy Safari for said reason. Since symbol names are now [mangled from within](mishoo#2948 (comment)), this fix adds argument names to existing names when mangling the name of a function expression.
Usually the function name is dropped if the function is a function expression. But when `ie8` option is enabled, the function name is kept and mangled which may break legacy Safari for said reason. Since symbol names are now [mangled from within](mishoo#2948 (comment)), this fix adds argument names to existing names when mangling the name of a function expression.
node test/benchmark.js