1392974 - [CEReactions] operations should not cause noticeable slowdown if the documents don't have any custom element when custom element feature is enabled

Status: RESOLVED FIXED

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

Description

[John Dai[:jdai]]

+++ This bug was initially created as a follow-up of Bug #1392790 comment #4+++

Even once we enable custom elements, this code should be fast for documents which don't have any custom elements. But that optimization can be done in a different bug.

Comment 1

[Edgar Chen [:edgar]]

If my understanding correct, this bug is filed for making [CEReaction] operations faster.

Comment 2

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6be18d75fe5ebd2750e7445c667b4f02de44604&group_state=expanded&filter-tier=1

Comment 3

[Edgar Chen [:edgar]]

I am thinking of lazily creating AutoCEReaction when the operations/attributes annotated with the [CEReactions] really enqueue reactions. This could probably make normal case (not use CustomElement feature) faster.

Comment 4

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

Need to write some microbenchmarks for this all.
But in general I wonder why GetCustomElementReactionsStack works the way it does.
Usually the caller does have already the relevant Element. Accessing DocGroup from that should be faster than via JSObject.

Comment 5

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

Attachment: a testcase

CustomElementReactionsStack::CreateAndPushElementQueue() call is slow
(and I don't understand why we need it if the relevant element isn't CE, but perhaps I'm missing something) and GetCustomElementReactionsStack() is slow too, mostly because of CheckedUnwrap and WindowGlogalOrNull

Comment 6

[Edgar Chen [:edgar]]

(In reply to Olli Pettay [:smaug] from comment #4)
> Usually the caller does have already the relevant Element. Accessing
> DocGroup from that should be faster than via JSObject.

(I assume you mean the __self__ is an Element-like pointer)
Not every caller has the relevant Element, e.g.
https://searchfox.org/mozilla-central/source/__GENERATED__/dom/bindings/CustomElementRegistryBinding.cpp#180
https://searchfox.org/mozilla-central/source/__GENERATED__/dom/bindings/CSSStyleDeclarationBinding.cpp#38
https://searchfox.org/mozilla-central/source/__GENERATED__/dom/bindings/DOMStringMapBinding.cpp#457
....

But we could probably create a helper function to deal with that, instead of getting it via JSObject.
I will try this way.

(In reply to Olli Pettay [:smaug] from comment #5)
> (and I don't understand why we need it if the relevant element isn't CE, but
> perhaps I'm missing something)

Even if the relevant element isn't a CE, it is still possilbe enqueuing reactions, for example, <div><custom-element></custom-element></div>. If we call the remove() method of <div>, a disconnected callback will be enqueued for <custom-element>.

Comment 7

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

right. So do we need a flag telling whether CEReaction is needed for an element?
And do we need different versions of AutoCEReaction, so that we can deal with for example Elements explicitly there, hopefully in fastest possible way, and perhaps CSS stuff would be a bit slower?

Comment 8

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=39c2411988b0f69952492a27b1ca1d5ae6234b83&group_state=expanded&filter-tier=1

Comment 9

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=56a999845f8683f3689423a645d26e401579d6e4

Comment 10

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar] from comment #9)
> https://treeherder.mozilla.org/perf.html#/
> comparechooser?newProject=try&newRevision=56a999845f8683f3689423a645d26e40157
> 9d6e4

The result of dromaeo_dom and microbenchmarks looks much better after applying the WIP patches,

dromaeo_dom opt e10s:
  linux64    -0.97% 
  linux64-qr -0.76%

dromaeo_dom opt e10s stylo_disabled:
  linux64    -1.17% 

dromaeo_dom pgo e10s:
  linux64    -0.22% 

dromaeo_dom pgo e10s stylo_disabled:
  linux64    0.29%

So I am going to,
- Add fast path to get DocGroup for binding code.
- Lazy push/pop CustomElementReactionsStack entry when we really enqueue reactions.

Will probably file a separated bug for each of them.

Comment 11

[Edgar Chen [:edgar]]

Attachment: testcase 2

Comment 12

[Edgar Chen [:edgar]]

Test https://bugzilla.mozilla.org/attachment.cgi?id=8932732 again with recent m-c (8062887ff0d9), the average time with and without CE enabled are almost the same, so close this bug.