Loading breakpoints is slow on pages with many scripts

VERIFIED FIXED in Firefox 66

Status

P2
normal
VERIFIED FIXED
4 months ago
28 days ago

People

(Reporter: mattheww, Assigned: mattheww)

Tracking

(Blocks: 1 bug)

65 Branch
Firefox 66
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 months ago
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:65.0) Gecko/20100101 Firefox/65.0

Steps to reproduce:

The app I'm working on has about 250 scripts, which are not bundled at all in debug mode.  Loading pages with any breakpoints set takes several seconds longer.  I understand that I have a lot of scripts, but this still seems like too much time.


Actual results:

This is a fairly typical profile, with 5 breakpoints set: https://perf-html.io/public/fae3b88abc1c4e731cca8cd8180f8aaa39e92688/flame-graph/?globalTrackOrder=6-0-2-3-4-5-1-7-8-9-10-11&hiddenGlobalTracks=1-2-3-5-6-7-9-10&localTrackOrderByPid=16804-0~3108-0~&thread=11&transforms=ff-180~mf-170~mcn-combined-Bk_j_k_oz_s~mcn-combined-Bk_j_k_oN0&v=3
It shows several seconds spent in Debugger::findScripts(), and GetScriptLineExtent() in particular.

Looking up the stack, it appears that _addSource() is called for every script, which calls _setBreakpointAtGeneratedLocation() for every breakpoint, which calls Debugger::findScripts(), which calls ScriptQuery::consider() for every script.  This would mean that loading breakpoints for the entire page takes quadratic time with respect to the number of scripts.


Expected results:

I think that the slowdown could be avoided by filtering breakpoints down to those that match the script passed to _addSource().  This wouldn't stop multiple breakpoints in the same file all calling findScripts() for that file, but it would stop them calling it for every single script.

This is more tentative, but I think the commonFilter check in consider() would benefit from being moved ahead of the line and possibly realm checks, as well.  The URL and source comparisons seem like they should be pretty cheap and would have a very low false-positive rate.
Component: Untriaged → Debugger
Product: Firefox → DevTools
(Assignee)

Comment 1

3 months ago
Only attempt to add breakpoints for the current source in Thread._addSource(),
reducing the number of costly Debugger::findScripts() calls made when loading a
page.

In addition, speed up findScripts() itself by moving the cheaper URL/source
checks in commonFilter() ahead of the line number checks.
(Assignee)

Comment 2

3 months ago
I created a benchmark page that loads jquery 100 times: https://wartmanm.github.io/breakpoint-benchmark/
In each of these profiles, I set a breakpoint on line 10347 of jquery number 100 and reloaded the page.

Here is the original: https://perfht.ml/2GqfSKQ
1600ms is spent in _addSource, 31% of which is in consider().

In this one, commonFilter() is moved to the top: https://perfht.ml/2GvR2Jl
1100ms is spent in _addSource, 1% of which is in consider().
I tested this without the _addSource change, because calling consider() 9900 fewer times would make the effect that much less noticeable.

Finally, this one has commonFilter() moved, and filtering added to _addSource(): https://perfht.ml/2Gswxxl
700ms is spent in _addSource, almost all of which is spent in delazifying.
Thanks so much for the thorough investigation into all of this. This logic is definitely suboptimal at the moment, good find. If you're interested in contributing more, we have a Slack instance Devtools discussions: https://devtools-html-slack.herokuapp.com/
Blocks: 1122748
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2

Comment 4

2 months ago
Pushed by lsmyth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/957cc2dce89c
Improve performance when loading breakpoints on pages with many scripts r=loganfsmyth

Comment 5

2 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox66: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66
Assignee: nobody → mattheww
(Assignee)

Updated

2 months ago
Status: RESOLVED → VERIFIED

Thanks again for this!

Thanks for this patch, this had a significant impact on DAMP:

== Change summary for alert #18687 (as of Thu, 10 Jan 2019 17:54:21 GMT) ==

Improvements:

6% damp custom.jsdebugger.pause.DAMP linux64 pgo e10s stylo 348.73 -> 327.91
5% damp custom.jsdebugger.pause.DAMP windows7-32 opt e10s stylo 402.41 -> 381.99
4% damp custom.jsdebugger.pause.DAMP windows10-64 opt e10s stylo 408.64 -> 390.40
4% damp custom.jsdebugger.pause.DAMP linux64 opt e10s stylo 400.30 -> 382.50
4% damp custom.jsdebugger.pause.DAMP windows10-64-qr opt e10s stylo 405.86 -> 388.15

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

Depends on: 1530423
You need to log in before you can comment on or make changes to this bug.