1578219 - DAMP Perf regression in debugger stepIn test (+40%)

Status: RESOLVED WONTFIX

(DevTools :: Debugger, defect, P2)

Description

[Julian Descottes [:jdescottes]]

Performance regression on the debuger stepIn test, most likely related to Bug 1576679 (Enable Inline Preview for all channels).

Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=cd3f609f008112adf2fdba0180bfe1bb9802cfe8&tochange=8f134018ed03d592cd9dc27ef641565163472bbd

Alert summary:
== Change summary for alert #22876 (as of Wed, 28 Aug 2019 16:03:31 GMT) ==

Regressions:

34% damp custom.jsdebugger.stepIn.DAMP linux64-shippable-qr opt e10s stylo 386.42 -> 517.35
33% damp custom.jsdebugger.stepIn.DAMP windows10-64-shippable-qr opt e10s stylo 392.63 -> 520.27
30% damp custom.jsdebugger.stepIn.DAMP linux64-shippable opt e10s stylo 381.08 -> 494.02
9% damp custom.jsdebugger.stepOut.DAMP linux64-shippable opt e10s stylo 369.78 -> 402.31
3% damp custom.jsdebugger.stepOver.DAMP linux64-shippable-qr opt e10s stylo 259.25 -> 267.22

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

Comment 1

[David Walsh :davidwalsh]

Attachment: Bug 1578219 - Improve performance regression for Inline Preview r=jlast

Comment 2

[David Walsh :davidwalsh]

After much profiling and investigation I've isolated the problem to the following line: https://searchfox.org/mozilla-central/source/devtools/client/debugger/src/actions/pause/inlinePreview.js#66

await parser.getScopes(frame.location);

The parser worker is taking the contents of step-in-test.js, asking Babel to parse it, and return the scopes. Parsing and analyzing a 160kb file is causing the perf hit due to the about of time Babel requires. As I can't force Babel to go faster, we're in sort of a weird spot. We could find an alternative to Babel but that's obviously a much larger task.

A few ideas:

1. Maybe we don't provide inline preview for files larger than a certain kb
2. Greg mentioned that the profiler doesn't use workers because they can be slow; i.e. the time during postMessage calls is "wasted", whereas simply doing the work doesn't waste any time. We could try not using workers(?)

I don't think this is an issue with a simple quick fix.

Comment 3

[David Walsh :davidwalsh]

It appears that Chrome uses acorn.js for parsing: https://github.com/acornjs/acorn

Comment 4

[David Walsh :davidwalsh]

Profile for the StepIn test

Inline Preview on: https://profiler.firefox.com/public/763db196ddf808d7230e943ad5d59eae3ef51bf5/flame-graph/?globalTrackOrder=0-1-2-3-4-5&hiddenGlobalTracks=1-2-3-4-5&hiddenLocalTracksByPid=95454-1-2-0&implementation=js&localTrackOrderByPid=95454-1-2-0~95458-0-1~95455-0~95459-0~95456-0~95457-0~&range=5.3067_12.7561~10.4221_11.3638&thread=0&transforms=df-597~df-1678~df-1970~df-1649&v=4

Inline Preview off: https://profiler.firefox.com/public/1ad547e9b77b4521e7053b37d92241717b169854/flame-graph/?globalTrackOrder=0-1-2-3-4-5&hiddenGlobalTracks=1-2-3-4-5&hiddenLocalTracksByPid=96558-1-2-0&implementation=js&localTrackOrderByPid=96558-1-2-0~96562-0-1~96561-0~96560-0~96563-0~96559-0~&range=5.2174_12.3274~10.2740_11.0700&thread=0&transforms=df-421~df-1696~df-1808~df-972&v=4

You can see the "worker runnable" blocks are much larger with Preview on.

Comment 5

[David Walsh :davidwalsh]

Attachment: InlinePreviewDoesntBlockScopes.png

My console.logs make it obvious that the Scopes pane isn't waiting for inline preview to finish so generateInlinePreview, which calls the await parser.getScopes(frame.location) code, isn't holding up other parts of the debugger.

Comment 6

[Sorin Davidoi]

Attachment: perf(debugger): Optimize stepping

• Don't render if the props didn't change
• Avoid accessing codeMirror getter inside loops
• Don't compute the same information twice
• Avoid memory allocations

I couldn't capture two identical enough profiles such that they can be easily compared.

Comment 7

[Phabricator Automation]

Comment on attachment 9101662 [details]
perf(debugger): Optimize stepping

Revision D49477 was moved to bug 1550494. Setting attachment 9101662 [details] to obsolete.

Comment 8

[Jason Laster [:jlast]]

Closing as we understand the impact from the test was due to an new parser.getScopes call and do not intend to change that.