878486 - Sweeping breakpoints is very slow when browser console is open

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

I've been using my crappy netbook while on vacation, and performance is pretty terrible. One thing I've noticed is that the time for sweeping breakpoints jumps hugely when I open the browser console. Right now it's at ~800ms. If I close the console, it drops. The relevant code is here:
  http://mxr.mozilla.org/mozilla-central/source/js/src/jscompartment.cpp#762
I'm assuming this affects us whenever the debugger is enabled.

Comment 1

[Jim Blandy :jimb]

Thanks for tracking down the hot spot!

Does CellIterUnderGC i(zone(), FINALIZE_SCRIPT) iterate over the JSScripts being finalized, or does it iterate over all scripts? Are there many scripts being GC'd?

I don't think the web console sets breakpoints anywhere, or sets onStep handlers on any stack frames; Mihai, is that correct? If so, we shouldn't be hitting that inner loop at all. I wonder if something's going wrong.

Bill, is there an easy way for me to see the time spent in PHASE_SWEEP_TABLES_BREAKPOINT?

Comment 2

[Mihai Sucan [:msucan]]

(In reply to Jim Blandy :jimb from comment #1)
> ...
> I don't think the web console sets breakpoints anywhere, or sets onStep
> handlers on any stack frames; Mihai, is that correct? If so, we shouldn't be
> hitting that inner loop at all. I wonder if something's going wrong.

The web/browser console does not set any breakpoints or any onStep handlers. This is rather surprising it happens only while the browser console is open.

Comment 3

[Jon Coppeard (:jonco)]

(In reply to Jim Blandy :jimb from comment #1)
> Does CellIterUnderGC i(zone(), FINALIZE_SCRIPT) iterate over the JSScripts
> being finalized, or does it iterate over all scripts? Are there many scripts
> being GC'd?

Unfortunately this iterates over all the scripts in the zone.

So even though there aren't any breakpoints set, it's doing quite a lot of work.

One improvement would be to sweep breakpoints per zone rather than per compartment, since here we iterate scripts by zone and then filter by compartment, for every compartment.

It would also be nice if we could tell whether there were any breakpoints set in a given zone/compartment so we could skip checking every script in this case.

Comment 4

[Jim Blandy :jimb]

(In reply to Jon Coppeard (:jonco) from comment #3)
> One improvement would be to sweep breakpoints per zone rather than per
> compartment, since here we iterate scripts by zone and then filter by
> compartment, for every compartment.

That seems like a good idea.

It might also help to replace:

    if (rt->debuggerList.isEmpty())
        return;

with:

    if (getDebuggees().empty())
        return;

Then, if this compartment is not being debugged, we won't bother with it at all.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: Proposed fix

Bill, I didn't notice that you had assigned this to yourself and wrote a patch for it.  I hope you didn't already do this too.

As far as the fix goes, using getDebuggees().empty() doesn't work because even if all debuggees have been removed there may still be breakpoints that need to be swept.  I tired a more sophisticated scheme of keeping a flag on the zone that was only cleared at the end of GC if there were no debugees left, but this didn't work either.

Anyway this just does the sweeping by zone and it improves the time spent sweeping breakpoints in a browser with 5 JS heavy tabs and the console open from 23-24ms to 0.4-0.5ms on my macbook.

Comment 6

[Jim Blandy :jimb]

Comment on attachment 759061 [details] [diff] [review]
Proposed fix

Changing the reviewer to Terrence, since Bill's on vacation.

Comment 7

[Terrence Cole [:terrence]]

Comment on attachment 759061 [details] [diff] [review]
Proposed fix

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

Stealing review at Jim's request.

r=me with the one nit scratched.

::: js/src/gc/Zone.cpp
@@ +170,5 @@
> +        for (unsigned i = 0; i < script->length; i++) {
> +            BreakpointSite *site = script->getBreakpointSite(script->code + i);
> +            if (!site)
> +                continue;
> +            // nextbp is necessary here to avoid possibly reading *bp after

Comment style in Zone.cpp appears to be /* */, however, this destroy-while-iterating-list pattern is typical enough that I don't really think we need this comment.

Comment 8

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fab0b456189c

Comment 9

[Jim Blandy :jimb]

(Sorry, R/F is not appropriate for inbound...)

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fab0b456189c