1445811 - Consider avoiding synchronously focusing elements as it triggers sync style flush

Status: NEW

(Firefox :: General, enhancement, P2)

Description

[Xidorn Quan [:xidorn] UTC+10]

Changing focus can trigger synchronous style flush because we need to check whether something is focusable via frame.

For example see this profile: https://perfht.ml/2FHyW2a the first restyle (cyan bar) on the main thread is a sync style flush triggered by https://searchfox.org/mozilla-central/rev/8976abf9cab8eb4661665cc86bd355cd08238011/browser/base/content/tabbrowser.js#1215

In this case, if I change the line to
> requestAnimationFrame(() => {
>   fm.setFocus(newBrowser, focusFlags);
> });
that restyle and the second one would be merged together.

In this specific case, though, it may not be that worthwhile. According to my measurement, doing so may save us ~1ms in tabpaint, which probably isn't that significant. But given that we generally want to avoid sync flush, this is potentially something we can improve in the frontend code.

Comment 1

[Florian Quèze [:florian]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #0)
> Changing focus can trigger synchronous style flush because we need to check
> whether something is focusable via frame.

Would it be possible to avoid that style flush in the common cases? (The one that I saw the most often is focusing the urlbar)

> For example see this profile: https://perfht.ml/2FHyW2a the first restyle
> (cyan bar) on the main thread is a sync style flush triggered by
> https://searchfox.org/mozilla-central/rev/
> 8976abf9cab8eb4661665cc86bd355cd08238011/browser/base/content/tabbrowser.
> js#1215

That profile doesn't contain JS stack frames :-(.

> In this case, if I change the line to
> > requestAnimationFrame(() => {
> >   fm.setFocus(newBrowser, focusFlags);
> > });
> that restyle and the second one would be merged together.

Are we sure that using requestAnimationFrame here will never cause the focus change to one frame later (and cause flicker)?

> In this specific case, though, it may not be that worthwhile. According to
> my measurement, doing so may save us ~1ms in tabpaint, which probably isn't
> that significant.

On which hardware is this 1ms measurement taken?

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Florian Quèze [:florian] from comment #1)
> (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #0)
> > Changing focus can trigger synchronous style flush because we need to check
> > whether something is focusable via frame.
> 
> Would it be possible to avoid that style flush in the common cases? (The one
> that I saw the most often is focusing the urlbar)

I don't have idea. Given that we need the frame and its style to check whether something is focusable, it's probably not? Maybe we can have some backdoor to skip it if we know something is definitely focusable, but I'm not sure whether it's worth it.

> 
> > For example see this profile: https://perfht.ml/2FHyW2a the first restyle
> > (cyan bar) on the main thread is a sync style flush triggered by
> > https://searchfox.org/mozilla-central/rev/
> > 8976abf9cab8eb4661665cc86bd355cd08238011/browser/base/content/tabbrowser.
> > js#1215
> 
> That profile doesn't contain JS stack frames :-(.

Sorry about that. That profile is probably caught with JS switched off. I investigated this in another profile with JS stack, but I didn't upload it.

> > In this case, if I change the line to
> > > requestAnimationFrame(() => {
> > >   fm.setFocus(newBrowser, focusFlags);
> > > });
> > that restyle and the second one would be merged together.
> 
> Are we sure that using requestAnimationFrame here will never cause the focus
> change to one frame later (and cause flicker)?

That's not impossible, and thus I didn't submit this patch for review, just for measurement.

> > In this specific case, though, it may not be that worthwhile. According to
> > my measurement, doing so may save us ~1ms in tabpaint, which probably isn't
> > that significant.
> 
> On which hardware is this 1ms measurement taken?

On my Windows machine, which is Lenovo P50, with GeckoProfiler enabled with 1ms interval. (This profile from the link here has 0.1ms interval, fwiw).

Comment 3

[:Gijs (out for now; he/him)]

p2 to investigate, because I think we hit this in common user interactions (at least for tabswitches, probably other stuff), though it may be quite hard to fix without inadvertently introducing race conditions.

Comment 4

[:Gijs (out for now; he/him)]

5 years later, and we do actually try to do this in a number of places, but AIUI it's generally difficult for frontend to predict what focus calls are likely to trigger a style flush (ie when the flush is 'free' because we know there's nothing to update, and when it's not).

We could add a flag to the focus manager to prevent it from flushing, e.g. for the URL bar focusing case or the browser focusing case that we do on tabswitch, because frontend knows that the element is present and can be focused... but I don't know how feasible this really is, and as a general problem I wonder about how this affects web content, and if there's not a way that we can avoid the flushing on the backend instead of forcing all the consumers to adapt or pass magical chromeonly flags.

Emilio, do you have any thoughts/advice here?

Comment 5

[Emilio Cobos Álvarez (:emilio)]

It's hard to not flush for content because we don't really want to expose the concept of flushing there...

Rather than passing a magic flag via the focus manager it could be more convenient to just add it in the FocusOptions dictionary, but other than that I don't have any other specific thoughts here.

Comment 6

[:Gijs (out for now; he/him)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #5)

It's hard to not flush for content because we don't really want to expose the concept of flushing there...

Sorry for under-specifying my question. Yeah, I didn't mean to suggest exposing the concept of flushing to web content.

Rather, I wonder if we could:

1. use an extant frame for a node if one exists and use that to determine if it's focusable and to then focus it
2. register a callback for when the next restyle/layout flush happens and doublecheck the node is still focusable at that point
3. revert focus to where it was if the focusability of the node has changed inbetween these two points.

Perhaps even just doing (1) would be sufficient here?

As part of looking at this, I poked at chromium and our source code for a bit. It seems they also flush layout, except they avoid it in a few cases (old link from paul irish's overview).

Then I looked at our source code, and I was confused. In particular, it looks like in chrome documents we never skip focusing nodes:

https://searchfox.org/mozilla-central/rev/dd2fe65d792943365a03fa996cdf9766829575b6/dom/base/Element.cpp#458,465-471 + https://searchfox.org/mozilla-central/source/dom/base/nsFocusManager.cpp#5328

This seems bad, and per bug 1366517 is a leftover from XBL-ness. Maybe that means we can get rid of it now?

It also seems like there may be other cases where we might avoid flushing (though they look unlikely, off-hand, to significantly impact the issues outlined here).

Comment 7

[Emilio Cobos Álvarez (:emilio)]

(In reply to :Gijs (he/him) from comment #6)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #5)

It's hard to not flush for content because we don't really want to expose the concept of flushing there...

Sorry for under-specifying my question. Yeah, I didn't mean to suggest exposing the concept of flushing to web content.

Rather, I wonder if we could:

1. use an extant frame for a node if one exists and use that to determine if it's focusable and to then focus it

That by definition would expose the flushing to the web (because there might be a pending display: none change for example).

2. register a callback for when the next restyle/layout flush happens and doublecheck the node is still focusable at that point
3. revert focus to where it was if the focusability of the node has changed inbetween these two points.

Focus is really weird because you can do that kind of circular :focus { display: none } shenanigans. Also focusing itself changes style...

https://searchfox.org/mozilla-central/rev/dd2fe65d792943365a03fa996cdf9766829575b6/dom/base/Element.cpp#458,465-471 + https://searchfox.org/mozilla-central/source/dom/base/nsFocusManager.cpp#5328

This seems bad, and per bug 1366517 is a leftover from XBL-ness. Maybe that means we can get rid of it now?

Yeah, that special-case seems worth trying to remove nowadays.

It also seems like there may be other cases where we might avoid flushing (though they look unlikely, off-hand, to significantly impact the issues outlined here).

Agreed. In general we can avoid flushing if there's no pending restyle in that tree (we could do something similar to what we do for getComputedStyle). Not quite sure if the complexity is worth it.