Closed Bug 1438232 Opened 3 years ago Closed 3 years ago

Can CanReuseScriptForClone be optimized more?

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: jandem, Assigned: jandem)

Details

Attachments

(1 file, 1 obsolete file)

CanReuseScriptForClone returns false when we have a lazy script:

    // We need to clone the script if we're not already marked as having a
    // non-syntactic scope. If we're lazy, go ahead and clone the script; see
    // the big comment at the end of CopyScriptInternal for the explanation of
    // what's going on there.
    return fun->hasScript() && fun->nonLazyScript()->hasNonSyntacticScope();

This will make the caller clone the function + script (after delazifying).

What if we delazify in CanReuseScriptForClone (since we will delazify anyway) and then check hasNonSyntacticScope()? It seems like that could help avoid script clones.

I don't know how common this case is though; should instrument a browser build to get some data.
Attached patch Patch (obsolete) — Splinter Review
Attachment #8951228 - Flags: feedback+
Attached patch PatchSplinter Review
So here's a much nicer patch, thanks to help from Shu.

Let's see if this is also green on Try.
Assignee: nobody → jdemooij
Attachment #8951228 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8951456 - Flags: review+
(In reply to Jan de Mooij [:jandem] from comment #0)
> I don't know how common this case is though; should instrument a browser
> build to get some data.

For posterity: I instrumented a browser build and this definitely hits many times when starting the browser, even more when opening devtools. So I hope this will improve both perf and memory usage.
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/784c1815c3c4
Handle lazy scripts in CanReuseScriptForClone without forcing a delazify/clone. r=shu
https://hg.mozilla.org/mozilla-central/rev/784c1815c3c4
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
AWSY report below. This was a 2-3% JS memory usage win.

mccr8, I think lazy parsing for non-syntactic scopes (bug 1364566) should be a lot more effective now. We no longer unnecessarily delazify + clone scripts when cloning such (lazy) functions.

---

== Change summary for alert #11619 (as of Thu, 15 Feb 2018 23:06:33 GMT) ==

Improvements:

  3%  JS windows7-32 pgo stylo     87,252,061.99 -> 84,582,935.05
  3%  JS windows10-64 opt stylo    115,966,089.39 -> 112,636,003.66
  3%  JS windows7-32 opt stylo     86,772,136.57 -> 84,458,974.43
  3%  JS linux64-stylo-sequential opt stylo-sequential104,128,567.46 -> 101,451,809.46
  2%  JS windows10-64 pgo stylo    115,672,796.91 -> 112,963,230.23
  2%  JS linux64 opt stylo         104,154,544.19 -> 101,914,488.72
  2%  JS osx-10-10 opt stylo       103,851,712.72 -> 101,649,556.17

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=11619
And some perf improvements:

== Change summary for alert #11610 (as of Thu, 15 Feb 2018 23:06:33 GMT) ==

Improvements:

  5%  cpstartup content-process-startup windows7-32 opt e10s stylo     295.67 -> 280.33
  4%  cpstartup content-process-startup windows10-64 opt e10s stylo    301.54 -> 288.92
  4%  cpstartup content-process-startup windows7-32 pgo e10s stylo     263.79 -> 253.33
  3%  cpstartup content-process-startup linux64 pgo e10s stylo         200.38 -> 194.83

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=11610
You need to log in before you can comment on or make changes to this bug.