Closed Bug 880085 Opened 9 years ago Closed 8 years ago

named lambdas shouldn't be treated as run-once closures


(Core :: JavaScript Engine, defect)

Not set





(Reporter: luke, Assigned: bhackett1024)




(1 file)

IIUC, the only way a run-once closure should be able to be run multiple times is really pathological stuff like arguments.caller.  However, in this example

(function rec(depth) {
    function unusedFun() {}
    if (depth) {
        rec(depth - 1);

'rec' is being marked treatAsRunOnce.  Based on this mis-speculation, 'unusedFun' (and, in general, all nested functions) will be js::CloneScript()ed N times (since it is speculatively given a singleton type).

Should be simple to fix though.  This has pretty scary perf regression potential so hopefully it can get fixed soon.
One alternative is if we had a way to deoptimize so that if the function is called multiple times the inner scripts are only cloned the first time, and given non-singleton types the second and later executions.  Doing that is more involved, but would allow us to be more speculative about run once functions, i.e. we could give the treatment to a top level function with inner functions / closure vars, with the hope it will only be called once.  That is a speculative optimization though, and for now this proposed fix would be good.
I don't know if this was already fixed or it dropped off your radar.  Seems pretty important to fix.
Flags: needinfo?(bhackett1024)
Attached patch patchSplinter Review
Well, this was "fixed" insofar that run-once lambda optimizations seem to have been broken since lazy parsing landed.  This patch updates things so that run-once lambdas are still emitted with lazy parsing, and doesn't treat named functions as being run-once lambdas.
Assignee: general → bhackett1024
Attachment #814592 - Flags: review?(luke)
Flags: needinfo?(bhackett1024)
Comment on attachment 814592 [details] [diff] [review]

Great, thanks!
Attachment #814592 - Flags: review?(luke) → review+
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
This caused a regression in the CanvasMark benchmark on all platforms.  Has anyone looked into this or filed a bug?
Depends on: 927383
(In reply to Matt Brubeck (:mbrubeck) from comment #7)
> This caused a regression in the CanvasMark benchmark on all platforms.  Has
> anyone looked into this or filed a bug?
> discussion

I just filed bug 927383 for this.  I haven't looked at the regression yet but will this week.
You need to log in before you can comment on or make changes to this bug.