1513095 - Loading breakpoints is slow on pages with many scripts

Status: VERIFIED FIXED

(DevTools :: Debugger, defect, P2)

Description

[mattheww]

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.

Comment 1

[mattheww]

Attachment: Improve performance when loading breakpoints on pages with many scripts

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.

Comment 2

[mattheww]

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.

Comment 3

[Logan Smyth [:loganfsmyth]]

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/

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/957cc2dce89c
Improve performance when loading breakpoints on pages with many scripts r=loganfsmyth

Comment 5

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/957cc2dce89c

Comment 6

[Logan Smyth [:loganfsmyth]]

Thanks again for this!

Comment 7

[Alexandre Poirot [:ochameau]]

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