Skip to content
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.

Sign up for GitHub

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

[WIP] improve mangle #2219

Closed
wants to merge 1 commit into from
Closed

[WIP] improve mangle #2219

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 17 additions & 15 deletions lib/scope.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,14 +415,10 @@ AST_Toplevel.DEFMETHOD("mangle_names", function(options){
var to_mangle = [];

if (options.cache) {
this.globals.each(function(symbol){
if (options.reserved.indexOf(symbol.name) < 0) {
to_mangle.push(symbol);
}
});
push_symbols(this.globals);
}

var tw = new TreeWalker(function(node, descend){
this.walk(new TreeWalker(function(node, descend) {
if (node instanceof AST_LabeledStatement) {
// lname is incremented when we get to the AST_Label
var save_nesting = lname;
Expand All @@ -431,13 +427,7 @@ AST_Toplevel.DEFMETHOD("mangle_names", function(options){
return true; // don't descend again in TreeWalker
}
if (node instanceof AST_Scope) {
var p = tw.parent(), a = [];
node.variables.each(function(symbol){
if (options.reserved.indexOf(symbol.name) < 0) {
a.push(symbol);
}
});
to_mangle.push.apply(to_mangle, a);
push_symbols(node.variables);
return;
}
if (node instanceof AST_Label) {
Expand All @@ -450,13 +440,25 @@ AST_Toplevel.DEFMETHOD("mangle_names", function(options){
to_mangle.push(node.definition());
return;
}
});
this.walk(tw);
}));
to_mangle.forEach(function(def){ def.mangle(options) });

if (options.cache) {
options.cache.cname = this.cname;
}

function push_symbols(dict) {
var a = [];
dict.each(function(symbol) {
if (options.reserved.indexOf(symbol.name) < 0) {
a.push(symbol);
}
});
a.sort(function(m, n) {
return n.references.length - m.references.length;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think symbol references is updated in compress and would present the pre-compress value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

figure_out_scope() is called again after compress() and right before mangle_names(), as I mentioned in the code references in #2219 (comment)

... or have I missed something? 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I missed mentioning that within figure_out_scope(), init_scope_vars() is called on each AST_Scope, which resets all references to previous instances of SymbolDef:
https://github.com/mishoo/UglifyJS2/blob/94e5e00c0321b92aca1abf170c12a02d6c3275b5/lib/scope.js#L117
https://github.com/mishoo/UglifyJS2/blob/94e5e00c0321b92aca1abf170c12a02d6c3275b5/lib/scope.js#L262-L263

Copy link
Contributor

Choose a reason for hiding this comment

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

...wait - my mistake - it's figure_out_scope() that determines all the references in the first place is it not?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it's figure_out_scope() that determines all the references in the first place is it not?

Yup - reduce_vars does rebuild some of that within reset_opt_flags(), but whenever figure_out_scope() is called everything is reset to a clean slate.

Copy link
Contributor

Choose a reason for hiding this comment

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

+        a.sort(function(m, n) {
+            return n.references.length - m.references.length;

I vaguely recall seeing that mangle approach attempted a couple years back and abandoned due to gzip results. I could be mistaken.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In that case, more the reason to try and get to the bottom of this - mysteries are there to be solved 😜

});
to_mangle.push.apply(to_mangle, a);
}
});

AST_Toplevel.DEFMETHOD("compute_char_frequency", function(options){
Expand Down
2 changes: 1 addition & 1 deletion test/compress/functions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ issue_203: {
}
expect: {
var m = {};
var fn = Function("n,o", "o.exports=42");
var fn = Function("o,n", "n.exports=42");
fn(null, m, m.exports);
console.log(m.exports);
}
Expand Down
4 changes: 2 additions & 2 deletions test/compress/issue-1202.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ mangle_keep_fnames_false: {
expect: {
"use strict";
function total() {
return function t(n, r, u) {
return n + r + u;
return function c(t, r, u) {
return t + r + u;
};
}
}
Expand Down
2 changes: 1 addition & 1 deletion test/compress/issue-1446.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,5 +71,5 @@ undefined_redefined_mangle: {
return typeof n == "undefined";
}
}
expect_exact: "function f(n){var r=1;return void 0===r}"
expect_exact: "function f(r){var n=1;return void 0===n}"
}
4 changes: 2 additions & 2 deletions test/mocha/minify.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ describe("minify", function() {
compressed += result.code;
});
assert.strictEqual(JSON.stringify(cache).slice(0, 20), '{"cname":5,"props":{');
assert.strictEqual(compressed, 'function n(n){return 3*n}function r(n){return n/2}function c(o){l("Foo:",2*o)}var l=console.log.bind(console);var f=n(3),i=r(12);l("qux",f,i),c(11);');
assert.strictEqual(compressed, 'function n(n){return 3*n}function r(n){return n/2}function l(o){c("Foo:",2*o)}var c=console.log.bind(console);var f=n(3),i=r(12);c("qux",f,i),l(11);');
assert.strictEqual(run_code(compressed), run_code(original));
});

Expand All @@ -69,7 +69,7 @@ describe("minify", function() {
compressed += result.code;
});
assert.strictEqual(JSON.stringify(cache).slice(0, 28), '{"vars":{"cname":5,"props":{');
assert.strictEqual(compressed, 'function n(n){return 3*n}function r(n){return n/2}function c(o){l("Foo:",2*o)}var l=console.log.bind(console);var f=n(3),i=r(12);l("qux",f,i),c(11);');
assert.strictEqual(compressed, 'function n(n){return 3*n}function r(n){return n/2}function l(o){c("Foo:",2*o)}var c=console.log.bind(console);var f=n(3),i=r(12);c("qux",f,i),l(11);');
assert.strictEqual(run_code(compressed), run_code(original));
});

Expand Down