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

Add passes compress option. Fix duplicate compress warnings. #1040

Closed
wants to merge 1 commit into from
Closed

Add passes compress option. Fix duplicate compress warnings. #1040

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
3 changes: 3 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,9 @@ to set `true`; it's effectively a shortcut for `foo=true`).
compressor from mangling/discarding function names. Useful for code relying on
`Function.prototype.name`.

- `passes` -- default `1`. Number of times to run compress. Use an
integer argument larger than 1 to further reduce code size in some cases.
Note: raising the number of passes will increase uglify compress time.

### The `unsafe` option

Expand Down
2 changes: 1 addition & 1 deletion bin/uglifyjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,7 +423,7 @@ async.eachLimit(files, 1, function (file, cb) {

if (COMPRESS) {
time_it("squeeze", function(){
TOPLEVEL = TOPLEVEL.transform(compressor);
TOPLEVEL = compressor.compress(TOPLEVEL);
});
}

Expand Down
42 changes: 33 additions & 9 deletions lib/compress.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,18 +75,36 @@ function Compressor(options, false_by_default) {
screw_ie8 : false,
drop_console : false,
angular : false,

warnings : true,
global_defs : {}
global_defs : {},
passes : 1,
}, true);
this.warnings_produced = {};
};

Compressor.prototype = new TreeTransformer;
merge(Compressor.prototype, {
option: function(key) { return this.options[key] },
warn: function() {
if (this.options.warnings)
AST_Node.warn.apply(AST_Node, arguments);
compress: function(node) {
var passes = +this.options.passes || 1;
for (var pass = 0; pass < passes && pass < 3; ++pass) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@kzc Mind explaining the implicit maximum of 3 passes? My assumption is that it's an arbitrary "good enough" amount that prevents the user from shooting their foot. However I still feel it should be mentioned in the readme.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is arbitrary. I haven't seen any JS input that produces smaller output with more than 3 passes, and each additional pass makes minification slower. If you can demonstrate code that appreciably benefits from a higher number we will reconsider increasing the value.

If you wish to make a PR mentioning its max value of 3, that would be great.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense - thanks so much.

I'm putting together a PR tackling the side_effects option with passes - since pure annotations are removed on the first pass, thus removing the benefit of multiple passes. I'm having a hard time creating a small reproduction of code that doesn't get fully minified in one pass though - it's an interesting problem.

Copy link
Contributor Author

@kzc kzc May 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

since pure annotations are removed on the first pass, thus removing the benefit of multiple passes

That hasn't been my experience with #__PURE__ annotations. Test different passes settings in the following examples:

#1261 (comment)
#1261 (comment)
#1261 (comment)

Also, you can set an option to keep the pure annotation comments if you want uglify to emit them after minification.

Clarification: the /*#__PURE__*/ annotation is not set on a function, but a function call. I had missed that in the recent README doc fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm - thanks for the test references. Seems I need to better understand my problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the misunderstanding - you were right and my issue was not thoroughly marking the functions I needed to. I'll put together the PR for max passes now.

if (pass > 0) node.clear_opt_flags();
node = node.transform(this);
}
return node;
},
warn: function(text, props) {
if (this.options.warnings) {
// only emit unique warnings
var message = string_template(text, props);
if (!(message in this.warnings_produced)) {
this.warnings_produced[message] = true;
AST_Node.warn.apply(AST_Node, arguments);
}
}
},
clear_warnings: function() {
this.warnings_produced = {};
},
before: function(node, descend, in_list) {
if (node._squeezed) return node;
Expand Down Expand Up @@ -129,6 +147,15 @@ merge(Compressor.prototype, {
return this.print_to_string() == node.print_to_string();
});

AST_Node.DEFMETHOD("clear_opt_flags", function(){
this.walk(new TreeWalker(function(node){
if (!(node instanceof AST_Directive || node instanceof AST_Constant)) {
node._squeezed = false;
node._optimized = false;
}
}));
});

function make_node(ctor, orig, props) {
if (!props) props = {};
if (orig) {
Expand Down Expand Up @@ -392,10 +419,7 @@ merge(Compressor.prototype, {
var_defs_removed = true;
}
// Further optimize statement after substitution.
stat.walk(new TreeWalker(function(node){
delete node._squeezed;
delete node._optimized;
}));
stat.clear_opt_flags();

compressor.warn("Replacing " + (is_constant ? "constant" : "variable") +
" " + var_name + " [{file}:{line},{col}]", node.start);
Expand Down
48 changes: 16 additions & 32 deletions test/compress/issue-1034.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ non_hoisted_function_after_return_2a: {
hoist_funs: false, dead_code: true, conditionals: true, comparisons: true,
evaluate: true, booleans: true, loops: true, unused: true, keep_fargs: true,
if_return: true, join_vars: true, cascade: true, side_effects: true,
collapse_vars: false
collapse_vars: false, passes: 2
}
input: {
function foo(x) {
Expand All @@ -57,20 +57,11 @@ non_hoisted_function_after_return_2a: {
}
}
expect: {
// NOTE: Output is correct, but suboptimal. Not a regression. Can be improved in future.
// This output is run through non_hoisted_function_after_return_2b with same flags.
function foo(x) {
if (x) {
return bar(1);
} else {
return bar(2);
var b;
}
var c = bar(3);
return bar(x ? 1 : 2);
function bar(x) {
return 7 - x;
}
return b || c;
}
}
expect_warnings: [
Expand All @@ -79,7 +70,12 @@ non_hoisted_function_after_return_2a: {
"WARN: Dropping unreachable code [test/compress/issue-1034.js:51,16]",
"WARN: Declarations in unreachable code! [test/compress/issue-1034.js:51,16]",
"WARN: Dropping unused variable a [test/compress/issue-1034.js:48,20]",
"WARN: Dropping unused function nope [test/compress/issue-1034.js:55,21]"
"WARN: Dropping unused function nope [test/compress/issue-1034.js:55,21]",
"WARN: Dropping unreachable code [test/compress/issue-1034.js:53,12]",
"WARN: Declarations in unreachable code! [test/compress/issue-1034.js:53,12]",
"WARN: Dropping unreachable code [test/compress/issue-1034.js:56,12]",
"WARN: Dropping unused variable b [test/compress/issue-1034.js:51,20]",
"WARN: Dropping unused variable c [test/compress/issue-1034.js:53,16]"
]
}

Expand All @@ -91,7 +87,6 @@ non_hoisted_function_after_return_2b: {
collapse_vars: false
}
input: {
// Note: output of test non_hoisted_function_after_return_2a run through compress again
function foo(x) {
if (x) {
return bar(1);
Expand All @@ -107,31 +102,20 @@ non_hoisted_function_after_return_2b: {
}
}
expect: {
// the output we would have liked to see from non_hoisted_function_after_return_2a
function foo(x) {
return bar(x ? 1 : 2);
function bar(x) { return 7 - x; }
}
}
expect_warnings: [
// Notice that some warnings are repeated by multiple compress passes.
// Not a regression. There is room for improvement here.
// Warnings should be cached and only output if unique.
"WARN: Dropping unreachable code [test/compress/issue-1034.js:100,16]",
"WARN: Declarations in unreachable code! [test/compress/issue-1034.js:100,16]",
"WARN: Dropping unreachable code [test/compress/issue-1034.js:100,16]",
"WARN: Declarations in unreachable code! [test/compress/issue-1034.js:100,16]",
"WARN: Dropping unreachable code [test/compress/issue-1034.js:100,16]",
"WARN: Declarations in unreachable code! [test/compress/issue-1034.js:100,16]",
"WARN: Dropping unreachable code [test/compress/issue-1034.js:100,16]",
"WARN: Declarations in unreachable code! [test/compress/issue-1034.js:100,16]",
"WARN: Dropping unreachable code [test/compress/issue-1034.js:102,12]",
"WARN: Declarations in unreachable code! [test/compress/issue-1034.js:102,12]",
"WARN: Dropping unreachable code [test/compress/issue-1034.js:106,12]",
"WARN: Dropping unreachable code [test/compress/issue-1034.js:100,16]",
"WARN: Declarations in unreachable code! [test/compress/issue-1034.js:100,16]",
"WARN: Dropping unused variable b [test/compress/issue-1034.js:100,20]",
"WARN: Dropping unused variable c [test/compress/issue-1034.js:102,16]"
// duplicate warnings no longer emitted
"WARN: Dropping unreachable code [test/compress/issue-1034.js:95,16]",
"WARN: Declarations in unreachable code! [test/compress/issue-1034.js:95,16]",
"WARN: Dropping unreachable code [test/compress/issue-1034.js:97,12]",
"WARN: Declarations in unreachable code! [test/compress/issue-1034.js:97,12]",
"WARN: Dropping unreachable code [test/compress/issue-1034.js:101,12]",
"WARN: Dropping unused variable b [test/compress/issue-1034.js:95,20]",
"WARN: Dropping unused variable c [test/compress/issue-1034.js:97,16]"
]
}

11 changes: 11 additions & 0 deletions test/mocha/minify.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
var Uglify = require('../../');
var assert = require("assert");

describe("minify", function() {
it("Should test basic sanity of minify with default options", function() {
var js = 'function foo(bar) { if (bar) return 3; else return 7; var u = not_called(); }';
var result = Uglify.minify(js, {fromString: true});
assert.strictEqual(result.code, 'function foo(n){return n?3:7}');
});
});

2 changes: 1 addition & 1 deletion test/run-tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ function run_compress_tests() {
if (test.mangle_props) {
input = U.mangle_properties(input, test.mangle_props);
}
var output = input.transform(cmp);
var output = cmp.compress(input);
output.figure_out_scope();
if (test.mangle) {
output.compute_char_frequency(test.mangle);
Expand Down
2 changes: 1 addition & 1 deletion tools/node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ exports.minify = function(files, options) {
UglifyJS.merge(compress, options.compress);
toplevel.figure_out_scope();
var sq = UglifyJS.Compressor(compress);
toplevel = toplevel.transform(sq);
toplevel = sq.compress(toplevel);
}

// 3. mangle properties
Expand Down