Closed Bug 1520350 Opened 6 years ago Closed 6 years ago

Lazily load about:preferences markups from hidden panes

Categories

(Firefox :: Settings UI, enhancement, P1)

Product:
Firefox
Component:
Settings UI
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 67
Tracking Flags:
Tracking Status
firefox67 --- fixed

People

(Reporter: timdream, Assigned: timdream)

References

(Depends on 2 open bugs)

Details

Attachments

(1 file)

Bug 1520350 - Lazily load about:preferences markups from hidden panes
47 bytes, text/x-phabricator-request
Details | Review

In bug 1518932, <menulist> CEs will get constructed when the element is bind to the tree. This is a huge talos regession for about:preferences because the <menulist> XBL binding used to bind only when the elements are visible.

We would have to lazily append the DOM from the hidden pane. The intended way to do so would be to wrap it inside a <html:template> so that the markup gets parsed into a document fragment, and the CEs won't be constructed nor connected until the DOM is actually being moved from the document fragment to the document.

Unfortunately, our XUL parser won't process <html:template>. The markup inside of it won't get to the document fragment and being considered as normal child nodes. (see bug 1011831 for the XHTML parser.) That won't help us in terms of deferring loading CEs.

I tried to be creative and use comment node and other hacks. Turned out our XUL DOM is pretty robust — the inserted markup won't go through XSS filter (I ended up receiving "Flattening unsafe node (descendants are preserved)." warnings)

I would really like to avoid firing up DOMParser for this (that fixes our start-up speed but that by itself is slow). However, I don't see any available choices for now :-/

So I think I broke spotlight and/or some other features, but they should not affect talos test.

With that, the result looks promising so I am going to keep working on this approach.

That said, I don't think this is safe to land before soft code freeze. So everything would have to wait.

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #3)

So I think I broke spotlight and/or some other features, but they should not affect talos test.

With that, the result looks promising so I am going to keep working on this approach.

Wow, those about:preferences results look great! 20-30% on all platforms would be a huge win. Is that comparing with the menulist CE conversion as well, or just baseline?

I've removed a mutation observer added in bug 1407568 comment 35. It was added to workaround a timing issue. It turned out it simply wait for that idle callback because form autofill is waiting for the wrong pane to init! Having it init alongside with the privacy pane fixes the issue.

(In reply to Brian Grinstead [:bgrins] from comment #4)

Wow, those about:preferences results look great! 20-30% on all platforms would be a huge win. Is that comparing with the menulist CE conversion as well, or just baseline?

That's just comparing the baseline.

The lazily loaded panes didn't have their preferences updated (checkboxes are not checked when the pref is true, for example). I don't think this affects talos number but just to be on the safe side:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=43067715c57f&newProject=try&newRevision=6f43885c071a&framework=1

https://treeherder.mozilla.org/#/jobs?repo=try&revision=75f033b763c641e457fea0816e9f674e744635aa

Because custom elements will be constructed when DOM is constructed, construct the DOM in the hidden panels will be expensive as we move more and more widgets to custom elements from XBL. This patch attempts to counter that by moving all the pane markups into comment nodes, and use MozXULElement.parseXULToFragment() to insert it when it is being asked. They will be loaded lazily from an requestIdleCallback() in findInPage.js.
Depends on: 1515799

Triaging to P1 because we're working on this.

Note that I'm pretty nervous about this. If we have to do this for other documents where we convert a substantial amount of XBL bindings to custom elements (cough browser.xul cough) then that adds substantial additional complexity that I'd prefer to be able to do without.

Have we considered instead changing when we fire connectedcallbacks or similar for elements that are descendants of <panel>, <deck> and similar elements where the contents aren't currently visible and this is relatively cheap to determine -- or any other types of platform fixes that would scale better and not require reimplementing for all the other cases where we hit this type of problem?

This is also important because while we have tests for about:preferences, and to a less-well-understood degree for browser.xul (ts_paint and friends have "issues"), that's not really the case for a lot of the rest of the "wider use of XUL" in the codebase, and slowly regressing perf there runs counter to our long-term goals, I think...

Priority: -- → P1

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

Triaging to P1 because we're working on this.

Note that I'm pretty nervous about this. If we have to do this for other documents where we convert a substantial amount of XBL bindings to custom elements (cough browser.xul cough) then that adds substantial additional complexity that I'd prefer to be able to do without.

At least with the initial versions of this patch it wasn't adding substantial complexity. I guess we'll have to see what it takes to get it green. But also note that doing this appears to cause a 20-30% page load win for prefs against the baseline version (even without and additional Custom Elements added) - so this seems worth doing regardless of XBL. So I think highlights that we are paying a perf penalty for the current pattern of "dump all of the markup into the document and then eagerly initialize much of it from JS" (which we do in preferences and browser.xul), even if lazy XBL construction allows us to more or less get away with it.

Have we considered instead changing when we fire connectedcallbacks or similar for elements that are descendants of <panel>, <deck> and similar elements where the contents aren't currently visible and this is relatively cheap to determine -- or any other types of platform fixes that would scale better and not require reimplementing for all the other cases where we hit this type of problem?

I've asked about getting a JS callback for when an element gets a layout frame, so we could manually implement the XBL behavior in JS, but Emilio had a convincing argument against it. I'm sorry, but I can't find the log from that discussion. Emilio, could you elaborate on the reason why we shouldn't be doing this?

Perhaps there's a way we could handle specific hidden parents (like stuff inside of a hidden panel), where we set up a mutation observer for the [hidden] attribute being removed to more lazily initialize the content, although we'll have to tackle at least two things:

  1. Checking el.closest("panel[hidden]") will create a JS reference to the panel element in the DOM, which will eagerly construct the XBL binding and somewhat defeat the purpose. So we'd probably want a C++ API that allowed us to register a callback that fires when a hidden parent's [hidden] attribute becomes removed (which isn't the only way that the parent could become visible, but may be 'good enough').
  2. XBL has special behavior that causes it to become constructed when a JS reference is made to the element, even if it's hidden. We don't have such a thing with Custom Elements and I'm assuming we also don't want to add it in the platform. We have code that expects this to happen, so if we did something like (1), there would be calling code that breaks because you have a handle on an "uninitialized" Custom Element. This may be solvable in JS using proxies or something, but that's probably slow as well.
Flags: needinfo?(emilio)

(In reply to Brian Grinstead [:bgrins] from comment #11)

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

Triaging to P1 because we're working on this.

Note that I'm pretty nervous about this. If we have to do this for other documents where we convert a substantial amount of XBL bindings to custom elements (cough browser.xul cough) then that adds substantial additional complexity that I'd prefer to be able to do without.

At least with the initial versions of this patch it wasn't adding substantial complexity. I guess we'll have to see what it takes to get it green. But also note that doing this appears to cause a 20-30% page load win for prefs against the baseline version (even without and additional Custom Elements added) - so this seems worth doing regardless of XBL.

Well, "additional" is IMHO the key word here. I don't know how to quickly get a count, but we already use Custom Elements here, e.g. for <radiogroup>, <textbox> and <richlistbox> . That's probably at least part of what the 20-30% win comes from...

In terms of complexity, perhaps that was the wrong word to use. I'm worried about having to add in this hackery everywhere (see the other deps of bug 1515799). There's also the fact that the CDATA block means nothing checks whether the contents are valid XML/XUL, which will make development more painful and will hide bugs. No XML parsing errors on pageload, but runtime errors when trying to open/"unhide" the relevant content...

So I think highlights that we are paying a perf penalty for the current pattern of "dump all of the markup into the document and then eagerly initialize much of it from JS" (which we do in preferences and browser.xul), even if lazy XBL construction allows us to more or less get away with it.

XBL, in respect to this particular problem, being connected to frames is effectively a rendering engine level optimization to avoid initializing "custom element" style per-element JS. There are clearly problems with the approach it takes to do that, but there's value in it insomuch as it happens automatically, has clear semantics, and requires little or no additional effort for the consumer/developer (ie JS/frontend).

Have we considered instead changing when we fire connectedcallbacks or similar for elements that are descendants of <panel>, <deck> and similar elements where the contents aren't currently visible and this is relatively cheap to determine -- or any other types of platform fixes that would scale better and not require reimplementing for all the other cases where we hit this type of problem?

I've asked about getting a JS callback for when an element gets a layout frame, so we could manually implement the XBL behavior in JS, but Emilio had a convincing argument against it. I'm sorry, but I can't find the log from that discussion. Emilio, could you elaborate on the reason why we shouldn't be doing this?

Yeah, so, I would specifically not want to do the same thing XBL does. It's really annoying to keep track of when things get frames and then lose frames, that's part of XBL's bugginess. IMO this is because it's not explicit/restricted to the DOM, but also influenced by CSS and other factors.

I'm specifically asking, can we come up with any abstraction at the DOM (rather than layout) level to take care of this automagically? We currently attach things based on BindToTree or similar, I think, with an option to delay until DOMContentLoaded? I'm effectively asking if we can avoid this for some container elements, and define a "better" point to do the attaching for such container elements, based on an attribute or "something else".

Perhaps there's a way we could handle specific hidden parents (like stuff inside of a hidden panel), where we set up a mutation observer for the [hidden] attribute being removed to more lazily initialize the content, although we'll have to tackle at least two things:

FWIW, I've always thought the hidden=true hack for panels was awful. It requires the developer knowing that it exists, it should basically be the default for all <panel>s anyway (but somehow isn't) and it required fidgeting about with the attributes and forcing layout or XBL binding attachment to happen if/when you actually wanted to use the panel. Ideally, we would come up with a solution without those downsides.

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

(In reply to Brian Grinstead [:bgrins] from comment #11)

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

Triaging to P1 because we're working on this.

Note that I'm pretty nervous about this. If we have to do this for other documents where we convert a substantial amount of XBL bindings to custom elements (cough browser.xul cough) then that adds substantial additional complexity that I'd prefer to be able to do without.

At least with the initial versions of this patch it wasn't adding substantial complexity. I guess we'll have to see what it takes to get it green. But also note that doing this appears to cause a 20-30% page load win for prefs against the baseline version (even without and additional Custom Elements added) - so this seems worth doing regardless of XBL.

Well, "additional" is IMHO the key word here. I don't know how to quickly get a count, but we already use Custom Elements here, e.g. for <radiogroup>, <textbox> and <richlistbox> . That's probably at least part of what the 20-30% win comes from...

Sure, and it's hard to measure exactly since on some of the CE connections would be happening in XBL as well (for UI visible on that particular subpage). But I dont't imagine that individual Custom Element construction here adds up to anything close to 20-30%. To get some unscientific estimates - I see ~7-8ms of time in connectedCallback per cycle (on results that are around 125-175ms depending on the cycle). It's hard to tear apart which of those are eagerly required on each particular cycle but if you said 1/5 (assuming UI was evenly distributed across subpages), that'd be ~5-7ms or so wasted on unrendered CE (or maybe 4-6% total time). So most of the win is still apart from CE. I'm not saying we shouldn't also come up with a platform solution for lazy CE, I'm just saying that we should also be looking into lazifying more markup (and coming up with ways we think are ergonomic to do so. Especially in browser.xul where even a smaller win would be quite significant.

In terms of complexity, perhaps that was the wrong word to use. I'm worried about having to add in this hackery everywhere (see the other deps of bug 1515799). There's also the fact that the CDATA block means nothing checks whether the contents are valid XML/XUL, which will make development more painful and will hide bugs. No XML parsing errors on pageload, but runtime errors when trying to open/"unhide" the relevant content...

I'm not sure if CDATA is the best technical way to do this (certainly at least using html:template would at the least be preferrable, but it isn't available in xul docs). But if we introduced XML parsing errors, for example, there would be some pretty obvious test failures - even if it's not a YSOD. Do you have other ideas for how to lazify markup, or do you think the ergonomics here are worth the perf hit we currently take?

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

Have we considered instead changing when we fire connectedcallbacks or similar for elements that are descendants of <panel>, <deck> and similar elements where the contents aren't currently visible and this is relatively cheap to determine -- or any other types of platform fixes that would scale better and not require reimplementing for all the other cases where we hit this type of problem?

We would need to investigate further to tell if that gives us any gain. Construct custom elements w/ no-op constructor & callback is already slower (there used to be a jsperf.com test case but it got deleted 😞)

Deferring connectedCallback could also mean it is possible for other JS to access not-yet-connected custom elements, which is the same problem we have with XBL in the first place.

(In reply to Brian Grinstead [:bgrins] from comment #11)

I've asked about getting a JS callback for when an element gets a layout frame, so we could manually implement the XBL behavior in JS, but Emilio had a convincing argument against it. I'm sorry, but I can't find the log from that discussion. Emilio, could you elaborate on the reason why we shouldn't be doing this?

I don't recall the exact argument I used that time, but here are a few:

  • Running JS during layout is bad, so we wouldn't want to do it. It's pretty awkward all the hacks we have for XBL constructors to work somewhat sanely.

  • It makes basically impossible any sort of "moving layout to another thread".

  • Frames get constructed and destructed all the time, for other reasons that "we laid out this element for the first time". So running a callback whenever this element got a frame would be (a) expensive and/or (b) require more bookkeeping, either in the browser or in the front-end.

  • ... (I can find more if you want :))

Flags: needinfo?(emilio)

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

Yeah, so, I would specifically not want to do the same thing XBL does. It's really annoying to keep track of when things get frames and then lose frames, that's part of XBL's bugginess. IMO this is because it's not explicit/restricted to the DOM, but also influenced by CSS and other factors.

I'm specifically asking, can we come up with any abstraction at the DOM (rather than layout) level to take care of this automagically? We currently attach things based on BindToTree or similar, I think, with an option to delay until DOMContentLoaded? I'm effectively asking if we can avoid this for some container elements, and define a "better" point to do the attaching for such container elements, based on an attribute or "something else".

  • Other than keying on a [hidden] attribute on a parent, I can't think of anything in particular we could do on the DOM side to delay initialization of Custom Elements, but I'll ni? Emilio just in case he has ideas.
  • Maybe we could do something in the frontend with an attribute on the node like [delay=true], skip connectedCallback logic when the attribute is present, and then manually trigger it from JS whenever we know the node is going to become visible?

As Tim mentions in Comment 14, anything we do here will require calling code to be careful to check if nodes are in a delayed state (which is a "correctness" tradeoff that we'd be making for perf).

I have a feeling we are going to have to come up with a way to lazify markup that we are happy with (<template>, CDATA, virtual DOM, vanilla DOM creation from JS, ...), and then do the hard work to update documents are perf sensitive (especially browser.xul). With the knowledge that we aren't just doing this to accommodate Custom Elements, but that it has all kinds of downstream perf wins (less DOM at startup, less time resolving styles, faster startup time, ...).

Flags: needinfo?(emilio)

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #14)

Have we considered instead changing when we fire connectedcallbacks or similar for elements that are descendants of <panel>, <deck> and similar elements where the contents aren't currently visible and this is relatively cheap to determine -- or any other types of platform fixes that would scale better and not require reimplementing for all the other cases where we hit this type of problem?

We would need to investigate further to tell if that gives us any gain. Construct custom elements w/ no-op constructor & callback is already slower (there used to be a jsperf.com test case but it got deleted 😞)

I can't remember the numbers there, but I did a bunch of testing with an empty CE vs empty XBL attachment in Bug 1387125 (see https://bugzilla.mozilla.org/show_bug.cgi?id=1387125#c24, for instance), and there wasn't a huge difference between them.

I'm not sure of all the requirements that such a solution would have, but I know that a bunch of web apps do sort of cooperative DOM building (like, they build the non-critical pieces of the DOM using JS on idle callbacks and such).

But other than that I don't have any brighter idea.

Flags: needinfo?(emilio)

and it seems that unrelated wpt and mda test failures only shows up on artifact build.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c05c3c88a05cef0b7057bf067fb7a5d74e98700

Pushed by tchien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ad9834d2b44e Lazily load about:preferences markups from hidden panes r=jaws

The failed test is pretty weird: according to dump() someone changed the deck selectedIndex after appUpdater correctly sets it the right value. I am unable to locate who yet.

Flags: needinfo?(timdream)

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #23)

The failed test is pretty weird: according to dump() someone changed the deck selectedIndex after appUpdater correctly sets it the right value. I am unable to locate who yet.

The only offender seems to be

https://searchfox.org/mozilla-central/rev/5c8ea961d04767db723a0a15e3a8f7fbca154129/layout/xul/nsDeckFrame.cpp#156

I will not be able to create a full build on Windows to verify this statement. But if so a simple rAF should fix it.

Pushed by tchien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/41e11bb52568 Lazily load about:preferences markups from hidden panes r=jaws

https://hg.mozilla.org/mozilla-central/rev/41e11bb52568

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox67: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67

== Change summary for alert #18982 (as of Tue, 29 Jan 2019 01:40:18 GMT) ==

Improvements:

16% about_preferences_basic windows7-32 opt e10s stylo 130.26 -> 109.19

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=18982

Depends on: 1532071
Depends on: 1532712
Depends on: 1536752
Depends on: 1544027
Depends on: 1545962
Regressions: 1582740
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: