934799 - Make turning on debug mode for all compartments not delazify all lazy scripts

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

The remaining pain point in bug 815603 once bug 933882 is fixed is that entering debug mode has a callstack like this:

  nsXPConnect::CheckForDebugMode
  JS_SetDebugModeForAllCompartments
  JSCompartment::setDebugModeFromC
  CreateLazyScriptsForCompartment

and this last is taking a bunch of self time, at least in my testcase with several gmails loaded.

It's not obvious why we need this delazification here, apart from "makes it pass tests".  Some obvious options, but keep in mind we may want to backport whatever fixes we end up with here to Aurora 27, or even at a stretch Beta 26, because what Firebug is ending up doing to work around bug 815603 sucks a lot:

1)  Make jsd1 play well with lazy scripts as needed.  This might be hard, of
    course.
2)  Add a boolean to JSCompartment to indicate whether it has any lazy scripts
    and skip the work if it's not set.  This means the first debugger activation
    in a large session will still be slow-ish, and later ones will need to
    delazify everything that got loaded after the last debugger pause().  It
    also doesn't play well with relazification ideas, though we could just
    disable relazification on compartments that get delazified this way, just
    like we disable lazy script compilation for them.
3)  Make CreateLazyScriptsForCompartment faster.  I can't get useful
    line-by-line info out of my profiler for some reason, unfortunately, so I
    don't know which parts of this are "slow", if any.

Comment 1

[Shu-yu Guo [:shu]]

This has a funny history. Before bug 678037 landed, Debugger was ensuring that interpreted functions had scripts by calling getOrCreateScript(). After bug 678037, all those checks were removed in favor of the CellIter that delazified all scripts. Then came bug 897919, where the removal of the logic dealing with lazy scripts resulted in crashes when dealing with self-hosted code, which also have lazy scripts.

Comment 2

[Shu-yu Guo [:shu]]

This is not so easy to fix because Debugger.prototype.findScripts needs to be able to see all scripts. Without changing how the Debugger works fundamentally, I'm going to play with the following approach.

 - Iterate over all *functions* instead of *scripts* in findScripts, delazifying scripts as needed.
 - Since we iterate over functions, we need a way to find the non-function scripts.
    - When we toggle debug mode, we have to iterate over all scripts to discard JIT code anyways. Piggyback on this and snapshot a list of non-function scripts when turning on debugMode.
    - Afterwards, since debugMode is now on and that non-functions scripts shouldn't be lazy (someone check me on this), we should observe all non-function scripts by way of fireNewScript. We can piggyback on this hook to synch the non-function scripts vector.

Comment 3

[Shu-yu Guo [:shu]]

Nope, that doesn't work because the delazifying a script can create new functions while we're in a CellIter iterating over functions.

Comment 4

[Shu-yu Guo [:shu]]

Attachment: Don't delazify all lazy scripts in a compartment when toggling debug mode. (r=?)

Just going to do the obvious, dumb thing here: set a flag to signal we need to
delazify the compartment, and do so lazily in Debugger.prototype.findScripts.

No change to net performance, just moves it to later in time.

Comment 5

[Shu-yu Guo [:shu]]

Attachment: Lazify delazifying lazy scripts in debug mode. (r=?)

Creating LazyScripts will flip a bit on the compartment's debugModeBits so that
next time findScripts is called, we delazify the compartment. Also optimizes
the delazifying function a bit by iterating over LazyScript cells to find root
lazy functions instead of iterating over all functions.

Comment 6

[Jim Blandy :jimb]

Comment on attachment 828578 [details] [diff] [review]
Lazify delazifying lazy scripts in debug mode. (r=?)

Review of attachment 828578 [details] [diff] [review]:
-----------------------------------------------------------------

Still reviewing...

::: js/src/jscompartment.cpp
@@ +713,5 @@
>  
> +bool
> +JSCompartment::ensureDelazifyScriptsForDebugMode(JSContext *cx)
> +{
> +    fprintf(stderr, "need delazy on %p: %d\n", this, debugModeBits & DebugNeedDelazification);

Left a debugging printf in...

Comment 7

[Jim Blandy :jimb]

Comment on attachment 828578 [details] [diff] [review]
Lazify delazifying lazy scripts in debug mode. (r=?)

Review of attachment 828578 [details] [diff] [review]:
-----------------------------------------------------------------

I get that this just adds a new layer  of laziness to the system: we put off delazifying scripts for the debugger until the debugger actually asks for it.

How does this support JSD, though? Does JSD only enumerate scripts created while it was watching? Or is Firebug simply not using that part of JSD any more?

(Note that the fprintfs need to come out.)

Comment 8

[Shu-yu Guo [:shu]]

(In reply to Jim Blandy :jimb from comment #7)
> Comment on attachment 828578 [details] [diff] [review]
> Lazify delazifying lazy scripts in debug mode. (r=?)
> 
> Review of attachment 828578 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I get that this just adds a new layer  of laziness to the system: we put off
> delazifying scripts for the debugger until the debugger actually asks for it.
> 
> How does this support JSD, though? Does JSD only enumerate scripts created
> while it was watching? Or is Firebug simply not using that part of JSD any
> more?

That's a good question, I'll needinfo? sfink.

Comment 9

[Shu-yu Guo [:shu]]

Attachment: Part 2: Disable lazy parsing for JSD. (r=?)

Comment 10

[Steve Fink [:sfink] [:s:]]

Comment on attachment 829403 [details] [diff] [review]
Part 2: Disable lazy parsing for JSD. (r=?)

Review of attachment 829403 [details] [diff] [review]:
-----------------------------------------------------------------

shu: the only thing that bothers me is that whoever split up the debug mode bits in the first place (jimb? jorendorff?) carefully referred to them as "from C" vs "from JS". In practice that maps to "JSD" vs "Debugger", but I'd rather not introduce a mention of JSD for one API but not the others.

r+ if you rename it to something without jsd in it. Otherwise, if you disagree, we can discuss it.

Comment 11

[Steve Fink [:sfink] [:s:]]

For the record: JSD only hands back scripts created (or encountered) while it is on. The IDL comments mention a magical "autostart" preference that would make enumerateScripts return all scripts, but it doesn't seem to exist in the implementation. The basic model seems heavily tied to "turn it on, then reload".

I don't think JSD would be happy dealing with scripts that got lazified behind its back. Not that I really know what these lazy scripts are. JSD keeps script metadata in its own mirror objects, keyed off of the JSScript* pointer value, so it probably wouldn't be insanely hard to make JSD handle lazy scripts if absolutely necessary. But why touch JSD code when you could be spending a pleasant evening ramming chopsticks up your nose instead?

Comment 12

[Till Schneidereit [:till]]

(In reply to Steve Fink [:sfink] from comment #11)
> I don't think JSD would be happy dealing with scripts that got lazified
> behind its back.

We don't currently ever lazify formerly-non-lazy functions, so this can't happen. In bug 886193 I want to change that, but I don't know when I'll be able to get to it.

Comment 13

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae6f2151610f
https://hg.mozilla.org/integration/mozilla-inbound/rev/82495f0c5da2

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out as part of https://hg.mozilla.org/integration/mozilla-inbound/rev/67f5d934127c because something in the push caused various test bustage on Linux ASAN's browser-chrome suite: 

https://tbpl.mozilla.org/php/getParsedLog.php?id=30444281&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=30444457&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=30445445&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=30445739&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=30445414&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=30446297&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=30446502&tree=Mozilla-Inbound

Shu and RyanVM both suspect that https://hg.mozilla.org/integration/mozilla-inbound/rev/ce4011f33422 was the cause for the failures, but I backed out the whole push just to be sure.

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5e6899ab5ead
https://hg.mozilla.org/mozilla-central/rev/9f3212effb9f

Comment 16

[Carsten Book [:Tomcat]]

(In reply to Wes Kocher (:KWierso) from comment #14)
> Shu and RyanVM both suspect that
> https://hg.mozilla.org/integration/mozilla-inbound/rev/ce4011f33422 was the
> cause for the failures, but I backed out the whole push just to be sure.

backout completed as part of https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=494827a9190e as part of discussion in https://bugzilla.mozilla.org/show_bug.cgi?id=937997#c48

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b0e18dd50141
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0bfc071cd47c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4e3a84277bd2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/01b9888de10c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5c1d58a7dfc9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3382fad9edf0
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/49229e7d1afd
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/494827a9190e

however see bug 937997 for investigations going on for the root cause and tree reopening

Comment 17

[Shu-yu Guo [:shu]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/774ca6ed0745
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/862e92d55d50

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/774ca6ed0745
https://hg.mozilla.org/mozilla-central/rev/862e92d55d50

Comment 19

[Shu-yu Guo [:shu]]

Attachment: Part 1: Lazify delazifying lazy scripts in debug mode. (

Comment 20

[Shu-yu Guo [:shu]]

Comment on attachment 8341343 [details] [diff] [review]
Part 1: Lazify delazifying lazy scripts in debug mode. (

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
  Bug 754201 most recently. Not sure what introduced AutoDebugModeGC initially.

User impact if declined: 
  Firebug unusable with many tabs. Firebug has worked around this by disabling JITs permanently if it's on, which is in some ways worse since it slows down *all* tabs.

Testing completed (on m-c, etc.): 
  On m-c.

Risk to taking this patch (and alternatives if risky): 
  GC behavior changes surfacing existing leaks/OOMs. When landing on m-c new leaks were surfaced (and eventually fixed). I am nominating those fixes for uplift also.

String or IDL/UUID changes made by this patch:
  None

The Firebug slowness metabug is bug 815603. List of bugs I'm nominating for uplift in this batch:

 - bug 936143
 - bug 935228
 - bug 933882
 - bug 935470 
 - bug 942346
 - bug 934799

Comment 21

[Shu-yu Guo [:shu]]

Attachment: Part 2: Disable lazy parsing for JSD. (

Comment 22

[Shu-yu Guo [:shu]]

Comment on attachment 8341352 [details] [diff] [review]
Part 2: Disable lazy parsing for JSD. (

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
  Bug 754201 most recently. Not sure what introduced AutoDebugModeGC initially.

User impact if declined: 
  Firebug unusable with many tabs. Firebug has worked around this by disabling JITs permanently if it's on, which is in some ways worse since it slows down *all* tabs.

Testing completed (on m-c, etc.): 
  On m-c.

Risk to taking this patch (and alternatives if risky): 
  GC behavior changes surfacing existing leaks/OOMs. When landing on m-c new leaks were surfaced (and eventually fixed). I am nominating those fixes for uplift also.

String or IDL/UUID changes made by this patch:
  None

The Firebug slowness metabug is bug 815603. List of bugs I'm nominating for uplift in this batch:

 - bug 936143
 - bug 935228
 - bug 933882
 - bug 935470 
 - bug 942346
 - bug 934799

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8341343 [details] [diff] [review]
Part 1: Lazify delazifying lazy scripts in debug mode. (

We can take this on Aurora and thus it will be on Beta in a week, but we're way past the point of taking untracked, major change to Firefox 26 as we are going to build our final candidate today as well as the RC.

Comment 24

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/69910444a8d5
https://hg.mozilla.org/releases/mozilla-aurora/rev/7245e4b8d5da

Comment 25

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Follow-up: https://hg.mozilla.org/releases/mozilla-aurora/rev/9bc4a9b4064f