1463555 - Queue adding the tabs to the inspector sidebar

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P3)

Description

[Gabriel Luong [:gl] (ΦωΦ)]

We can significantly improve the performance of adding the sidebar panels to the inspector sidebar by queuing up all the tabs to add, and setting the new state once to reduce the number of renders that occur for each time we call setState from adding a new tab.

Comment 1

[Gabriel Luong [:gl] (ΦωΦ)]

baseline https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8fd0effb2f8c2cbf01c9796ae850d7ec17237f5
talos https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=de0299c173c2cbb451a50c0dd4a43b59f7c5eb51

Comment 2

[Gabriel Luong [:gl] (ΦωΦ)]

baseline https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbf6c1d8fb2beb8532011c18b8c345f18f30b5c7
talos https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=abb93b152e860a9b346fec288f5777f4be79bb16

Comment 3

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 1463555 - Queue adding the tabs to the inspector sidebar.

Review commit: https://reviewboard.mozilla.org/r/246002/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246002/

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

try https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e8c8b457fcb72422994157424aeb8dacaca05c4

Comment 5

[Gabriel Luong [:gl] (ΦωΦ)]

try https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b7c417644d961ba29d448d69225fc2c612f1c4f

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8979831 [details]
Bug 1463555 - Queue adding the tabs to the inspector sidebar.

https://reviewboard.mozilla.org/r/246002/#review252110

Thansk for working on this Gabriel.

Looks good to me, but please see my inline comments.

Also, I am curious how much this improves the performance.
Do you have some data from Talos prooving that
this makes the UI faster?

Honza

::: devtools/client/shared/components/tabs/TabBar.js:145
(Diff revision 1)
> +
> +    let tabs = this.state.tabs.slice();
> +    let activeId;
> +    let activeTab;
> +
> +    for (let { id, index, panel, selected, title, url } of this.queuedTabs) {

Content of this loop is duplicating `addTab` method. Can you reuse that method in the loop? Or use another way to dup the code?

::: devtools/client/shared/components/tabs/TabBar.js:172
(Diff revision 1)
> +    });
> +
> +    this.queuedTabs = [];
> +  }
> +
> +  queueTab(id, title, selected = false, panel, url, index = -1) {

Please add a comment explaining why tab-queuing APIs have been introduced.

Comment 7

[Gabriel Luong [:gl] (ΦωΦ)]

This is the talos result https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=cbf6c1d8fb2beb8532011c18b8c345f18f30b5c7&newProject=try&newRevision=abb93b152e860a9b346fec288f5777f4be79bb16&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&framework=1. We're looking at 4-5%% improvement on open.

Comment 8

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8979831 [details]
Bug 1463555 - Queue adding the tabs to the inspector sidebar.

https://reviewboard.mozilla.org/r/246002/#review252110

> Content of this loop is duplicating `addTab` method. Can you reuse that method in the loop? Or use another way to dup the code?

I don't think we can do anything about reusing the code in a helper or using the addTab method here. The point of this was to limit the setState call to just once with all the queued tabs.

Comment 9

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8979831 [details]
Bug 1463555 - Queue adding the tabs to the inspector sidebar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246002/diff/1-2/

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8979831 [details]
Bug 1463555 - Queue adding the tabs to the inspector sidebar.

https://reviewboard.mozilla.org/r/246002/#review252478

Thanks for the update!

R+ assuming try is green

Honza

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7690bdfe60a0
Queue adding the tabs to the inspector sidebar. r=Honza

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/30c7c7093432
Followup: Remove unexpected 'debugger' statement. r=me CLOSED TREE

Comment 13

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/7690bdfe60a0
https://hg.mozilla.org/mozilla-central/rev/30c7c7093432

Comment 14

[Alexandre Poirot [:ochameau]]

Thanks Gabriel, this patch recovered a significant part of the 3panes regression.
Two mores like this one and we get back to the baseline perf :)

http://firefox-dev.tools/performance-dashboard/perfherder/?test=complicated.inspector.open&days=14&filterstddev=true

Comment 15

[Ionuț Goldan [:igoldan]]

Noticed perf improvements:

== Change summary for alert #13621 (as of Thu, 24 May 2018 17:56:10 GMT) ==

Improvements:

  4%  damp windows10-64 pgo e10s stylo     85.00 -> 81.20

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