1484285 - Return false in MayNeedToLoadXBLBinding whenever a Custom Element is actually defined for a XUL tag name

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P3)

Description

[Brian Grinstead [:bgrins]]

I'm planning to land the MayNeedToLoadXBLBinding optimization for dropmarker specifically in Bug 1478999, but ideally we could skip computing style for all Custom Elements (which shouldn't also have a XBL binding attached).

From discussion at https://phabricator.services.mozilla.com/D3539:

> Looks like an accessibility test is failing with that change: 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=47ce00f86b8f72480a96732e5a41cb8050082c43&selectedJob=194539267
> which is interesting.. I wonder if we are accidentally putting a CE and a XBL
> binding on the same node.

> So apparently HasCustomElementData() is true for xul colorpicker in that test.
> Which is.. surprising considering that AFAIK we never register a colorpicker
> custom element.

> I'm going to keep the MayNeedToLoadXBLBinding function, but update it to special 
> case dropmarker only, and then make a follow up bug to figure out what is going 
> on.

> I'm going to keep the MayNeedToLoadXBLBinding function, but update it to special 
> case dropmarker only, and then make a follow up bug to figure out what is going 
> on.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

It was wanted that basically any xul element may be a custom element, so they all have CustomElementData which is needed for pending, will-possible-be-custom-element-in-the-future, elements.

Comment 2

[Brian Grinstead [:bgrins]]

(In reply to Olli Pettay [:smaug] from comment #1)
> It was wanted that basically any xul element may be a custom element, so
> they all have CustomElementData which is needed for pending,
> will-possible-be-custom-element-in-the-future, elements.

Thanks, I was wondering if something like that was the case. From Emilio on #developers: you could actually check whether the definition is in the `eCustom` state (or not `eUndefined`, I guess).

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

So I guess there isn't anything to figure out here.

Comment 4

[Brian Grinstead [:bgrins]]

Will repurpose this for making the change in Comment 2 (as opposed to targeting dropmarker specifically). This way as we migrate more stuff out of XBL it will automatically skip looking up computed style for this function.

Comment 5

[Brian Grinstead [:bgrins]]

It seems for both dropmarker (which does have a CE) and colorpicker (which doesn't), `aElement.GetCustomElementData()->mState == CustomElementData::State::eUndefined)` seems to be true during MayNeedToLoadXBLBinding. I'm not sure if it's related, but after adding a constructor to https://searchfox.org/mozilla-central/rev/71ef4447db179639be9eff4471f32a95423962d7/toolkit/content/widgets/general.js#38 and doing console.log() from it, it seems like the constructor runs after MayNeedToLoadXBLBinding.

Comment 6

[Brian Grinstead [:bgrins]]

Attachment: binding-logging.txt

I had a patch which ended up returning false for all xul elements. Which is incorrect behavior, but led to some talos wins: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=a36848a5629d4a95dd2dfcd28b22e6c4d823a5d6&newProject=try&newRevision=f60f0a7f8e81f93452783ab9350d919deccafe33&framework=1&showOnlyImportant=1.

So I did some logging when a new window is opened for when Element::GetBindingURL and we end up computing styles. Here's the output sorted based on usage:

> sort binding-logging.txt | uniq -c | sort -r

  59 Got binding menuitem chrome://global/content/bindings/menu.xml#menuitem
  48 NO binding command
  43 Got binding menupopup chrome://global/content/bindings/popup.xml#popup
  19 NO binding menuseparator
  16 NO binding image
  16 Got binding toolbarbutton chrome://global/content/bindings/toolbarbutton.xml#toolbarbutton
  15 Got binding menu chrome://global/content/bindings/menu.xml#menu
  13 NO binding hbox
  11 NO binding vbox
   7 NO binding broadcaster
   7 Got binding menu chrome://global/content/bindings/menu.xml#menu-menubar
   4 NO binding toolbarseparator
   4 NO binding toolbaritem
   4 NO binding key
   4 Got binding label chrome://global/content/bindings/text.xml#text-label
   3 NO binding panelview
   3 NO binding box
   3 Got binding toolbar chrome://browser/content/customizableui/toolbar.xml#toolbar
   3 Got binding menu chrome://global/content/bindings/menu.xml#menu-iconic
   2 NO binding toolbarspring
   2 Got binding toolbarbutton chrome://global/content/bindings/toolbarbutton.xml#toolbarbutton-badged
   2 Got binding menuitem chrome://global/content/bindings/menu.xml#menuitem-iconic
   2 Got binding label chrome://global/content/bindings/text.xml#text-link
   2 Got binding description chrome://global/content/bindings/text.xml#text-base
   1 NO binding window
   1 NO binding toolbox
   1 NO binding toolbarpalette
   1 NO binding stringbundleset
   1 NO binding splitter
   1 NO binding panelmultiview
   1 NO binding moz-input-box
   1 NO binding menubar
   1 NO binding keyset
   1 NO binding deck
   1 Got binding toolbar chrome://browser/content/customizableui/toolbar.xml#toolbar-menubar-stub
   1 Got binding tabs chrome://browser/content/tabbrowser.xml#tabbrowser-tabs
   1 Got binding tabpanels chrome://global/content/bindings/tabbox.xml#tabpanels
   1 Got binding tabbox chrome://global/content/bindings/tabbox.xml#tabbox
   1 Got binding tab chrome://browser/content/tabbrowser.xml#tabbrowser-tab
   1 Got binding notificationbox chrome://global/content/bindings/notification.xml#notificationbox
   1 Got binding menupopup chrome://global/content/bindings/popup.xml#popup-scrollbars
   1 Got binding button chrome://global/content/bindings/button.xml#button

Presumably in at least some of the cases where we don't have a binding and also don't have a Custom Element (command, menuseparator, image) we could short circuit as we do dropmarker.

Comment 7

[Brian Grinstead [:bgrins]]

And when commenting out the dropmarker early return and I see:

> 35 NO binding dropmarker

So it's the second most computed after command.

Comment 8

[Brian Grinstead [:bgrins]]

Early returning on some of the common ones (dropmarker, command, image, menuseparator) does lead to less work, although I'm not seeing anything meaningful in talos so far: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=b5bcbeac0ccaf33c285f66a62491b9163c95bf6a&newProject=try&newRevision=03859f52a747cc63ec88835ece1da8b7b7f06c84&framework=1&showOnlyImportant=1

Comment 9

[Edgar Chen [:edgar]]

Please upgrade to P2 if appropriate.

Comment 10

[Florian Quèze [:florian]]

(In reply to Brian Grinstead [:bgrins] from comment #6)
> Created attachment 9002829 [details]
> binding-logging.txt
> 
> I had a patch which ended up returning false for all xul elements. Which is
> incorrect behavior, but led to some talos wins:
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=a36848a5629d4a95dd2dfcd28b22e6c4
> d823a5d6&newProject=try&newRevision=f60f0a7f8e81f93452783ab9350d919deccafe33&
> framework=1&showOnlyImportant=1.

The large (13-17%) win in about_preferences_basic makes me wonder if we should investigate what the preferences code does specifically that needs to access so many invisible dom nodes, and try to delay it.

Comment 11

[Brian Grinstead [:bgrins]]

I did some more testing with a larger whitelist of tags and same as Comment 8, but I'm still not seeing any detectable wins on talos. So not sure if it's worth landing that.

And still haven't had any luck determining if a XUL element actually has a custom element defined or not in this function.

I'll push up my patches for reference at least.

Comment 12

[Brian Grinstead [:bgrins]]

Attachment: Bug 1484285 - WIP - Don't check for -moz-binding for any XUL element with a Custom Element defined

Comment 13

[Brian Grinstead [:bgrins]]

Attachment: Bug 1484285 - WIP - Expand list of tag names that are known to not have XBL bindings for GetBindingURL

Comment 14

[Brian Grinstead [:bgrins]]

Going to drop this for now. But if we can figure out what's going on with Comment 5, or if any new elements that could be whitelisted start to show up in profiles then this would be a nice to have.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Trying something.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1484285 - Avoid loading XBL bindings for stuff that is very likely to be a custom-element. r=smaug

This should be more generic than hardcoding the tags, and be enough for our
chrome to avoid regressions as they migrate more stuff to CE, avoiding calling
into the GetComputedStyle machinery..

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9011122 [details]
Bug 1484285 - Avoid loading XBL bindings for stuff that is very likely to be a custom-element. r=smaug

Olli Pettay [:smaug] (r- if the bug doesn't explain what the change(s) are about.) has approved the revision.

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Brian lmk when you have your Talos push around, to ensure that we don't regress anything with this patch. Thanks!

Comment 19

[Brian Grinstead [:bgrins]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #18)
> Brian lmk when you have your Talos push around, to ensure that we don't
> regress anything with this patch. Thanks!

Still waiting on them to complete, but here's the comparison with m-c base:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=4fa7b50eb4b23f4ef98a190f64aa5f489ebb9976&newProject=try&newRevision=8d2364903d37665950880ea51ac005bb79a509ce&framework=1&showOnlyImportant=1

Comment 20

[Brian Grinstead [:bgrins]]

And a normal try push with your patch applied: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ab408c68fd50c2242f1437d6c4f3d413911d799

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Both look fine.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c7203fb6f1da
Avoid loading XBL bindings for stuff that is very likely to be a custom-element. r=smaug

Comment 23

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/c7203fb6f1da