-
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
Fuzz v3 #1697
Fuzz v3 #1697
Conversation
The major bottleneck in the number of cycles per second is without doubt the minification process. Is there a particular way to speed that up? Is that desirable at all? Or is perf of the minifier a low prio? |
Quick tip: use Node.js 4.x instead of 7.x will give you 100% speed boost Performance is not critical in the sense that I only focus on pathological cases, i.e. so slow it makes But before that, I'll need to update this |
test/ufuzz.js
Outdated
@@ -440,7 +502,7 @@ for (var round = 0; round < num_iterations; round++) { | |||
var beautify_result = run_code(beautify_code); | |||
|
|||
try { | |||
var uglify_code = minify(beautify_code, { | |||
var uglify_code = minify(original_code, { |
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.
IIRC there was a prior discussion involving @kzc and we ended up keeping this unchanged.
If we are going to change this, might as well get rid of beautify_result
and move computation of beautify_code
into log()
. That way you should get a nice speed-up.
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 would expect it to test both the beautifier and the minifier this way. I assume they use completely different code paths after parsing. Parsing doesn't appear to be a bottleneck because the beautification is about as fast as I'd expect it and I expect that to use the same parser.
I don't mind either way, you have to use the tool so it's really up to you.
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.
Testing beautify on input revealed a couple of bugs. It would be nice if were an option, disabled by default to speed up fuzzing.
When there's a failure can both the minified output code as well as beautified output code be logged? That way it's easier to diagnose. We have to beautify the output anyway.
I like that. If there's a bounded number of combinations to test then it could easily be done and show you a nice table of "works v doesnt work" kind of result. Should not be too difficult to make and fairly separate from the generation code. In such case you'd probably split the generation to its own file etc. |
fwiw, some numbers on my machine:
|
Once I've rebased #1460, you can see if it gives you better performance. It handles sequences more efficiently, which I imagine is part of the problem when the fuzzer generates a large body of statements for instance. Edit: rebased |
When I tried to run the script with this PR:
|
test/ufuzz.js
Outdated
@@ -122,6 +129,8 @@ var TYPEOF_OUTCOMES = [ | |||
'crap' ]; | |||
|
|||
var FUNC_TOSTRING = 'Function.prototype.toString=function(){return"function(){}"};'; |
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.
@kzc I shall wait on this PR before putting in #1688 (comment) to avoid merge conflicts.
That's fine. My stuff affects one file so I can easily rebase it. Go ahead. |
I tried testing the fuzzer in this PR and noticed that each iteration is several times slower than the existing version. I assume it's because of the complexity of the test produced by each iteration increased? Disabling beautify on the input code by default should speed it up a bit. Having an option to enable it would be nice. |
It's quite exponentially tied to the size of the generated code. Try lowering the As the numbers above should tell you the beautifier doesn't add a lot to the runtime, most of the time is spent in the minifier. So yeah, we can make that optional, but runtime perf won't be noticeably affected by turning it off. |
Mind you, it could also be caused by new additions that trigger slow paths in the minifier. I didn't profile it. The numbers above tell me that the generating part is fine (just like the beautifier). |
If the input code beautifier in the fuzzer on master is disabled it increases fuzzer performance by 16%. |
@kzc I think @qfox is seeing @qfox as discussed in #1700 (comment), I think deeper code generations may not bring about the best efficiency in catching issues. So may be we can back off a bit and avoid this performance issue altogether? |
Indeed - using these settings I was able to get 140 iterations per second for this PR, compared to 125 on master: +var MAX_GENERATED_TOPLEVELS_PER_RUN = 1;
var MAX_GENERATED_FUNCTIONS_PER_RUN = 1;
-var MAX_GENERATION_RECURSION_DEPTH = 15;
+var MAX_GENERATION_RECURSION_DEPTH = 3;
var INTERVAL_COUNT = 100;
+var loops = 0;
+var funcs = 0; By the way, I had to declare a few variables to get this script to run without error: |
You should run with I just scrubbed the options a bit so it'll be |
@alexlamsl yeah that makes sense. I've made the recursion depth optional. You can play around (I suggest running with |
(updated, rebased) |
@alexlamsl btw that
|
As an addendum to the recursion depth (and amount of toplevels generated); since the nature of a fuzzer is very probabilistic generally more means better. There is definitely merit in running with a low recursion value but if that runs stable for a few thousand cycles then a higher recursion depth allows for randomly generating code that refers to other generated variables, weird constructions, etc. With |
Can you post some fuzzed generated code that is producing output with |
test/ufuzz.js
Outdated
|
||
function run_code(code) { | ||
var stdout = ""; | ||
var original_write = process.stdout.write; | ||
process.stdout.write = function(chunk) { | ||
process.stdout.write = function (chunk) { |
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.
While I appreciate the updating of two-space to four-space indentation for consistency, can we avoid these extraneous whitespaces getting inserted in the process?
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.
Sorry, can you be more specific? Your comment is ambiguous to me. Did you mean;
- this particular line that has
process
- just code according to the style guide already
- dont do this kind of whitespace-only update in the future
- d: something else?
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.
(FWIW I've updated my editor for this project to use 4 instead of 2 tabs, which is the biggest reason for the fluctuating indents, so those shouldn't appear anymore. If there's other style guide rules to adhere I'll configure them for sure. I can also replace this commit with those style changes instead. Or just drop the commit if you prefer it that way. It's just an automatic beautification really.)
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.
Sorry about the confusion.
What I meant in this particular case is don't change any whitespaces except for the indentation.
There are a few other places where this happened as well.
@kzc took a while :)
|
$ node -e 'function a(){}; console.log(a); console.log(a.toString());'
[Function: a]
function a(){} |
Any idea how to override how
If not, we have to disable |
Hey guys, I already did that in #1688, but only if you pass it a function directly 😉 https://github.com/mishoo/UglifyJS2/pull/1688/files#diff-723be892ae46591e4816c74a1fe0cf21R137 So if you don't mind, don't pass it an array like |
It's
Can we override that in Function.prototype? |
Could you change it to:
|
So there is one solution to the So that would be ( |
OT: I wonder if we can get rid of this now... https://github.com/mishoo/UglifyJS2/blob/master/lib/output.js#L657-L663 |
Workaround not needed for Safari 9 which I'm using. https://en.wikipedia.org/wiki/Safari_version_history#Mac |
Don't get rid of this one - probably still affects 25% of Safari users:
|
Considering IE8 was released in 2009, I think we should probably keep both of these Safari workarounds. |
I am not aware of a general correct solution to the The naïve implementation of |
@kzc indeed - I see it as a choice between false positives (currently) or false negatives (same string). Actually, come to think of it, my naïve implementation at first was me not wanting to restart the tests manually whenever it hits a false positive. But we can workaround that by That way, combined with my proposed enhancement of singling out option flags that cause a given test failure, should make life easier while still maintaining the best coverage. |
True.
Yeah, it would reduce the burden of isolating the problem. |
Additionally, perhaps after a bad test case is found the original code AST could be scanned for instances of |
The latest commit to the fuzzer has been running without a problem or bug for over a million iterations. Maybe it's time to squash and merge? |
If tests continue beyond a failure please remember to have a non-zero exit code (when max iterations specified). |
Impressive. Well done @qfox and @alexlamsl! |
@qfox would you mind incorporating these two line changes? new vm.Script(FUNC_TOSTRING + code).runInNewContext({
console: {
log: function() {
return console.log.apply(console, [].map.call(arguments, function(arg) {
- return typeof arg == "function" ? "[Function]" : arg;
+ return typeof arg == "function" ? arg.toString() : arg;
}));
}
}
- }, { timeout: 5000 });
+ }, { timeout: 30000 });
return stdout; First one is #1697 (comment), while second one just bumps the execution timeout - if a 4.5GHz core can't run the test code within 5 seconds, I think it's safe to say we need more time 😉 |
I guess the only remaining ES5 features not in the fuzzer are:
Anything else I missed? |
Btw if your test case really needs 30 seconds to run you need to use a lower |
@qfox I agree, though that 30 second is just me getting this check out of the way, but not removing it totally in case something does get stuck in an infinite loop. And BTW, systems with ARM processors do exist 😉 |
In addition, that 30 second is saying "we might get a rare outliner which takes a long time to run". Even if it means say 0.01%, I certainly don't want to restart the fuzzer every ~10,000 runs just because of a false positive. |
mmm can't think of anything else right now |
Sure, but you wouldn't run this fuzzer on them. That's what super computers are built for ;) |
Fix bug where a `throw` was generated without expression Reenable try/catch/finally and fix them up Skip serialization errors Allow function decl in other funcs but not in blocks etc Rename function to be more appropriate Fix global functions not getting certain names Make the canaries more likely to appear as expressions Add a silly rounding edge case Add a new canary, `c`, which should only ever be incremented Refactoring Fix (another) iife not actually being invoked When a statement hits recursion max return an expression instead of `;` When a expression hits recursion max also inc `c` Generate global code as well as function code Also fixes some argument juggling related bugs. No longer reduces the recursion max when generating sub functions. Generates a function arg. Add used names to var name pool while in that scope This is a little wonky, possibly a hack, but since it's synchronous code I think it's alright to do this. The alternative is to slice the varnames array and juggle them through almost all the generator functions and there are various reasons why this patch is a better alternative. Minify generated code, not beautified code. Prevents beautifier bias. Prevent unnecessary duplication Remove serialization protection because I think it got handled elsewhere Abstract toplevel code generation Add example line of running test case Add poor man options parser, and some options Reindent to 4 spaces Lower chance of `default` generation Comment example of testing a case and output improvement Enable `default` clause appearing at any clause index Removing some training wheels; dont add parens where we dont absolutely need them Support `-s1` and `-s2` to force specific statements being generated at that recursion level Add round number to output when failing. For stats and fun and profit. Solidify statement depth counting. The argument juggling is real. Renamed option to something long. -scf was ugly and probably confusing. Fix missing arguments causing `canThrow` to be truthy, generating crashing code Generate more binary nested expressions Add black and white list cli options for statement generation Allows you to explicitly require or forbid certain statements from/to being made. ``` node test/ufuzz.js --without-stmt switch,try -t 5 -r 5 -V ``` ``` node test/ufuzz.js --only-stmt ifelse,expr -t 5 -r 5 -V ``` Similar granularity for expression may be added later. There can be no comma between names; it just does a split on that arg. Trim down the binary expression generator Prevent scoping issues in nodejs by preventing certain names in global space Oh this list was incomplete? Allow bin-expr to generate assignments too. More vigilant with storing and reusing vars. Add more global builtin names Update wrapper code Also patch Function valueOf
sasquatched 👹 ready for commit |
@qfox thanks for all the work - much appreciated 😉 |
Various improvements to the fuzzer.
This is probably my last (major) iteration of the fuzzer for now.