14.72 KB, text/plain
46 bytes, text/x-phabricator-request
|Details | Review|
Bug 1484285 - Avoid loading XBL bindings for stuff that is very likely to be a custom-element. r=smaug
46 bytes, text/x-phabricator-request
|Details | Review|
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.
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.
(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).
So I guess there isn't anything to figure out here.
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → WORKSFORME
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
Assignee: nobody → bgrinstead
Status: REOPENED → ASSIGNED
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.
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.
And when commenting out the dropmarker early return and I see: > 35 NO binding dropmarker So it's the second most computed after command.
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
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.
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.
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: nobody → emilio
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+
Brian lmk when you have your Talos push around, to ensure that we don't regress anything with this patch. Thanks!
(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
And a normal try push with your patch applied: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ab408c68fd50c2242f1437d6c4f3d413911d799
Both look fine.
Pushed by email@example.com: 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
You need to log in before you can comment on or make changes to this bug.