1413829 - RequestListContent scroll to bottom should avoid sync reflow

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement, P1)

Description

[Ricky Chien [:rickychien] (inactive)]

One of the major perf issues of netmonitor is scroll to bottom. Every componentDidUpdate will access node.scrollHeight [1] in order to set the scroll position to the bottom of table. Each time we query node.scrollHeight will cause a sync reflow which is really expensive.

In order prevent sync reflow, we can simply set maximum of int32 2147483647 to bypass accessing element height.

[1] https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/RequestListContent.js#104

Comment 1

[Ricky Chien [:rickychien] (inactive)]

Attachment: Bug 1413829 - RequestListContent scroll to bottom should avoid sync reflow

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

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8924470 [details]
Bug 1413829 - RequestListContent scroll to bottom should avoid sync reflow

https://reviewboard.mozilla.org/r/195750/#review200874

::: devtools/client/netmonitor/src/components/RequestListContent.js:30
(Diff revision 1)
>  
>  const { div } = DOM;
>  
>  // tooltip show/hide delay in ms
>  const REQUESTS_TOOLTIP_TOGGLE_DELAY = 500;
> +const MAX_SCROLL_HEIGHT = 2147483647;

Does using Number.MAX_SAFE_INTEGER provides the same benefit ? It has the advantage of not looking like a "magic" number :)

Comment 3

[Ricky Chien [:rickychien] (inactive)]

No, Number.MAX_SAFE_INTEGER is 2^53 - 1 not equal to the maximum value is int32 2^31 - 1 (2147483647). 

Gecko's scrollTop is int32_t https://searchfox.org/mozilla-central/source/dom/base/Element.cpp#903

Comment 4

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

Comment on attachment 8924470 [details]
Bug 1413829 - RequestListContent scroll to bottom should avoid sync reflow

https://reviewboard.mozilla.org/r/195750/#review200950

Nice catch Fred, thanks!
R+
Honza

Comment 5

[Ricky Chien [:rickychien] (inactive)]

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=ebae20d13648ddf663a420a4ed93effb5d9207ec&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&framework=1&selectedTimeRange=172800

damp simple.netmonitor.close.DAMP 	55.99 ± 8.35%	>	55.02 ± 8.10%	-1.73%

damp simple.netmonitor.open.DAMP	529.05 ± 2.24%	>	527.12 ± 3.56%	-0.36%
	
damp simple.netmonitor.reload.DAMP	80.55 ± 2.36%	>	78.54 ± 2.33%	-2.49%	


DAMP result comparison

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8570b22fc8f3
RequestListContent scroll to bottom should avoid sync reflow r=Honza

Comment 7

[[e-mail]]

Comment on attachment 8924470 [details]
Bug 1413829 - RequestListContent scroll to bottom should avoid sync reflow

https://reviewboard.mozilla.org/r/195750/#review200994

::: devtools/client/netmonitor/src/components/RequestListContent.js:30
(Diff revision 1)
>  
>  const { div } = DOM;
>  
>  // tooltip show/hide delay in ms
>  const REQUESTS_TOOLTIP_TOGGLE_DELAY = 500;
> +const MAX_SCROLL_HEIGHT = 2147483647;

we can add some comments for this magic number, ex:

Gecko's scrollTop is int32_t , the maximum value is int32 2^31 - 1  = 2147483647.

Comment 8

[Andreea Pavel [:apavel]]

Backed out for backout bug https://bugzilla.mozilla.org/show_bug.cgi?id=1413941

Link to backout: https://hg.mozilla.org/integration/autoland/rev/2ec0c37e6fed0d7ed02dcb8eba80eae97143ee83

Comment 9

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8924470 [details]
Bug 1413829 - RequestListContent scroll to bottom should avoid sync reflow

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6af2bb14bd4c
RequestListContent scroll to bottom should avoid sync reflow r=Honza

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6af2bb14bd4c