2.65 - 56.86% damp custom.jsdebugger.preview.DAMP + more (Linux, OSX, Windows) regression Mon April 24 2023
Categories
(DevTools :: Debugger, defect, P2)
Tracking
(firefox115 fixed)
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
Reporter | ||
Comment 1•11 months ago
|
||
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
Updated•11 months ago
|
Reporter | ||
Comment 2•11 months ago
|
||
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.
Reporter | ||
Comment 3•11 months ago
•
|
||
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.
Reporter | ||
Comment 4•11 months ago
|
||
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?
Assignee | ||
Comment 5•11 months ago
|
||
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
Assignee | ||
Comment 6•11 months ago
|
||
Updated•11 months ago
|
Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7085887b4a17 [devtools] Memoize getVisibleBreakpointPositions selector by using createSelector. r=jdescottes,devtools-reviewers
Comment 8•11 months ago
|
||
bugherder |
Reporter | ||
Comment 9•10 months ago
|
||
== 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
Description
•