1502161 - Consider marking some XUL panels/popups as reflow roots

Status: RESOLVED INVALID

(Core :: Layout, enhancement, P3)

Description

[Daniel Holbert [:dholbert]]

mconley brought up a particular "optimizations wanted" use case for CSS containment in frontend code: the XUL <panel> element, and maybe other popups (at least, for the near future while we're still using XUL in Firefox code).

References:
 https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/panel
 https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/PopupGuide

Some facts:
 1) Some examples of panels are the hamburger menu and the awesomebar menu.
 2) the layout within a panel doesn't seem (?) to have the ability to influence the layout outside of the panel -- this makes it potentially a good candidate for containment or containment-like features.
 3) panels are part of the same frame tree as the rest of browser UI -- they're all contained in a single nsPopupSetFrame which is rooted 
 4) panels are not currently reflow roots -- if we flush layout from inside of a panel, it flushes layout in the whole UI.

So anyway: mconley's brought up panels/popups as a use case for CSS Containment, so that we could hypothetically add + measure content inside of one panel without those measurements causing layout flushes in other panels [which maybe have also undergone changes since the last refresh driver tick].  Depending on how common this scenario is, this could be a nice responsiveness win.

However: rather than implementing CSS Containment in these XUL classes, I wonder if it's simpler to just directly make these frames (or certain subsets of them) be reflow roots automatically? Either piggybacking off of bug 1159042, or perhaps just directly adding to some frames where it'd help...

As an experiment, I tried adding NS_FRAME_REFLOW_ROOT to nsPopupSetFrame, and it didn't break any tests, but it also didn't seem to help in any of our talos tests. (Though this is also one level of granularity higher than the individual popups.)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: just-for-fun patch to make nsPopupSetFrame a reflow root

(In reply to Daniel Holbert [:dholbert] from comment #0)
> As an experiment, I tried adding NS_FRAME_REFLOW_ROOT to nsPopupSetFrame,
> and it didn't break any tests, but it also didn't seem to help in any of our
> talos tests. (Though this is also one level of granularity higher than the
> individual popups.)

Here's the patch that I used to do this, FWIW.

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)
>  3) panels are part of the same frame tree as the rest of browser UI --
> they're all contained in a single nsPopupSetFrame which is rooted 

Sorry, I didn't finish this sentence -- I meant to say "...nsPopupSetFrame, whose parent is some close-to-the-root frame in the same tree as the rest of the browser." (I forget the exact parent frame.)

Comment 3

[Mike Conley (:mconley) (:⚙️)]

I did a quick and dirty performance test here, where I used performance.mark and performance.measure to measure the time between these two points[1] of browser_urlbar_keyed_search.js, which is a reflow test that intentionally attempts to hit the worse-case scenarios for sync layout flushes.

I did 4 runs, with and without the patch. Pretty wide standard deviation, so please take this with a grain of salt.

Without the patch:

2987.582172938713
3070.518976855348
3333.9413356011237
2898.3655784696825

With the patch:

2775.723701091618
2829.7078510003444
3083.414001158333
2147.3876674266694


So there might be something here.


[1]: https://searchfox.org/mozilla-central/rev/e0c879c86b95bdc752b1dbff6088169735674e4a/browser/base/content/test/performance/browser_urlbar_keyed_search.js#173-177

Comment 4

[Mike Conley (:mconley) (:⚙️)]

(Not super-scientific, but it smells like there's something here.)

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Daniel: I pinged you on bug 1497414 comment 3, but I should have commented here. (Feel free to ignore/discard the former.)

For easy reference, here's what I said, which may be relevant to what you're trying here:
- Bug 1499961 fixes an issue when moving a reflow root, so if you see frames not being moved as expected, try that patch.
- Bug 1159042 tracks overflows, and also ensures a better ordering of reflows (when a reflow root and a descendant reflow root both need reflow). If you need this, try patches 1-4.
'hope this helps.

Comment 6

[:Gijs (he/him)]

P2, but would probably be P1 if this was something we (the frontend perf team) would work on ourselves (in terms of our expertise) and assuming the complexity wasn't crazy. The numbers in comment #3 seem pretty compelling. Daniel, what would it take to push this over the line?

Comment 7

[Daniel Holbert [:dholbert]]

We probably need Bug 1159042 before this will be valid (specifically the overflow handling stuff). With that, the attached patch is likely OK to land (and that's what got us the numbers in comment 3).

(Per end of comment 0, the attached patch isn't as thorough as what I'd initially filed this bug about, which was making popups *themselves* into reflow roots.  That part requires a bit more research to assess its complexity/soundness.)

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Hey dholbert, bug 1159042 is RESOLVED FIXED... do we still think we should go ahead and land this?

Comment 9

[Daniel Holbert [:dholbert]]

Probably! I'll take a look soon. Still want to do a bit of research to convince myself that it's correct, since I haven't really ever looked at XUL popup code before.

Comment 10

[Daniel Holbert [:dholbert]]

Hey, nsPopupSetFrame (the class touched in the patch here) no longer exists, as part of XUL removal. Hooray!

I think that means there's nothing to do here at this point, making this invalid these days.