Closed Bug 934799 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28
Tracking Status
firefox26 --- wontfix
firefox27 --- fixed
firefox28 --- fixed

People

(Reporter: bzbarsky, Assigned: shu)

References

Details

(Whiteboard: [qa-])

Attachments

(4 files, 1 obsolete file)

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.
Blocks: 815603
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.
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.
Nope, that doesn't work because the delazifying a script can create new functions while we're in a CellIter iterating over functions.
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.
Assignee: nobody → shu
Status: NEW → ASSIGNED
Attachment #827823 - Flags: feedback?(jimb)
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.
Attachment #828578 - Flags: review?(jimb)
Attachment #827823 - Attachment is obsolete: true
Attachment #827823 - Flags: feedback?(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 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.)
Attachment #828578 - Flags: review?(jimb) → review+
(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.
Flags: needinfo?(sphink)
Attachment #829403 - Flags: review?(sphink)
Flags: needinfo?(sphink)
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.
Attachment #829403 - Flags: review?(sphink) → review+
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?
(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.
https://hg.mozilla.org/mozilla-central/rev/5e6899ab5ead
https://hg.mozilla.org/mozilla-central/rev/9f3212effb9f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/774ca6ed0745
https://hg.mozilla.org/mozilla-central/rev/862e92d55d50
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
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
Attachment #8341343 - Flags: approval-mozilla-beta?
Attachment #8341343 - Flags: approval-mozilla-aurora?
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
Attachment #8341352 - Flags: approval-mozilla-beta?
Attachment #8341352 - Flags: approval-mozilla-aurora?
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.
Attachment #8341343 - Flags: approval-mozilla-beta?
Attachment #8341343 - Flags: approval-mozilla-beta-
Attachment #8341343 - Flags: approval-mozilla-aurora?
Attachment #8341343 - Flags: approval-mozilla-aurora+
Attachment #8341352 - Flags: approval-mozilla-beta?
Attachment #8341352 - Flags: approval-mozilla-beta-
Attachment #8341352 - Flags: approval-mozilla-aurora?
Attachment #8341352 - Flags: approval-mozilla-aurora+
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.