Move custom element definitions into ElementRareData

Move custom element definitions into ElementRareData

Instead of keeping a separate hash map we should just store this
information in the ElementRareData. This simplifies the implementation and
in practice will likely reduce memory usage since almost all custom elements
we've seen so far touch something that ends up making them use rare data like
childNodes, attributes, tabIndex, classList or dataset.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169609

[dominicc (has gone to gerrit), 2014-03-03 04:32:23]

LGTM from a code point of view; I'd like to know more about performance.

This implies if we use Custom Elements for built-in elements down the road that
we're OK with them having ERD baggage. I suspect this is probably true; what
does dglazkov think?

https://codereview.chromium.org/177063004/diff/1/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/177063004/diff/1/Source/core/dom/Element.cpp#...
Source/core/dom/Element.cpp:1742:
ensureElementRareData().setCustomElementDefinition(definition);
We should assert that there isn't a definition already here.

https://codereview.chromium.org/177063004/diff/1/Source/core/dom/ElementRareD...
File Source/core/dom/ElementRareData.cpp (right):

https://codereview.chromium.org/177063004/diff/1/Source/core/dom/ElementRareD...
Source/core/dom/ElementRareData.cpp:43: void* pointers[12];
What's the performance impact of this?

https://codereview.chromium.org/177063004/diff/1/Source/core/dom/custom/Custo...
File Source/core/dom/custom/CustomElement.h (left):

https://codereview.chromium.org/177063004/diff/1/Source/core/dom/custom/Custo...
Source/core/dom/custom/CustomElement.h:65: // API for wrapper creation, which
uses a definition as a key
Yay.

What do you think about moving the element -> callback queue map into ERD too?

[dglazkov, 2014-03-03 17:13:21]

On 2014/03/03 04:32:23, dominicc wrote:
> LGTM from a code point of view; I'd like to know more about performance.
> 
> This implies if we use Custom Elements for built-in elements down the road
that
> we're OK with them having ERD baggage. I suspect this is probably true; what
> does dglazkov think?

I support this.

[esprehn, 2014-03-05 23:12:04]

On 2014/03/03 04:32:23, dominicc wrote:
> LGTM from a code point of view; I'd like to know more about performance.
> 
> This implies if we use Custom Elements for built-in elements down the road
that
> we're OK with them having ERD baggage. I suspect this is probably true; what
> does dglazkov think?

Yeah that should be fine. At the point where we think a lot of the platform is
using custom elements we can move this to a top level pointer in Element.h since
it won't be rare. :)

> 
> https://codereview.chromium.org/177063004/diff/1/Source/core/dom/Element.cpp
> File Source/core/dom/Element.cpp (right):
> 
>
https://codereview.chromium.org/177063004/diff/1/Source/core/dom/Element.cpp#...
> Source/core/dom/Element.cpp:1742:
> ensureElementRareData().setCustomElementDefinition(definition);
> We should assert that there isn't a definition already here.
> 

Okay, I'll add an ASSERT.

>
https://codereview.chromium.org/177063004/diff/1/Source/core/dom/ElementRareD...
> Source/core/dom/ElementRareData.cpp:43: void* pointers[12];
> What's the performance impact of this?
> 

There shouldn't be much. It might make micro benchmarks look a bit slower but
looking at polymer apps there's no difference since they touch attributes on
every element causing an ERD allocation. Making ERD bigger hasn't shown as a
perf impact at any point, it used to be much bigger and we lazy allocated parts
and so no perf change.

>
https://codereview.chromium.org/177063004/diff/1/Source/core/dom/custom/Custo...
> Source/core/dom/custom/CustomElement.h:65: // API for wrapper creation, which
> uses a definition as a key
> Yay.
> 
> What do you think about moving the element -> callback queue map into ERD too?

Yeah I think we should do that in the next patches. We should probably share the
definition between instances of the same element too?

[dominicc (has gone to gerrit), 2014-03-18 23:56:31]

On 2014/03/05 23:12:04, esprehn wrote:
> Yeah I think we should do that in the next patches. We should probably share
the
> definition between instances of the same element too?

I think we already do that, no?

[esprehn, 2014-03-20 00:54:52]

On 2014/03/18 at 23:56:31, dominicc wrote:
> On 2014/03/05 23:12:04, esprehn wrote:
> > Yeah I think we should do that in the next patches. We should probably share
the
> > definition between instances of the same element too?
> 
> I think we already do that, no?

Yeah it does seem we already do that.

[esprehn, 2014-03-20 00:57:48]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2014-03-20 00:57:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/esprehn@chromium.org/177063004/20001

[commit-bot: I haz the power, 2014-03-20 02:16:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-20 02:16:11]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[esprehn, 2014-03-20 02:32:33]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2014-03-20 02:32:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/esprehn@chromium.org/177063004/20001

[commit-bot: I haz the power, 2014-03-20 04:03:23]

Change committed as 169609