Open Bug 1624683 Opened 8 months ago Updated 2 months ago

[meta] put browser.xhtml's panels, popupnotification and context menus into html:template tags and only insert them into the DOM for real when needed, to improve startup and window opening performance

Categories

(Toolkit :: XUL Widgets, defect, P3)

defect

Tracking

()

People

(Reporter: Gijs, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: meta, Whiteboard: [fxperf:p2])

Off-hand this seems to lead to a 40% reduction in time spent in custom element code on startup (of a total of about 18-20ms on my high-end mbp).

The actual lazification is going to be more straightforward for some panels than for others.

Whiteboard: [fxperf] → [fxperf:p2]

Talos shows this gains us on the order of 13% on twinopen on macOS. I don't have linux/windows numbers because I think my hacky patches to check the impact of this broke something, but I wrote them on macOS where they didn't, so unsure what exactly - https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=53f0121950670da44af74b7526ad02e3b58ba921&newProject=try&newRevision=c9ca30341ad4db76ad148b8ac1f5ac0688c8c064&framework=1 .

ts_paint is very noisy so no conclusions there, but I would expect a similar win in ms (smaller in percentage terms, of course). Possibly the same kind of impact on sessionrestore many windows.

The tricky part is doing this without breaking any functionality. The wip patch I pushed ( https://hg.mozilla.org/try/rev/49fe74f15ca36743f8815c1dec683e1352793971 ) doesn't even try to be subtle about things, it was just designed to test what happens if we don't incur any of the relevant cost.

Some panels/menus/tooltips should be relatively straightforward - we could just wrap them and make the few places that access them unwrap them.

Other panels are harder, because there's existing code that assumes the panel/popup is always available and it (and its contents) can be modified (the page actions menu, site identity popup, and ctrl tabs pane, hamburger panel, etc.). While we could just delazify things as soon as they're accessed, that would likely not win us much (or perhaps lose us something, as we incur more runtime costs than if we never modified the DOM, and potentially additional ones from additional localization passes).

So we may have to do the hard work of lazifying access to some of these panels. That's annoying, but on the other hand it brings other benefits - for instance, it seems we currently update various UI items in panels when the user switches tabs or navigates, and if we could stop doing that work unless the panel is delazified and open, navigation/tabswitching costs also improve.

Keywords: meta
Summary: Lazify browser.xhtml's panels and popupnotification elements into html:template → [meta] Lazify browser.xhtml's panels and popupnotification elements into html:template
Depends on: 1634026
Depends on: 1634030
Depends on: 1634031
Depends on: 1634032
Depends on: 1634033

Tweaking the summary to make it more obvious what this is about.

TL;DR: when we had XBL, the panels had no initialization cost because they were hidden=true so they didn't get a layout frame, so XBL didn't construct their bindings. Now, that trick doesn't work anymore, and we run Custom Element constructors and connectedCallbacks for all of the panels and their contents. This is a significant part of the cost of opening a browser window. We should stop paying it by only creating these nodes when we need them. This has no visual impact for users because we do not show all these panels when the window loads - only later and/or in response to user interaction.

Summary: [meta] Lazify browser.xhtml's panels and popupnotification elements into html:template → [meta] put browser.xhtml's panels, popupnotification and context menus into html:template tags and only insert them into the DOM for real when needed, to improve startup and window opening performance
Depends on: 1634042
Depends on: 1634046
Depends on: 1634048
Depends on: 1634051

Thanks for breaking this down Gijs! I know you are alraedy aware of this, but I wanted to call something out explicitly for posterity. While this is true:

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

TL;DR: when we had XBL, the panels had no initialization cost because they were hidden=true so they didn't get a layout frame, so XBL didn't construct their bindings.

We also would have had to eagerly run XBL constructors in any case where panels were being touched from JS while hidden, for instance in all these cases:

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

Other panels are harder, because there's existing code that assumes the panel/popup is always available and it (and its contents) can be modified (the page actions menu, site identity popup, and ctrl tabs pane, hamburger panel, etc.). While we could just delazify things as soon as they're accessed, that would likely not win us much (or perhaps lose us something, as we incur more runtime costs than if we never modified the DOM, and potentially additional ones from additional localization passes)

So that means after doing this work we should be in a better place than XBL days because the following optimizations you mention should have applied (mostly) equally at that time:

So we may have to do the hard work of lazifying access to some of these panels. That's annoying, but on the other hand it brings other benefits - for instance, it seems we currently update various UI items in panels when the user switches tabs or navigates, and if we could stop doing that work unless the panel is delazified and open, navigation/tabswitching costs also improve.

Depends on: 1558635

Is this also related to the idea of having each MozElement register itself to document.l10n when connected, and unregister when diconnected?

(In reply to Zibi Braniecki [:zbraniecki][:gandalf] from comment #4)

Is this also related to the idea of having each MozElement register itself to document.l10n when connected, and unregister when diconnected?

No, I don't think it is.

Depends on: 1636559

Because this bug's Severity has not been changed from the default since it was filed, and it's Priority is P3 (Backlog,) indicating it has been triaged, the bug's Severity is being updated to S3 (normal.)

Severity: normal → S3
Depends on: 1644778
Depends on: 1646780
Depends on: 1665237
You need to log in before you can comment on or make changes to this bug.