Closed Bug 1829640 Opened 11 months ago Closed 11 months ago

2.65 - 56.86% damp custom.jsdebugger.preview.DAMP + more (Linux, OSX, Windows) regression Mon April 24 2023

Categories

(DevTools :: Debugger, defect, P2)

defect

Tracking

(firefox115 fixed)

RESOLVED FIXED
115 Branch
Tracking Status
firefox115 --- fixed

People

(Reporter: jdescottes, Assigned: ochameau)

Details

Attachments

(1 file)

== Change summary for alert #38190 (as of Mon, 24 Apr 2023 03:26:28 GMT) ==

Regressions:

Ratio Test Platform Options Absolute values (old vs new)
57% damp custom.jsdebugger.stepIn.DAMP linux1804-64-shippable-qr e10s fission stylo webrender 748.71 -> 1,174.41
57% damp custom.jsdebugger.stepIn.DAMP linux1804-64-shippable-qr e10s fission stylo webrender-sw 741.59 -> 1,163.14
47% damp custom.jsdebugger.stepOver.DAMP macosx1015-64-shippable-qr e10s fission stylo webrender-sw 562.01 -> 827.50
44% damp custom.jsdebugger.stepOver.DAMP linux1804-64-shippable-qr e10s fission stylo webrender 466.84 -> 671.17
42% damp custom.jsdebugger.stepOver.DAMP macosx1015-64-shippable-qr e10s fission stylo webrender 572.59 -> 814.09
42% damp custom.jsdebugger.stepOver.DAMP linux1804-64-shippable-qr e10s fission stylo webrender-sw 464.14 -> 658.61
39% damp custom.jsdebugger.stepIn.DAMP windows10-64-shippable-qr e10s fission stylo webrender-sw 650.58 -> 907.16
39% damp custom.jsdebugger.stepOver.DAMP windows10-64-shippable-qr e10s fission stylo webrender-sw 405.45 -> 562.91
39% damp custom.jsdebugger.stepIn.DAMP windows10-64-shippable-qr e10s fission stylo webrender 655.75 -> 910.29
34% damp custom.jsdebugger.project-search.DAMP macosx1015-64-shippable-qr e10s fission stylo webrender 978.30 -> 1,315.21
... ... ... ... ...
11% damp custom.jsdebugger.project-search.first-search-result.DAMP macosx1015-64-shippable-qr e10s fission stylo webrender-sw 78.99 -> 87.88
11% damp custom.jsdebugger.project-search.first-search-result.DAMP macosx1015-64-shippable-qr e10s fission stylo webrender 77.04 -> 85.42
7% damp custom.jsdebugger.project-search.first-search-result.DAMP windows10-64-shippable-qr e10s fission stylo webrender 87.14 -> 93.28
6% damp custom.jsdebugger.project-search.first-search-result.DAMP windows10-64-shippable-qr e10s fission stylo webrender-sw 87.26 -> 92.53
3% damp custom.jsdebugger.preview.DAMP linux1804-64-shippable-qr e10s fission stylo webrender-sw 531.59 -> 545.66

Improvements:

Ratio Test Platform Options Absolute values (old vs new)
7% damp complicated.jsdebugger.close.DAMP linux1804-64-shippable-qr e10s fission stylo webrender-sw 28.14 -> 26.11
7% damp complicated.jsdebugger.close.DAMP windows10-64-shippable-qr e10s fission stylo webrender 25.77 -> 24.01
7% damp complicated.jsdebugger.close.DAMP windows10-64-shippable-qr e10s fission stylo webrender-sw 25.71 -> 24.03
6% damp complicated.jsdebugger.close.DAMP linux1804-64-shippable-qr e10s fission stylo webrender 27.90 -> 26.24
6% damp complicated.jsdebugger.close.DAMP macosx1015-64-shippable-qr e10s fission stylo webrender-sw 18.72 -> 17.63
... ... ... ... ...
3% damp custom.jsdebugger.close.DAMP windows10-64-shippable-qr e10s fission stylo webrender-sw 28.51 -> 27.54

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=38190

Potential regressors:

Bug 1822301 - Clear Sources content reducer on each target/thread removal
Bug 1827909 - Use mutable maps in sources reducer
Bug 1822302 - Clear breakable lines and positions in sources reducer on each target/thread removal
Bug 1822307 - Clear highlightedLineRange in UI reducer on each target/thread removal

Severity: -- → S3
Priority: -- → P2

I really focused on the stepIn regression, and it seems to have specifically regressed because of this changeset: https://hg.mozilla.org/integration/autoland/rev/dec5a35facabc46cbf1ee212e688f5091ed2b658

Bug 1822302 - [devtools] Use mutable map for breakpoint positions. r=devtools-reviewers,nchevobbe

However my profiles are not extremely helpful either. I can see a lot more reselect activity with this changeset (with, without) but I don't have any interpretation for that yet.

The DAMP comparison seems to confirm that this changeset is responsible for most of the regression: https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=841671e3ee9219f0daac3e9e88a7ddc4178f6e94&originalSignature=3130994&newSignature=3130994&framework=12&application=&originalRevision=02d7f08f3ade13370408cc3ec55f6971e948c8ed&page=1&showOnlyImportant=1

With the patch the custom.debugger DAMP test performs almost 700 calls to the getColumnBreakpoints select, less than 50 without the patch.
And this directly translates to re-renders of some components, such as the ColumnBreakpoints component.

Hi Alex,

I tried to investigate a bit this regression. As mentioned in the comment above, the column breakpoints components are rendered a lot more with the changeset from https://hg.mozilla.org/integration/autoland/rev/dec5a35facabc46cbf1ee212e688f5091ed2b658

I am a bit lost when it comes to pros and cons of using reselect or not, but maybe some of those memoized selectors were preventing re-renders?

Flags: needinfo?(poirot.alex)

Oh! Thanks a lot for the analysis. Somehow I ended up with different conclusion from the profiler. I haven't seen getColumnBreakpoints/reselect, but only CodeMirror differences. May be that was only the re-renders which are the main overhead while the cause, selectors aren't necessarily visible.

It sounds like my change to getVisibleBreakpointPositions may force to return new empty arrays instances or not be cached if selected source and positions did not change. This would lead to call getColumnBreakpoints more frequently.

export const visibleColumnBreakpoints = createSelector(
  getVisibleBreakpointPositions,
  getVisibleBreakpoints,
  getViewport,
  getSelectedSource,
  getSelectedSourceTextContent,
  getColumnBreakpoints
);

This has been spotted during the review but I thought it would be harmless, but these selectors are much more complicated than what I assumed!
We might be able to inline some selector computation into the reducer so that selectors are simpler.
Otherwise, I got rid of createSelector because it is complex to make it accept an argument -or- have a first selector returned value to become an argument.

Reusing a more convoluted createSelector seems to bring back significant improvements:
https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=de8c69691968955a6d7926beaf1114a2925da4b2&originalSignature=4763575&newSignature=4763575&framework=12&application=firefox&originalRevision=4293799f4c7edce8f4abde4614a74a1fbfb04ea0&page=1&showOnlyImportant=1&pageTitle=Use+createSelector
And using shallowEqual doesn't improve performance further:
https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=c38537db9372c6167adc3edce5b5beb8523b2d64&originalSignature=4763575&newSignature=4763575&framework=12&application=firefox&originalRevision=de8c69691968955a6d7926beaf1114a2925da4b2&page=1&pageTitle=Use+shallow+equal+on+top+of+createSelector

Flags: needinfo?(poirot.alex)
Assignee: nobody → poirot.alex
Status: NEW → ASSIGNED
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7085887b4a17
[devtools] Memoize getVisibleBreakpointPositions selector by using createSelector. r=jdescottes,devtools-reviewers
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → 115 Branch

== Change summary for alert #38365 (as of Wed, 17 May 2023 22:58:11 GMT) ==

Improvements:

Ratio Test Platform Options Absolute values (old vs new)
36% damp custom.jsdebugger.stepIn.DAMP linux1804-64-shippable-qr e10s fission stylo webrender-sw 1,141.98 -> 728.38
36% damp custom.jsdebugger.stepIn.DAMP linux1804-64-shippable-qr e10s fission stylo webrender 1,151.32 -> 741.24
32% damp custom.jsdebugger.stepOver.DAMP macosx1015-64-shippable-qr e10s fission stylo webrender 854.63 -> 577.58
32% damp custom.jsdebugger.stepIn.DAMP windows10-64-shippable-qr e10s fission stylo webrender 948.06 -> 642.12
32% damp custom.jsdebugger.stepIn.DAMP windows10-64-shippable-qr e10s fission stylo webrender-sw 943.77 -> 640.18
... ... ... ... ...
6% damp custom.jsdebugger.preview.DAMP linux1804-64-shippable-qr e10s fission stylo webrender 544.23 -> 512.42

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=38365

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

Attachment

General

Created:
Updated:
Size: