1943351 - Visibility change handler shouldn't force react render but only freeze redux updates on panel hiding

Status: RESOLVED FIXED

(DevTools :: Framework, defect, P3)

Description

[Alexandre Poirot [:ochameau]]

Right now the VisibilityHandler code, when the panel is hidden, only hold off React updates via React.shouldComponentUpdate:
https://searchfox.org/mozilla-central/rev/5b061cdc4d40d44988dc61aa941cfbd98e31791f/devtools/client/shared/components/VisibilityHandler.js#38-40
while this prevent most of the computations, all connected components get their mapStateToProps being called.
I think this is what later justifies to use React.forceUpdate as their props may already be updated while the panel was hidden.
https://searchfox.org/mozilla-central/rev/5b061cdc4d40d44988dc61aa941cfbd98e31791f/devtools/client/shared/components/VisibilityHandler.js#48

Ideally, we should be holding off execution at an higher level: redux and the connect/mapStateToProps functions.
While the panel are hidden, Redux connect shouldn't call any of the connected component's mapStateToProps methods.
This would prevent any React updates as it would prevent changing any component props.
Once we manage to do that, the update on panel being shown is simple: force a store state change and resume calling the mapStateToProps methods.
With such setup, the React component would switch from the state of props while the panel was last visible, to updated props when the panel is re-shown. Everything should be updating efficiently based on redux and react diffing algorithms.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1943351 - [devtools] Hold off all redux updates when netmonitor is hidden.

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1943351 - [devtools] Also avoid uncessary redux updates when the console is hidden.

Comment 3

[Alexandre Poirot [:ochameau]]

I'm still getting regression reports from DAMP when applying these patches on top of bug 1943364 fixes:
https://perf.compare/subtests-compare-results?baseRev=2530b075b0483b6e9f7e05168f099c2dfcffec1c&baseRepo=try&newRev=3554833720a31a80b12485af29ada90337c0b3af&newRepo=try&framework=12&baseParentSignature=4763542&newParentSignature=4763542&filter_confidence=medium%2Chigh
16% regression on togglePanel

Comment 4

[Alexandre Poirot [:ochameau]]

But note that it is a smaller regression compared to the 25% regression reported when applying these patches directly on m-c without bug 1943364
https://perf.compare/subtests-compare-results?baseRev=32e983880d5f360c847b6bc3a7cd1c93f49623c4&baseRepo=try&newRev=a10573059de1d7f73ab128cda194e63db03e4d6f&newRepo=try&framework=12&baseParentSignature=4763542&newParentSignature=4763542&filter_confidence=medium%2Chigh

Comment 5

[Alexandre Poirot [:ochameau]]

These patches on top of bug 1943543 report no particular perf change:
https://perf.compare/subtests-compare-results?baseRev=b2cddc6b72b2c4bf754ab13f517e2292342a5883&baseRepo=try&newRev=3994c3dd91bb5e79bf31eac8b6dfe36e584f4c89&newRepo=try&framework=12&baseParentSignature=4763542&newParentSignature=4763542&filter_confidence=medium%2Chigh

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1943351 - [DevTools] Hide the console autocomplete popup when opening another panel.

We were hiding it via some late action fired while the panel was already hidden.
This help fix browser_jsterm_autocomplete_mapped_variables.js which was failing
because of the popup consuming the click event when switching to the debugger.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9ad9f4004de4
[devtools] Hold off all redux updates when netmonitor is hidden. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/d958cc2493a6
[devtools] Also avoid uncessary redux updates when the console is hidden. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/bdf540812b20
[DevTools] Hide the console autocomplete popup when opening another panel. r=devtools-reviewers,jdescottes,nchevobbe

Comment 8

[agoloman]

Backed out for causing bc failures @browser_ext_devtools_panel.js.

• Backout link
• Push with failures
• Failure Log

Comment 9

[Alexandre Poirot [:ochameau]]

Should be fixed now:
https://treeherder.mozilla.org/jobs?repo=try&revision=68ca3d1d9b1304183b97c1333ad6f75ad10c06a0
https://treeherder.mozilla.org/jobs?repo=try&revision=21c4d87e52131b44770611d3255bde78c5bfb305

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e05b8223d353
[devtools] Hold off all redux updates when netmonitor is hidden. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/1112744717ae
[devtools] Also avoid uncessary redux updates when the console is hidden. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/0650029a3ec3
[DevTools] Hide the console autocomplete popup when opening another panel. r=devtools-reviewers,jdescottes,nchevobbe

Comment 11

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/e05b8223d353
https://hg.mozilla.org/mozilla-central/rev/1112744717ae
https://hg.mozilla.org/mozilla-central/rev/0650029a3ec3

Comment 12

[Florin Bilt]

(In reply to Serban Stanca [:SerbanS] from comment #11)

https://hg.mozilla.org/mozilla-central/rev/e05b8223d353
https://hg.mozilla.org/mozilla-central/rev/1112744717ae
https://hg.mozilla.org/mozilla-central/rev/0650029a3ec3

Perfherder has detected a devtools performance change from push 0650029a3ec3ced370663cd51701c62a77383337.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
59%	damp custom.netmonitor.manyrequests.togglepanel	windows11-64-shippable-qr	e10s fission stylo webrender	475.48 -> 193.68
45%	damp custom.netmonitor.manyrequests.togglepanel	macosx1470-64-shippable	e10s fission stylo webrender	813.06 -> 446.28
37%	damp custom.netmonitor.manyrequests.togglepanel	linux1804-64-shippable-qr	e10s fission stylo webrender	1,067.73 -> 668.36
29%	damp custom.netmonitor.bigdatarequests.navigate.DAMP	macosx1470-64-shippable	e10s fission stylo webrender	2,405.91 -> 1,708.03
26%	damp custom.netmonitor.bigdatarequests.requestsFinished.DAMP	macosx1470-64-shippable	e10s fission stylo webrender	2,626.78 -> 1,942.94
...	...	...	...	...
4%	damp browser-toolbox.styleeditor-ready.DAMP	windows11-64-shippable-qr	e10s fission stylo webrender	284.99 -> 274.12

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 43786

For more information on performance sheriffing please see our FAQ.