-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
implement function inlining #2053
Conversation
|
Was curious about timings, so I looked at a couple of files... Without this PR:
With this PR:
So
|
My quick diff over I'm currently scratching my head as to where that speed-up comes from, as it might be indicative of a bug. |
@kzc do you happen to know if there are other examples of large-ish codebases that goes through |
I don't know any off-hand, but most large projects use webpack these days. So they shouldn't be too hard to find. If this optimization inlines an integral part of webpack packaging then it could be a big win. |
|
@alexlamsl Have you tried node 8 yet? Do you know if that node 7 speed regression was addressed with the official release? |
I've eradicated 7 from my systems as soon as 8.0.0 hits, but I haven't got round to evaluating its performance yet. |
Unfortunately the Just finished 🚿 and brewed ☕, so here we go... |
Could you summarize the criteria for inlined functions in this PR? Looking at the patch it appears to only consider functions with a single Might you consider extending this optimization to non-returning single line functions like the following?
This pattern is very common. |
The speed-up is pretty much within $ uglifyjs math.js --timings -c reduce_vars=0 -b bracketize -o min.js
- parse: 0.562s
- scope: 0.281s
- compress: 8.313s
- mangle: 0.000s
- properties: 0.000s
- output: 0.516s
- total: 9.672s
$ uglifyjs math.js --timings -c reduce_vars=1 -b bracketize -o min.js
- parse: 0.515s
- scope: 0.266s
- compress: 3.852s
- mangle: 0.000s
- properties: 0.000s
- output: 0.484s
- total: 5.117s |
That and the function is nameless - the rationale being that there can be no inner variable/function defintions from the function body which may otherwise pollute the parent scope.
I was planning to address general function inlining in a future PR, though now that you've mentioned it, I guess it's no harm making this PR also do:
That way it's not so different from the existing In short, thanks for the suggestion 😉 👍 |
Even if inlining functions could be extended to single-statement function bodies containing just AST_Call or AST_Sequence, that would handle most cases. It would avoid the troublesome cases involving variable declarations and |
@kzc to avoid any confusion from my babbling above - yes it's a great idea, I'm working on it as we speak 👻 |
@kzc added support for single |
Looks good.
|
|
Probably worth fuzzing this PR for a few million cycles as well. Although I don't know if |
It should, as other optimisations would reduce some of the generated functions to a single statement, most likely of |
Yeah, I found such a case: {
var foo_2 = function f10() {
function f11(a_1) {
c = 1 + c, "number" >> /[a2][^e]+$/ <= ("number" ^ 5) > (true >= -3 > ("bar" ^ Infinity));
c = 1 + c, (-4 >= "foo") % ([] != "object") > (-3, 38..toString()) + {} % undefined;
}
var b_1 = f11();
}(--b + (typeof b !== "symbol"));
}
var foo_2=function(){c=1+c,c=1+c,38..toString()}(--b);
var foo_2=(c=1+c,c=1+c,void 38..toString()); Edit: note the bug above in the removal of the |
The |
@alexlamsl The result in #2053 (comment) shows a bug in the final transform regarding the removal of |
Grrr... not sure if want. Oh well. I think I've missed checking |
Exactly what was seen in test case above. |
True enough. I was just excited by the prospect of using that flag for the first time. |
I was hoping I don't need to change any code as well - running with aforementioned modification now. |
Yup, that change made Now, to laser focus: --- a/lib/output.js
+++ b/lib/output.js
@@ -593,6 +593,7 @@ function OutputStream(options) {
// a function expression needs parens around it when it's provably
// the first token to appear in a statement.
PARENS(AST_Function, function(output){
+ if (output.parent() instanceof AST_Dot && output.parent().property == "prototype") return true;
if (first_in_statement(output)) {
return true;
} |
Confirmed - this is all there is between pass and fail: --- a/good.js
+++ b/bad.js
@@ -32938,12 +32938,12 @@ var PDFJS = {};
},
isStream: !0
}, Stream;
- }(), DecodeStream = ((function(str) {
+ }(), DecodeStream = (function(str) {
for (var length = str.length, bytes = new Uint8Array(length), n = 0; length > n; ++n) {
bytes[n] = str.charCodeAt(n);
}
Stream.call(this, bytes);
- }).prototype = Stream.prototype, function() {
+ }.prototype = Stream.prototype, function() {
function DecodeStream() {
this.pos = 0, this.bufferLength = 0, this.eof = !1, this.buffer = null;
} |
Nice detective work. Are you sure it's not a general ECMAScript issue? Did you test on other browsers? Even if it's a WebKit bug I'd be surprised if it was specific to the PARENS(AST_Function, function(output){
+ var p = output.parent();
+ if (p instanceof AST_PropAccess && p.expression === this) return true; Accessing properties directly on a function expression is so rare in practise (because it's useless) that we might always emit the parens in that case. Edit: removed an incorrect Closure example that differed from the issue in question. In a future PR maybe we can drop a property being set directly on a function expression as part of |
Another data point - Babel 5 adds the parens: $ echo 'var a = function(){}.prop = {};' | babel
"use strict";
var a = (function () {}).prop = {};
Babel 6 does not add the parens. |
Let me get home to test this on the full range of web browsers - I only know Node.js has no trouble with lack of parenthesis thus far. As for |
<sarcasm>I don't have $400B to gain access to the required specification.</sarcasm> |
Minimal test case: (function() {1 + 1}.a = 1);
|
Tried |
(function() {1 + 1}.a = 1);
So @alexlamsl Are you inclined to have uglify emit the unnecessary parens behind an output option (
A function expression is no more constant than is a RegExp literal. I think But then again, this particular optimization could be full of snakes for little gain. Maybe best to avoid it. |
Indeed - I was thinking of If somebody is evil enough to define setter on Anyway, let's leave that for another PR and focus on getting |
@kzc #2053 (comment) should be addressed by 71acf65 |
Could you please add a |
There is a But I'll make one as you suggested inside |
- empty body - single `AST_Return` - single `AST_SimpleStatement` - avoid `/*#__PURE__*/` Miscellaneous - enhance single-use function substitution fixes mishoo#281
|
LGTM - well done. |
Enhance single-use function substitution.
fixes #281