Closed Bug 1419661 Opened 2 years ago Closed 2 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: jdai, Assigned: smaug)

References

Details

Attachments

(2 files)

+++ 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
Needs profiling.
Duplicate of this bug: 1419660
Depends on: 1420178
Depends on: 1428784
Depends on: 1469446
The main issue here is that custom elements require slots and extended slots to be created asap, because CustomElementData lives on the extended slots.
And also CustomElementData is allocated,so 3 extra allocations comparing to normal elements.
oh, wait, I was profiling without define
This is weird bug actually, since of course we don't have anything like this without custom elements.
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
But then, blink doesn't have built-ins enabled, I think.
Depends on: 1470191
Depends on: 1440382
Attached patch fatslots.diffSplinter Review
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
Assignee: nobody → bugs
Attachment #8988366 - Flags: review?(mrbkap)
(the failures on try aren't coming from this patch.)
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.
Attachment #8988366 - Flags: review?(mrbkap) → review+
Attached patch fatslots_2.diffSplinter Review
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a054f354013
if ExtendedDOMSlots are used before slots, use FatSlots to have fewer allocations, r=mrbkap
https://hg.mozilla.org/mozilla-central/rev/2a054f354013
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.