Sweeping breakpoints is very slow when browser console is open

RESOLVED FIXED in mozilla24

Status

()

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: billm, Assigned: billm)

Tracking

unspecified
mozilla24
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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

6 years ago
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?
Flags: needinfo?(wmccloskey)
Flags: needinfo?(mihai.sucan)
(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.
Flags: needinfo?(mihai.sucan)
(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.
Flags: needinfo?(wmccloskey)

Comment 4

5 years ago
(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.
Created attachment 759061 [details] [diff] [review]
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.
Attachment #759061 - Flags: review?(wmccloskey)

Comment 6

5 years ago
Comment on attachment 759061 [details] [diff] [review]
Proposed fix

Changing the reviewer to Terrence, since Bill's on vacation.
Attachment #759061 - Flags: review?(wmccloskey) → review?(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.
Attachment #759061 - Flags: review?(terrence) → review+

Comment 8

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/fab0b456189c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla24

Comment 9

5 years ago
(Sorry, R/F is not appropriate for inbound...)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/fab0b456189c
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.