Closed Bug 1413829 Opened 2 years ago Closed 2 years ago

RequestListContent scroll to bottom should avoid sync reflow

Categories

(DevTools :: Netmonitor, enhancement, P1)

enhancement

Tracking

(firefox58 fixed)

RESOLVED FIXED
Firefox 58
Tracking Status
firefox58 --- fixed

People

(Reporter: rickychien, Assigned: rickychien)

References

Details

Attachments

(1 file)

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 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 :)
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 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
Attachment #8924470 - Flags: review?(odvarko) → review+
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
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8570b22fc8f3
RequestListContent scroll to bottom should avoid sync reflow r=Honza
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.
Flags: needinfo?(rchien)
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6af2bb14bd4c
RequestListContent scroll to bottom should avoid sync reflow r=Honza
https://hg.mozilla.org/mozilla-central/rev/6af2bb14bd4c
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Duplicate of this bug: 1404117
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.