From IRC:

22:47:41 <florian> looking at Mike's profile... How do we feel about all these custom element JS files being loaded eagerly? https://perfht.ml/2UoBppG
22:48:53 <florian> that's taking longer to load than the big browser.js file

I'd been hoping to be able to look at this stuff myself soon, but I keep being distracted by other stuff. It'd be useful if someone could start looking at this.

The template solution from bug 1520350 has caused quite a few regressions so far, but we probably need to consider something vaguely like that (though for some of the bindings that I'm seeing showing up when opening browser.xul:

0:11.30 GECKO(92563) cache miss: jssubloader/global/resource/gre/chrome/toolkit/content/global/customElements.js
0:11.30 GECKO(92563) cache miss: jssubloader/global/resource/gre/chrome/toolkit/content/global/elements/general.js
0:11.30 GECKO(92563) cache miss: jssubloader/global/resource/gre/chrome/toolkit/content/global/elements/checkbox.js
0:11.30 GECKO(92563) cache miss: jssubloader/global/resource/gre/chrome/toolkit/content/global/elements/menu.js
0:11.30 GECKO(92563) cache miss: jssubloader/global/resource/gre/chrome/toolkit/content/global/elements/notificationbox.js
0:11.31 GECKO(92563) cache miss: jssubloader/global/resource/gre/chrome/toolkit/content/global/elements/popupnotification.js
0:11.31 GECKO(92563) cache miss: jssubloader/global/resource/gre/chrome/toolkit/content/global/elements/radio.js
0:11.31 GECKO(92563) cache miss: jssubloader/global/resource/gre/chrome/toolkit/content/global/elements/richlistbox.js
0:11.31 GECKO(92563) cache miss: jssubloader/global/resource/gre/chrome/toolkit/content/global/elements/autocomplete-popup.js
0:11.31 GECKO(92563) cache miss: jssubloader/global/resource/gre/chrome/toolkit/content/global/elements/autocomplete-richlistitem.js
0:11.31 GECKO(92563) cache miss: jssubloader/global/resource/gre/chrome/toolkit/content/global/elements/textbox.js
0:11.31 GECKO(92563) cache miss: jssubloader/global/resource/gre/chrome/toolkit/content/global/elements/tabbox.js
0:11.31 GECKO(92563) cache miss: jssubloader/global/resource/gre/chrome/toolkit/content/global/elements/tree.js
0:11.32 GECKO(92563) cache miss: jssubloader/global/resource/gre/chrome/toolkit/content/global/elements/wizard.js

we should probably instead be wondering why we're even loading those files... )

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

we should probably instead be wondering why we're even loading those files... )

We added the chrome-only setElementCreationCallback onto CustomElementRegistry as a way to avoid eager loading when it's not necessary. In general I've tried to use that instead of eager subscript loading if the defined CE aren't in browser.xul at startup. But since we have no automated test to prevent it, I'm guessing some have slipped through the cracks here (wizard, tree, and maybe some of the autocomplete stuff seem like they could be candidates here). There are a couple of downsides to lazifying, primarily that extended classes (like places-tree, which extends tree) or callers (like we do for findbar) need to handle the "non registered" case instead of being able to assume they are there. If we will see real startup time improvements though, it's probably worth that trade-off.

A test that iterated defined CEs after a browser window opens and checks to make sure at least one has been constructed may be a good start here. I'm digging out from being away at the moment so likely won't have time to work on it this week, but I'll think about it some more and bring it up in our weekly meeting on Wednesday.

Giving this fxperf:p2 since it's startup-related, and the earlier we can get proactive solutions in place the less reactive work we'll have to do later.

This isn't intended to land, just a demonstration of a way we could get more insight into how much
time we are spending inside of individual classes.

Comment on attachment 9053702 [details]
Bug 1515799 - POC - Instrument base custom element class and print call information in a table at startup

Revision D24953 was moved to bug 1541516. Setting attachment 9053702 [details] to obsolete.

Hm, the instrumentation in bug 1541516 seems to suggest there's a lot more than I thought - though perhaps some of it is new since I filed this bug. Besides the things already on file, I'm also seeing more text, dropdown, richlistbox and menulist widgets created than I would have expected... I'm also seeing stats printed for the hidden window's XUL, which I really wasn't expecting because in principle I'd expect there to be basically no actual XUL in there at all...

Brian, does the browser architecture group have cycles to look at this some more? ("Unfortunately no" is understandable, but I figured I'd ask.)

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

Hm, the instrumentation in bug 1541516 seems to suggest there's a lot more than I thought - though perhaps some of it is new since I filed this bug. Besides the things already on file, I'm also seeing more text, dropdown, richlistbox and menulist widgets created than I would have expected...

By the way, the instrumentation counts aren't very smart right now. It doesn't show self time and will double count calls like if connectedCallback() -> render() then the time spent in render also gets double counted in the totals (as part of both functions).

I'm also seeing stats printed for the hidden window's XUL, which I really wasn't expecting because in principle I'd expect there to be basically no actual XUL in there at all...

The hidden window does have XUL on OSX: https://searchfox.org/mozilla-central/rev/1b2636e8517aa48422ed516affe4d28cb7fa220a/browser/base/content/hiddenWindow.xul#18. I have a condition in the upcoming <menu> CE conversion to explicitly not fire connectedCallback logic for that - perhaps we should make that more general.

Brian, does the browser architecture group have cycles to look at this some more? ("Unfortunately no" is understandable, but I figured I'd ask.)

I've spent some time looking at optimizing parseXULToFragment (like, try using <html:template>, automatically cache the fragment and clone for reuse, etc). Nothing was really showing much win there unfortunately. I've talked with Brendan about investigating it more generally (probably using the <label> patch in https://bugzilla.mozilla.org/show_bug.cgi?id=1448213 for reference since there are a ton of them in tree).

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

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

Hm, the instrumentation in bug 1541516 seems to suggest there's a lot more than I thought - though perhaps some of it is new since I filed this bug. Besides the things already on file, I'm also seeing more text, dropdown, richlistbox and menulist widgets created than I would have expected...

By the way, the instrumentation counts aren't very smart right now. It doesn't show self time and will double count calls like if connectedCallback() -> render() then the time spent in render also gets double counted in the totals (as part of both functions).

Also I wanted to note that one concern I had with landing the probes in bug 1541516 was that it's the only thing that we are instrumenting in this way, so it could lead to a focus on Custom Element performance that doesn't exist for other code (and specifically the XBL code that we are migrating away from).

So if we have a <foo> element that's currently XBL I was concerned the instrumentation would lead to pushback against landing the MozFoo Custom Element, because we now see time showing up in the instrumentation that wasn't there before. And there are obviously cases where this is new time that we want to avoid as much as possible (like if there are lots of hidden <foo> elements with expensive <constructor>s that didn't used to run), but that isn't always the case. Sometimes we are just moving time away from XBL <foo> to Custom Element <foo>.

In the end I wanted it to land it because I think it'll be valuable to see this information and it's a tool that will help us do a better job with performance, but I don't think it makes sense as a metric that's tracked in isolation. If we do want to start tracking it as a performance metric then I think it needs a thorough audit and some improvements (to account for stuff like self-time I mention above, potentially integration with the profiler, etc).

I don't think we're at a point where it's usable to track as a perf metric where we check for regressions, no.

I was more surprised by some of the data it returned, like seeing 2 MozAutocompleteRichlistboxPopup, 3 RichListBox, 16 MozPopupNotification (though I think we're avoiding doing much there ie I'm not seeing connectedCallback, though I'm seeing attribute inheritance, where I wonder if we should be doing that before doing any other work) and 34 MozDropmarker (when on a clean profile, I don't think any dropmarkers are visible in the browser window at launch), 5 MozMenuList (again, I think 0 visible), and a bunch of MozXULMenu elements despite being on mac (so nothing's visible in the browser window. There's a few other things that look odd, too. It just seems worth poking at...

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

I don't think we're at a point where it's usable to track as a perf metric where we check for regressions, no.

I was more surprised by some of the data it returned, like seeing 2 MozAutocompleteRichlistboxPopup, 3 RichListBox, 16 MozPopupNotification (though I think we're avoiding doing much there ie I'm not seeing connectedCallback, though I'm seeing attribute inheritance, where I wonder if we should be doing that before doing any other work) and 34 MozDropmarker (when on a clean profile, I don't think any dropmarkers are visible in the browser window at launch), 5 MozMenuList (again, I think 0 visible), and a bunch of MozXULMenu elements despite being on mac (so nothing's visible in the browser window. There's a few other things that look odd, too. It just seems worth poking at...

Yeah worth looking into anything that looks out of the ordinary, for sure.

• One of the menulists is https://searchfox.org/mozilla-central/rev/1b2636e8517aa48422ed516affe4d28cb7fa220a/browser/base/content/browser.xul#248-260. Perhaps we could <template> that or build it from JS and only initialize it when a <select> is actually opened. Also, menulist inherits from MozXULMenu so it's likely the data for that is really coming from a menulist.
• For dropmarkers, in general just attaching a pretty empty CE should be very fast. I did some micro benchmark on this a while ago but it might be worth double checking and asking DOM folks if we are seeing a slowdown here vs a plain element. I do see it has a connectedCallback but it does very little. If there are any in hidden popups etc we could at least delay rendering until popupshowing.
• MozAutocompleteRichlistboxPopup, RichListBox, MozPopupNotification may all be candidates for lazification. I guess we'll need some kind of guidelines and best practices to decide if/how to do it (https://bugzilla.mozilla.org/show_bug.cgi?id=1544027 might be the place to start figuring that out).

There's still some room for further improvement here, e.g. https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=5d0c71e9afe0d6684a311731f7978b051350c94a&newProject=try&newRevision=f75302ba5da18f6b0c1943039f0c9d44a3bdfe4b&framework=1

This trypush:

1. removes the download panel overlay (which has button elements which have non-trivial overhead, as well as a richlistbox and a number of popups that come with arrowscrollboxes which all also have overhead)
2. removes the 4 menulist instances that we create for each window (1 for the ContentSelectDropdown, 3 in webrtc popupnotifications)
3. removes the checkbox instance we create in each window (in the password popup notification)

I'm seeing 10-20ms drops in timings for window opening tests, which amounts to anywhere between 1.2 and 9% improvements on sessionstore, ts_paint, and twinopen.

The simplest way to reduce this overhead for this naive trypush was to disable the bindings and/or use templates / XML comments to avoid these nodes being in the DOM. To fix this "for real", we could either create/insert some of this markup dynamically, or we could try and investigate why these particular custom elements have such significant overhead, and try to address that. The former is easier, but also makes it easier to regress some of this again in future. The latter is harder, and it may not be possible to fix some of the overhead from the bindings without breaking legitimate uses of the binding.

Besides what's in the trypush, there's also more that could be done around the overhead created by toolbarbuttons - there are some 40-odd ones left in a vanilla browser window that take some 10ms to be built/rendered on my local (fast, developer) machine. These 40 items include 7 toolbarbuttons in the sidebar (hidden by default), and 11 minimize/maximize/restore/close buttons (of which only 3 are visible to start with, with most never becoming visible - we have button items in each toolbar, for use with fullscreen and/or various forms of tabsintitlebar with the menubar visible / invisible). Then there are some other hidden items, like the ion and whatsnew buttons, which are not usually displayed on most startups (including first startup).

We also create labels for each of those 40-odd toolbar buttons, when only the bookmarks and import button have visible labels, and despite most of the toolbarbutton's binding time being spent on the labeling, inheriting of attributes, and dealing with potential other child nodes - so it may be worth splitting the binding so that we use a separate one for buttons that just have an icon, and for which we never show the label.