-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 --reduce-test
#3722
improve --reduce-test
#3722
Conversation
kzc
commented
Feb 15, 2020
- hoist body of functions and IIFEs
- simplify var declarations
Example:
without this PR:
with this PR:
|
test/exports.js
Outdated
@@ -13,3 +13,4 @@ exports["to_ascii"] = to_ascii; | |||
exports["tokenizer"] = tokenizer; | |||
exports["TreeTransformer"] = TreeTransformer; | |||
exports["TreeWalker"] = TreeWalker; | |||
exports["walk_body"] = walk_body; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably should either be a public export, or a method on TreeWalker. Good enough in test for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just a helper function that can be replicated, unlike List
which is irreplaceable.
In fact, there should be no body instanceof AST_Statement
within uglify-js
, so I'm going to nuke that for a performance bump.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean removing AST_Statement
from uglify-js
altogether?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that extreme − just removing that instanceof
clause since it's unreachable. 😅
@@ -61,9 +61,6 @@ module.exports = function reduce_test(testcase, minify_options, reduce_options) | |||
|
|||
var parent = tt.parent(); | |||
|
|||
// ignore call expressions | |||
if (parent instanceof U.AST_Call && parent.expression === node) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was needed once. Apparently not the case now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can now compare between runtime exceptions properly, so 0()
isn't an issue anymore.
@@ -291,7 +327,7 @@ module.exports = function reduce_test(testcase, minify_options, reduce_options) | |||
} | |||
|
|||
// replace this node | |||
var newNode = new U.parse(REPLACEMENTS[node.start._permute % REPLACEMENTS.length | 0], { | |||
var newNode = U.parse(REPLACEMENTS[node.start._permute % REPLACEMENTS.length | 0], { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed typo. I was surprised this had worked at all with new
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/new
The
new
keyword does the following things:
- Creates a blank, plain JavaScript object;
- Links (sets the constructor of) this object to another object;
- Passes the newly created object from Step 1 as the
this
context;- Returns
this
if the function doesn't return its own object.
But yeah, I didn't notice that either − sorry 😅
Edit: and it's got to be an object (i.e. array works too) − if I say return 42
it would not have any effects at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, that means we can optimise new F(...)
➡️ F(...)
as long as:
typeof F() == "object"
always!F.contains_this()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth a shot. There's always unanticipated found problems in practice. Does ufuzz generate such new
cases with and without this
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested with the evaluate xor bug test suite (tm) - no speed reduction, and reduced test cases are half the size with this PR. |
node.body = []; | ||
body.push(node); | ||
CHANGED = true; | ||
return List.splice(body); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function declaration with a now empty body
was pushed after its original (now hoisted) body
to prevent the future toplevel function call from failing. These calls do not have typeof
checks. The empty function decl and the now useless call will be dropped in the next pass.
test/reduce.js
Outdated
return true; // don't descend into nested functions | ||
} | ||
}); | ||
U.walk_body(fn, tw); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why a simple fn.walk(tw)
won't suffice here?
AFAICT we only need to change node instanceof U.AST_Lambda
above to node instanceof U.AST_Scope && node !== fn
, which is a common pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had what you described but thought it was a waste of CPU. The walk_body does exactly what I want without the need for such a check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, there are two calls to walk_body
in lib/compress.js
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll make the change you suggested, but it's less efficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think walk_body()
is an artifact left over from uglify-es
, as they have that AST_Arrow
madness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AST_Arrow could have been implemented better in hindsight, but I think there's a legitimate case for walk_body
. No matter - I made the changes locally pending testing.
test/reduce.js
Outdated
function to_sequence(expressions) { | ||
if (expressions.length == 0) return new U.AST_Number({value: 0, start: {}}); | ||
if (expressions.length == 1) return expressions[0]; | ||
return new U.AST_Sequence({expressions: expressions, start: {}}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this become an issue if elements of expressions
contains AST_Sequence
?
make_sequence()
handles it like this:
https://github.com/mishoo/UglifyJS2/blob/f0a29902acc24217881529449c7f9b8815896297/lib/compress.js#L954-L959
https://github.com/mishoo/UglifyJS2/blob/f0a29902acc24217881529449c7f9b8815896297/lib/compress.js#L1014-L1021
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to keep it simple. I ran the PR against dozens of ufuzz induced errors in xor, collapse_vars and reduce_vars and never saw the case you're describing come up. But I'll add the logic you mentioned. It will require retesting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made these changes locally but I am unable to come up with a test to exercise it. I suspect what happened before is that the unneeded sequence elements were just dropped by normal reduce_test
operations, and single-element sequences were already converted to a regular expression. Nevertheless, I'll keep the changes and continue to test with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may be correct that it was only done internally to ensure optimal compression.
Thanks for entertaining my worries nonetheless 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem.
Not much left to optimize in reduce_test. It's good enough.
The requested changes were pushed. The xor bug test suite ran the same as before with similar timings. Also ran ufuzz for 10 minutes and it looked fine. |
Don't merge just yet - adding support for hoisting simple statement IIFE function expression body. |
- hoist body of functions and IIFEs - simplify var declarations
Pushed optimization to hoist simple statement IIFEs... Without this PR:
With this PR:
Side note - I preferred the more succinct single line JSON options output. Oh well. In testing I encountered a case with a couple of nested loops with a complex loop condition expression taking hundreds of iterations to reduce. It took over 5 minutes to produce a reduced test case. Consider lowering the max_timeout. It might also be useful to memoize the code and result in a static map for |
When CI finishes it's good to merge. I will be re-retiring from uglify development. |
Indeed the maximum timeout would cause pathological cases to grow in total runtime, however I had frequent trouble with The static map idea sounds interesting, will investigate in due course. Thanks again for your contribution, as always 😉 |
What's left for me is reading the new and improved daily |
A pathological multi-loop xor bug induced test case (not provided) without memoization:
The same test case with the same max timeout with (naive) memoization:
This pathological test case with loops was reduced 10 times faster. A typical test case reduction is only 20% faster. The memoization implementation is left as an exercise to the reader. |
Unfortunately, the
The implementation of memoization I had was naïve with no regard to timeout involved - just Perhaps the cache implementation timing could be improved by having a coarser grained timeout increment. Or perhaps don't clear the cache under certain constraints. Here's the original test case:
Here's the compressor bug needed to reproduce the problem: --- a/lib/compress.js
+++ b/lib/compress.js
@@ -3221 +3221 @@ merge(Compressor.prototype, {
- case "^" : result = left ^ right; break;
+ case "^" : result = left + right; break; //XXX intentionally introduced bug |
I seem to have reduced that case of yours down to something much simpler: var c = 0;
do {
0;
} while ((1 ^ 1) << (c = c + 1));
function f0(undefined_1) {}
f0();
console.log(c); Would you grateful if you can perform sanity check for me − because if this is the case I can come up with a |
That's sufficient to reproduce the problem with c13caf4 with xor bug patch:
Strangely it's even slower than the original case WITHOUT the result cache. |
Thanks − will investigate and fix in a few hours. I think the way to deal with this is to record the original testcase runtime, then if the minified runtime exceeds 100x of that, treat it as failure rather than a timeout to retry. |
fyi, using the following patch on c13caf4 may be sufficient: --- a/test/reduce.js
+++ b/test/reduce.js
@@ -20,7 +20,7 @@ module.exports = function reduce_test(testcase, minify_options, reduce_options)
minify_options = minify_options || { compress: {}, mangle: false };
reduce_options = reduce_options || {};
var max_iterations = reduce_options.max_iterations || 1000;
- var max_timeout = reduce_options.max_timeout || 15000;
+ var max_timeout = reduce_options.max_timeout || 10000;
var verbose = reduce_options.verbose;
var minify_options_json = JSON.stringify(minify_options, null, 2);
var timeout = 1000; // start with a low timeout
@@ -406,7 +406,7 @@ module.exports = function reduce_test(testcase, minify_options, reduce_options)
// can't trust the validity of `code_ast` and `code` when timed out.
// no harm done - just ignore latest change and continue iterating.
if (timeout < max_timeout) {
- timeout += 250;
+ timeout += 5000;
result_cache = Object.create(null);
}
} else if (diff.error) {
|
Oops.... $ uglifyjs test.js -c unsafe_math --reduce-test
// reduce test pass 1, iteration 0: 86 bytes
// reduce test pass 1, iteration 25: 50 bytes
// reduce test pass 1, iteration 50: 50 bytes
// reduce test pass 1, iteration 75: 50 bytes
// reduce test pass 1: 49 bytes
// reduce test pass 2: 49 bytes
// reduce test pass 3: 49 bytes
var a = 1, b = 1;
while (a + 1 > a) {
a *= 2;
}
while (a++ + (1 - b) < a) {
0;
// output:
// minify: Error: Script execution timed out after 15000ms
// options: {
// "compress": {
// "unsafe_math": true
// },
// "mangle": false
// }
} |
Amusing unsafe_math false positive:
|