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

RESOLVED FIXED in mozilla27

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: luke, Assigned: bhackett)

Tracking

unspecified
mozilla27
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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);
    }
})(N);

'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.
(Assignee)

Comment 1

5 years ago
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.
(Reporter)

Comment 2

5 years ago
I don't know if this was already fixed or it dropped off your radar.  Seems pretty important to fix.
(Reporter)

Updated

5 years ago
Flags: needinfo?(bhackett1024)
(Assignee)

Comment 3

5 years ago
Created attachment 814592 [details] [diff] [review]
patch

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)
(Reporter)

Comment 4

5 years ago
Comment on attachment 814592 [details] [diff] [review]
patch

Great, thanks!
Attachment #814592 - Flags: review?(luke) → review+
https://hg.mozilla.org/mozilla-central/rev/c71abced5149
Status: NEW → RESOLVED
Last Resolved: 5 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?
https://groups.google.com/d/topic/mozilla.dev.tree-management/rFS0EPoKW5c/discussion
(Assignee)

Updated

5 years ago
Depends on: 927383
(Assignee)

Comment 8

5 years ago
(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?
> https://groups.google.com/d/topic/mozilla.dev.tree-management/rFS0EPoKW5c/
> discussion

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