Closed
Bug 821361
Opened 12 years ago
Closed 12 years ago
Optimize closures that only run once
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: bhackett1024, Assigned: bhackett1024)
References
Details
Attachments
(1 file)
(deleted),
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
Currently, placing code inside a closure will give considerably worse performance than if that code was at the top level of a page. This is because the engine treats all functions as if they can run multiple times, and optimizations that are done to scripts and data at the top level cannot be applied (top level <script> scripts will only run once).
In many cases, however, these closures will only run once, e.g. they are of the form '(function() { ....})()'. Code should run at the same speed whether it is at the top level or within a closure such as this.
It's not easy to prove that the closure will only run once, as the lambda can still escape through fun.caller accesses while it is still on the stack. So, this I think requires the compiler to optimistically treat these closures as running once, but be able to fall back and deoptimize if that assumption turns out incorrect.
Comment 1•12 years ago
|
||
+1 !! This would make firefox a lot faster for read world websites.
Comment 2•12 years ago
|
||
+1 !! This would make firefox a lot faster for real world websites.
Comment 3•12 years ago
|
||
By the way, this happens on windows also, not only MacOSx
Updated•12 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Comment 4•12 years ago
|
||
I recently read that V8 looks for the standard closure pattern "(function(" and handles it differently (optimizes). I try to dig up the source …
Assignee | ||
Comment 5•12 years ago
|
||
This patch watches for the (function() {...})() pattern at the top level and marks the lambda so that it should be treated as running once (though it may run more than once). This will cause singleton types to be generated for initializers and functions within the lambda, and if the lambda ends up being invoked multiple times its inner functions are deeply cloned. On this benchmark:
x = (function foo() {
var n = 0;
function g() {
this.a = 0;
this.b = 1;
}
function f() {
for (var i = 0; i < n; i++) {
new g();
}
}
function setn(n_) {
n = n_;
}
return {
f: f,
setn: setn
}
})();
x.setn(1000000);
x.f();
This takes time with --ion from 160ms to 40ms. The equivalent script in global code takes 40ms. This hasn't wiped out all overhead from using the closure, as closure variable lookups are still more expensive than global lookups. LICM/GVN in the compiler should mitigate these costs so that closure speed is close to what is seen in global code.
Assignee: general → bhackett1024
Attachment #692010 -
Flags: review?(luke)
Comment 6•12 years ago
|
||
Please note there are several different "styles" for this kind closures / IIFE [1]. This optimization should also (and at least) work with the Crockford style:
(function() {
// ...
}());
[1] http://jsperf.com/immediate-anonymous-function-invocation
Assignee | ||
Updated•12 years ago
|
Attachment #692010 -
Flags: review?(luke) → review?(jdemooij)
Assignee | ||
Updated•12 years ago
|
Attachment #692010 -
Flags: review?(jdemooij) → review?(luke)
Comment 7•12 years ago
|
||
Comment on attachment 692010 [details] [diff] [review]
patch (946bc83d50df)
Looks good. Could you add a test-case that calls a treatAsRunOnce lambda twice and that asserts/faults if you take out the "script->hasBeenCloned = true" statement in CloneFunctionObjectIfNotSingleton? r+ with this.
Attachment #692010 -
Flags: review?(luke) → review+
Assignee | ||
Comment 8•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/88daef90f2ab
Re: comment 6, this works with all the closure styles on that webpage except those which explicitly invoke Function() (which involves a function call, so is harder to handle during parsing).
Comment 9•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in
before you can comment on or make changes to this bug.
Description
•