1419661 - When custom elements preference is on, createElement with define() call and it'll run upgrade which is much slower than what we have now

Status: RESOLVED FIXED

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

Description

[John Dai[:jdai]]

+++ This bug was initially created as a follow-up of bug 1299363 comment #77 +++

* We should measure where the additional overhead comes from in the "have second arg with is= that matches a definition" case.  I can think of various sources, some of which already have bugs filed on them, but it would be good to see whether there's something non-obvious in there.

Performance detail result:
https://docs.google.com/a/mozilla.com/spreadsheets/d/1OLntq8Qd4e4td7ml6iq0r1sVxl4phMta3HFAAMvpAms/edit?usp=sharing

Comment 1

[Andrew Overholt [:overholt]]

Needs profiling.

Comment 3

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

The main issue here is that custom elements require slots and extended slots to be created asap, because CustomElementData lives on the extended slots.

Comment 4

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

And also CustomElementData is allocated,so 3 extra allocations comparing to normal elements.

Comment 5

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

oh, wait, I was profiling without define

Comment 6

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

This is weird bug actually, since of course we don't have anything like this without custom elements.

Comment 7

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

Perhaps this is trying to capture different issues.
Anyhow, https://edgarchen.github.io/tests/CustomElements/Performance/CreateElement/Builtin/no_definition_two_arg_with_is.html is one where we're quite a bit slower than blink

Comment 8

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

But then, blink doesn't have built-ins enabled, I think.

Comment 9

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

Attachment: fatslots.diff

Would have asked ehsan to review this, but he is on vacation.

We have currently slots and then extendedSlots for even more rarely used
things. Web components stuff lives in extended slots.
The patch makes us create FatSlots object in case we want to use extended slots before normal slots. FatSlots is both the normal slots and extendedslots at the same time. This setup is for now for FragmentOrElement only.
Creating FatSlots reduces one memory allocation during custom element creation, and malloc is _slow_.


(I'm not seeing other obvious things to optimize, but will reprofile.
Well, bindings layer is surprisingly slow here.)

remote: 
remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/6ce0dfbf830bab008704bdfee4128ca2f8b4b240
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ce0dfbf830bab008704bdfee4128ca2f8b4b240
remote: recorded changegroup in replication log in 0.119s

Comment 10

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

(the failures on try aren't coming from this patch.)

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8988366 [details] [diff] [review]
fatslots.diff

Review of attachment 8988366 [details] [diff] [review]:
-----------------------------------------------------------------

::: servo/components/style/gecko/wrapper.rs
@@ +651,5 @@
>      #[inline]
>      fn extended_slots(&self) -> Option<&structs::FragmentOrElement_nsExtendedDOMSlots> {
>          self.dom_slots().and_then(|s| unsafe {
> +            let e_slots = s._base.mExtendedSlots &
> +                !structs::nsIContent_nsContentSlots_sNonOwningExtendedSlotsFlag;

A comment pointing back to nsContentSlots::GetExtendedSlots would probably be nice here.

Comment 12

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

Attachment: fatslots_2.diff

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a054f354013
if ExtendedDOMSlots are used before slots, use FatSlots to have fewer allocations, r=mrbkap

Comment 14

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/2a054f354013