1396575 - Track performance for creating an autonomous custom elements which doesn't match CE definition

Status: RESOLVED FIXED

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

Description

[John Dai[:jdai]]

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

* The nearly-normal case (createElement, with the changes, define() call, one arg), is also much slower than what we have now.  Needs to be profiled, understood, and fixed: the one-arg case should have no performance penalty, I would think.

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

Comment 1

[John Dai[:jdai]]

This bug is for nearly-normal case (createElement, with the changes, define() call, one arg) that is we defined a custom element, but createElement doesn't use that defined custom element to create. Ex:

class FooElement extends HTMLDivElement {
  constructor() {
    super();
  }
};
customElements.define("x-foo", FooElement, { extends: "div" });

document.createElement("div");

And the bug title will be moved to bug 1419661.

Comment 2

[Edgar Chen [:edgar]]

(In reply to John Dai[:jdai] from comment #1)
> This bug is for nearly-normal case (createElement, with the changes,
> define() call, one arg) that is we defined a custom element, but
> createElement doesn't use that defined custom element to create. Ex:
> 
> class FooElement extends HTMLDivElement {
>   constructor() {
>     super();
>   }
> };
> customElements.define("x-foo", FooElement, { extends: "div" });
> 
> document.createElement("div");

This creates a normal element and this case is already tracked in bug 1396573.
I guess this bug is filed for "creating a custom element which doesn't match with any existing definition".
So perhaps, the test should be "document.createElement("x-foo");" ?

Comment 3

[John Dai[:jdai]]

(In reply to Edgar Chen [:edgar] from comment #2)
> (In reply to John Dai[:jdai] from comment #1)
> > This bug is for nearly-normal case (createElement, with the changes,
> > define() call, one arg) that is we defined a custom element, but
> > createElement doesn't use that defined custom element to create. Ex:
> > 
> > class FooElement extends HTMLDivElement {
> >   constructor() {
> >     super();
> >   }
> > };
> > customElements.define("x-foo", FooElement, { extends: "div" });
> > 
> > document.createElement("div");
> 
> This creates a normal element and this case is already tracked in bug
> 1396573.
> I guess this bug is filed for "creating a custom element which doesn't match
> with any existing definition".
> So perhaps, the test should be "document.createElement("x-foo");" ?

Maybe we can consider this bug is for creating an autonomous custom element or a customized built-in element which doesn't match with any existing definition. Does that make sense?

Comment 4

[Edgar Chen [:edgar]]

(In reply to John Dai[:jdai] from comment #3)
> Maybe we can consider this bug is for creating an autonomous custom element
> or a customized built-in element which doesn't match with any existing
> definition. Does that make sense?

I thought bug 1419660 is filed for creating a customized built-in element which doesn't match with any existing definition?

Comment 5

[John Dai[:jdai]]

(In reply to Edgar Chen [:edgar] from comment #4)
> (In reply to John Dai[:jdai] from comment #3)
> > Maybe we can consider this bug is for creating an autonomous custom element
> > or a customized built-in element which doesn't match with any existing
> > definition. Does that make sense?
> 
> I thought bug 1419660 is filed for creating a customized built-in element
> which doesn't match with any existing definition?

Bug 1419660 is for creating a customized built-in element with no define() calls(testcase [1]). We can separate creating a customized built-in element which doesn't match with any existing definition to another bug if it'll make this bug more clear.

[1] https://edgarchen.github.io/tests/CustomElements/Performance/CreateElement/Builtin/no_definition_two_arg_with_is.html

Comment 6

[John Dai[:jdai]]

Per off-line discussion, this bug is for creating an autonomous custom elements which doesn't match CE definition. We can consider CE preference is on/off case for document.createElement("x-foo").

Comment 7

[Edgar Chen [:edgar]]

Attachment: test.html

Comment 8

[Edgar Chen [:edgar]]

Quick test attachment #894177 on recent m-c, 05fed903f40f
pref-off: ~33ms
pref-on: ~54ms

In pref-on case, definition lookup is one possible operation that makes the createElement() slower. But I am not sure if there are others, need to do more investigation.

Comment 9

[Edgar Chen [:edgar]]

Test: attachment #8941774 [details] with pref-on
Base revision: 6f9b763bb1c9
Profile: https://perfht.ml/2DfQqWg

Comment 10

[Edgar Chen [:edgar]]

From profile [1] in comment #9, I found we spend some time atomizing aLocalName when looking up definition [2]. And the caller usually pass nodeInfo->LocalName() as aLocalName, like [3]. I wondering if we could use nodeInfo->NameAtom() given that these code is only called with element's nodeInfo which the LocalName() and NameAtom() should be the same value. Does this make sense?

[1] https://perf-html.io/public/708b78ee5c83e942269ea6b20795464b61337624/calltree/?hiddenThreads=&invertCallstack&range=2.6524_2.7236&thread=5&threadOrder=0-2-3-4-5-1&transforms=f-combined-xAi-i&v=2
[2] https://searchfox.org/mozilla-central/rev/41925c0b6c6d58578690121de439d2a8d3d690f3/dom/base/CustomElementRegistry.cpp#290
[3] https://searchfox.org/mozilla-central/rev/41925c0b6c6d58578690121de439d2a8d3d690f3/dom/base/nsContentUtils.cpp#10012

Comment 11

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

Sounds reasonable given that we're dealing with elements
https://searchfox.org/mozilla-central/rev/48cbb200aa027a0a379b6004b6196a167344b865/dom/base/NodeInfo.cpp#79,89

Comment 12

[Edgar Chen [:edgar]]

(In reply to Olli Pettay [:smaug] (please try to find other reviewers for non-web components patches) from comment #11)
> Sounds reasonable given that we're dealing with elements
> https://searchfox.org/mozilla-central/rev/
> 48cbb200aa027a0a379b6004b6196a167344b865/dom/base/NodeInfo.cpp#79,89

Filed bug 1430951. Thank you!

Comment 13

[Edgar Chen [:edgar]]

Another found: we spend some time AddRef()ing and Release()ing of atoms in nsContentUtils::NewXULOrHTMLElement [1], some is from https://searchfox.org/mozilla-central/rev/48cbb200aa027a0a379b6004b6196a167344b865/dom/base/nsContentUtils.cpp#9994. It seems we could just use raw pointer there given that the atom is owned by nodeInfo, but I am not not sure how much this could improve.

[1] https://perf-html.io/public/708b78ee5c83e942269ea6b20795464b61337624/calltree/?hiddenThreads=&invertCallstack&range=4.8478_4.9187&thread=5&threadOrder=0-2-3-4-5-1&v=2

Comment 14

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

We've fixed the obvious cases. One thing which would help is faster slots allocation.

Testcase is faster in Gecko than in Blink.

Marking fixed for now.