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

RESOLVED FIXED in Firefox 64

Status

()

enhancement
P3
normal
RESOLVED FIXED
10 months ago
7 months ago

People

(Reporter: bgrins, Assigned: emilio)

Tracking

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Reporter

Description

10 months ago
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

10 months ago
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.
Reporter

Comment 2

10 months ago
(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

10 months ago
So I guess there isn't anything to figure out here.
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → WORKSFORME
Reporter

Comment 4

10 months ago
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.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Summary: Figure out why HasCustomElementData is true on XUL elements that don't appear to have custom elements defined for their tag name → Return false in MayNeedToLoadXBLBinding whenever a Custom Element is actually defined for a XUL tag name
Reporter

Updated

10 months ago
Assignee: nobody → bgrinstead
Status: REOPENED → ASSIGNED
Reporter

Comment 5

10 months ago
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.
Reporter

Comment 6

10 months ago
Posted file 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.
Reporter

Comment 7

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

> 35 NO binding dropmarker

So it's the second most computed after command.
Reporter

Comment 8

10 months ago
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

10 months ago
Please upgrade to P2 if appropriate.
Priority: -- → P3
(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.
Reporter

Comment 11

10 months ago
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.
Reporter

Comment 14

10 months ago
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.
Assignee: bgrinstead → nobody
Status: ASSIGNED → NEW
Assignee

Comment 15

9 months ago
Trying something.
Assignee: nobody → emilio
Assignee

Comment 16

9 months ago
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 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.
Attachment #9011122 - Flags: review+
Assignee

Comment 18

9 months ago
Brian lmk when you have your Talos push around, to ensure that we don't regress anything with this patch. Thanks!
Flags: needinfo?(bgrinstead)
Reporter

Comment 19

9 months ago
(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
Flags: needinfo?(bgrinstead)
Assignee

Comment 21

9 months ago
Both look fine.

Comment 22

9 months ago
Pushed by emilio@crisal.io:
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

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c7203fb6f1da
Status: NEW → RESOLVED
Closed: 10 months ago9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Attachment #9005245 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.