Displaying awesomebar items is janky (especially adjustSiteIconStart)

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Location Bar
P1
normal
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: florian, Assigned: florian)

Tracking

(Blocks: 2 bugs)

unspecified
Firefox 55
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [photon-performance] [fxsearch])

Attachments

(1 attachment)

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

5 months ago
(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.
(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.
(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.
See Also: → bug 1353708

Comment 4

5 months ago
(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.
Blocks: 1353725
Whiteboard: [photon] → [photon][fxsearch]
Could we just use:

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

instead of

this._siteIcon.getBoundingClientRect();

?

Comment 6

4 months ago
(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.

Updated

4 months ago
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [photon][fxsearch] → [photon-performance] [fxsearch]
Here is a profile of displaying the awesomebar panel on an even slower machine (and old netbook): https://perfht.ml/2ovtidb
Created attachment 8857962 [details] [diff] [review]
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.
Attachment #8857962 - Flags: review?(mak77)

Comment 9

4 months ago
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.
(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. :-)
(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 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
Attachment #8857962 - Flags: review?(mak77) → review+
Depends on: 1356271

Comment 13

4 months ago
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f44ec6523ac2
Avoid calling getBoundingClientRect from adjustSiteIconStart, r=mak.
Duplicate of this bug: 1356762
https://hg.mozilla.org/mozilla-central/rev/f44ec6523ac2
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Duplicate of this bug: 1356855

Updated

4 months ago
Assignee: nobody → florian
Priority: P2 → P1

Updated

4 months ago
Iteration: --- → 55.3 - Apr 17
No longer blocks: 1348289
Blocks: 1348289
You need to log in before you can comment on or make changes to this bug.