1353563 - Displaying awesomebar items is janky (especially adjustSiteIconStart)

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Florian Quèze [:florian]]

See this profile: https://perf-html.io/public/d795e2cda214b8156da0d504d52750fd0f03c241/calltree/?jsOnly&range=10.1061_10.6607&thread=0

adjustSiteIconStart is causing 176ms of jank on this profile (done on a slow i3 laptop) when opening the awesomebar panel.

From the look of the profile, it seems we are calling adjustSiteIconStart for each autocomplete item; do we really need to do this more than once?

It also seems that this function is causing a layout flush. Do we really need this?

Comment 1

[Marco Bonardo [:mak]]

(In reply to Florian Quèze [:florian] [:flo] from comment #0)
> From the look of the profile, it seems we are calling adjustSiteIconStart
> for each autocomplete item; do we really need to do this more than once?

probably we need to fix each item when it's added, but we should not need to do all the calculations every time, it would suffice to do them only for the first addition. Also, when we reuse an item we may not need to adjust it if the layout didn't change.

> It also seems that this function is causing a layout flush. Do we really
> need this?+

All of the 176ms are indeed due to layout flushes.
Likely we need at least one flush, we want to align the popup favicons with the urlbar icon, and for that to be reliable we need the layout to have made all the calculations. It's possible the new photon design won't require that if the popup won't take the whole window.
A possible alternative may be to calculate coordinates just once, but then we should be able to observe any kind of change to the toolbar that may require an invalidation. That could be feasible spending some more time on it.

Comment 2

[Florian Quèze [:florian]]

(In reply to Marco Bonardo [::mak] from comment #1)
> (In reply to Florian Quèze [:florian] [:flo] from comment #0)
> > From the look of the profile, it seems we are calling adjustSiteIconStart
> > for each autocomplete item; do we really need to do this more than once?
> 
> probably we need to fix each item when it's added, but we should not need to
> do all the calculations every time, it would suffice to do them only for the
> first addition. Also, when we reuse an item we may not need to adjust it if
> the layout didn't change.
> 
> > It also seems that this function is causing a layout flush. Do we really
> > need this?+
> 
> All of the 176ms are indeed due to layout flushes.
> Likely we need at least one flush, we want to align the popup favicons with
> the urlbar icon, and for that to be reliable we need the layout to have made
> all the calculations.

What would it take to avoid this layout flush? I don't really understand why we can't save the urlbar icon's position before the panel opens and causes changes to DOM nodes, so that no layout work is needed to get its position.

There's another layout flush in this profile, done by ensureElementIsVisible (http://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/toolkit/content/widgets/richlistbox.xml#201) called by the selectedIndex setter at http://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/toolkit/content/widgets/autocomplete.xml#1086
I think this can be optimized too, as when opening the awesomebar panel we'll always select the first item, and if the scrollbar is at the top, we don't need to call getBoundingClientRect to know that the item is obviously visible.

> A possible alternative may be to calculate coordinates just once, but then
> we should be able to observe any kind of change to the toolbar that may
> require an invalidation. That could be feasible spending some more time on
> it.

I think spending some time on making the awesomebar items display as fast as possible is a reasonable investment.

Comment 3

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #2)

> There's another layout flush in this profile, done by ensureElementIsVisible
> (http://searchfox.org/mozilla-central/rev/
> 381a7b8f8a78b481336cfa384cb0a5536e217e4a/toolkit/content/widgets/richlistbox.
> xml#201) called by the selectedIndex setter at
> http://searchfox.org/mozilla-central/rev/
> 381a7b8f8a78b481336cfa384cb0a5536e217e4a/toolkit/content/widgets/
> autocomplete.xml#1086
> I think this can be optimized too, as when opening the awesomebar panel
> we'll always select the first item, and if the scrollbar is at the top, we
> don't need to call getBoundingClientRect to know that the item is obviously
> visible.

Filed bug 1353708 for this.

Comment 4

[Marco Bonardo [:mak]]

(In reply to Florian Quèze [:florian] [:flo] from comment #2)
> What would it take to avoid this layout flush? I don't really understand why
> we can't save the urlbar icon's position before the panel opens and causes
> changes to DOM nodes, so that no layout work is needed to get its position.

I don't know off-hand. I suspect with some investigation we could be able to find a more performant solution indeed.

> There's another layout flush in this profile, done by ensureElementIsVisible

Thanks for the bug, I think I know how we can avoid this cost in autocomplete.

> I think spending some time on making the awesomebar items display as fast as
> possible is a reasonable investment.

Agree.

Comment 5

[Florian Quèze [:florian]]

Could we just use:

let utils = window.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);
utils.getBoundsWithoutFlushing(this._siteIcon);

instead of

this._siteIcon.getBoundingClientRect();

?

Comment 6

[Marco Bonardo [:mak]]

(In reply to Florian Quèze [:florian] [:flo] from comment #5)
> Could we just use:
> 
> let utils =
> window.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.
> nsIDOMWindowUtils);
> utils.getBoundsWithoutFlushing(this._siteIcon);
> 
> instead of
> 
> this._siteIcon.getBoundingClientRect();

it's quite plausible. Someone should try doing that and see what happens.

Comment 7

[Florian Quèze [:florian]]

Here is a profile of displaying the awesomebar panel on an even slower machine (and old netbook): https://perfht.ml/2ovtidb

Comment 8

[Florian Quèze [:florian]]

Attachment: Use getBoundsWithoutFlushing

I think it's worth trying, and I didn't see any regression when trying this locally... but I'm afraid the getComputedStyle call 8 lines later will also cause a layout flush. Also, _handleOverflow does getBoundingClientRect calls too.

Comment 9

[Marco Bonardo [:mak]]

311ms: _adjustSiteIconStart, as previously discussed.
215ms: _handleOverflow is likely causing another flush, it tries to truncate strings
196ms: in search.xml _rebuild... I assume this is bug 1312999
165ms: _adjustAcItem is modifying the DOM iirc
139ms: The openAutoCompletePopup self time may be the fact we unhide the panel, and that applies the binding.

There's also other stuff, this is a quite nice profile to investigate.
The 2 clear paths forward are this bug and the search.xml rebuild.
handleOverflow and adjustACItem are the next possible targets, but I have no ideas off-hand on how to change those.

Comment 10

[Florian Quèze [:florian]]

(In reply to Marco Bonardo [::mak] from comment #9)

> 196ms: in search.xml _rebuild... I assume this is bug 1312999

The profile in comment 8 was the first display of the awesomebarpanel of the browsing session. Bug 1312999 is about caching the one-offs so we build them only once, so it wouldn't help with that profile.

However, here's is a profile of the second opening of the awesomebar on that same browsing session, and the search.xml stuff is still 189ms there: https://perfht.ml/2ovpqZY

> There's also other stuff, this is a quite nice profile to investigate.

Glad you like it. :-)

Comment 11

[Marco Bonardo [:mak]]

(In reply to Florian Quèze [:florian] [:flo] from comment #8)
> but I'm afraid the getComputedStyle call 8 lines later will also
> cause a layout flush.

That should be measurable through the same profiling you did already, right?
Off-hand looks like we always define a margin-inline-start in default themes so we probably don't hit that code path.

It's probably worth to file a bug that runs a query in the awesomebar and measures the number of flushes, as Mike suggested, so we can slowly reduce that number and avoid regressing it.

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 8857962 [details] [diff] [review]
Use getBoundsWithoutFlushing

Review of attachment 8857962 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with at least a bug filed to add a test measuring flushes for a simple location bar search. It should be trivial using promiseAutocompleteResultPopup

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f44ec6523ac2
Avoid calling getBoundingClientRect from adjustSiteIconStart, r=mak.

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/f44ec6523ac2