Bug 104312 – Shrink the size of NodeRareData by moving pointers into separate objects

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

Implement what I suggested in https://bugs.webkit.org/show_bug.cgi?id=104202#c22:
1. Move ChildNodeList back into NodeListsNodeData
2. Move m_mutationObserverRegistry & m_transientMutationObserverRegistry into a separate object owned by NodeRareData
3. Move m_itemProp, m_itemRef, m_itemType & into a separate object owned by NodeRareData

[Ryosuke Niwa]

Created attachment 178119 [details]
Patch

[Early Warning System Bot]

Comment on attachment 178119 [details]
Patch

Attachment 178119 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15159916

[Early Warning System Bot]

Comment on attachment 178119 [details]
Patch

Attachment 178119 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15186162

[Elliott Sprehn]

Comment on attachment 178119 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178119&action=review

> Source/WebCore/ChangeLog:16
> +        This patch also fixes Mac build when microdata is enabled.

Why?

> Source/WebCore/ChangeLog:57
> +        is different from that, Node, of LiveNodeList::namedItem. It was shadowing the function name instead of overriding.

This seems like it should be in a separate patch.

> Source/WebCore/dom/NodeRareData.cpp:41
> +struct SameSizeAsNodeRareData {

This is really the same size? What about the super class?

> Source/WebCore/dom/NodeRareData.cpp:53
> +COMPILE_ASSERT(sizeof(NodeRareData) == sizeof(SameSizeAsNodeRareData), NodeRareDataShouldStaySmall);

We usually (always?) use underscores for separating the assert name: NodeRareData_should_stay_small

> Source/WebCore/html/HTMLPropertiesCollection.idl:41
> +    [ImplementedAs=propertyNodeList] PropertyNodeList namedItem(in DOMString name);

Why is this change in here? It doesn't seem related.

[Ryosuke Niwa]

Created attachment 178124 [details]
Fixed builds

[Ryosuke Niwa]

(In reply to comment #4)
> > Source/WebCore/dom/NodeRareData.cpp:53
> > +COMPILE_ASSERT(sizeof(NodeRareData) == sizeof(SameSizeAsNodeRareData), NodeRareDataShouldStaySmall);
> 
> We usually (always?) use underscores for separating the assert name: NodeRareData_should_stay_small

That clearly violates the WebKit style guide: http://www.webkit.org/coding/coding-style.html#names We need to fix existing ones.

[Elliott Sprehn]

(In reply to comment #6)
> (In reply to comment #4)
> > > Source/WebCore/dom/NodeRareData.cpp:53
> > > +COMPILE_ASSERT(sizeof(NodeRareData) == sizeof(SameSizeAsNodeRareData), NodeRareDataShouldStaySmall);
> > 
> > We usually (always?) use underscores for separating the assert name: NodeRareData_should_stay_small
> 
> That clearly violates the WebKit style guide: http://www.webkit.org/coding/coding-style.html#names We need to fix existing ones.

I'm not sure that style thing really matches. This is what's output when the assertion fails.

[Elliott Sprehn]

Comment on attachment 178124 [details]
Fixed builds

View in context: https://bugs.webkit.org/attachment.cgi?id=178124&action=review

> Source/WebCore/dom/NodeRareData.cpp:43
> +    unsigned indicesAndBitfields[2];

Naming doesn't match. m_indicesAndBitfields.

> Source/WebCore/dom/NodeRareData.h:233
> +struct NodeMicroDataTokenLists {

Can we place these structs inside the NodeRareData definition? No one should ever call ::create() on them outside of here.

[Ryosuke Niwa]

(In reply to comment #8)
> (From update of attachment 178124 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178124&action=review
> 
> > Source/WebCore/dom/NodeRareData.cpp:43
> > +    unsigned indicesAndBitfields[2];
> 
> Naming doesn't match. m_indicesAndBitfields.

Oops, will fix.

> > Source/WebCore/dom/NodeRareData.h:233
> > +struct NodeMicroDataTokenLists {
> 
> Can we place these structs inside the NodeRareData definition? No one should ever call ::create() on them outside of here.

Good idea.

[Ryosuke Niwa]

Created attachment 178139 [details]
Moved new classes into NodeRareData

[Hajime Morrita]

Another idea is to let a set of sub-raredata polymorphic and store it to a list.
For non speed critical thing, it will work well.

[Andreas Kling]

Comment on attachment 178139 [details]
Moved new classes into NodeRareData

View in context: https://bugs.webkit.org/attachment.cgi?id=178139&action=review

Great, r=me.

> Source/WebCore/dom/NodeRareData.h:64
> +        if (m_childNodeList)
> +            return m_childNodeList;
> +        RefPtr<ChildNodeList> list = ChildNodeList::create(node);
> +        m_childNodeList = list.get();
> +        return list.release();

Would read slightly nicer as:

if (!m_childNodeList)
    m_childNodeList = ChildNodeList::create(node);
return m_childNodeList;

[Ryosuke Niwa]

(In reply to comment #12)
> (From update of attachment 178139 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178139&action=review
> 
> Great, r=me.
> 
> > Source/WebCore/dom/NodeRareData.h:64
> > +        if (m_childNodeList)
> > +            return m_childNodeList;
> > +        RefPtr<ChildNodeList> list = ChildNodeList::create(node);
> > +        m_childNodeList = list.get();
> > +        return list.release();
> 
> Would read slightly nicer as:
> 
> if (!m_childNodeList)
>     m_childNodeList = ChildNodeList::create(node);
> return m_childNodeList;

We can’t do that because m_childNodeList is a raw pointer.

[Ryosuke Niwa]

Committed r137003: <http://trac.webkit.org/changeset/137003>

[Xan Lopez]

This seems to be causing:

In file included from ../../Source/WebCore/dom/ChildNodeList.cpp:27:0:
../../Source/WebCore/dom/NodeRareData.h:230:23: error: ‘MutationObserverRegistration’ was not declared in this scope
../../Source/WebCore/dom/NodeRareData.h:230:51: error: template argument 1 is invalid
../../Source/WebCore/dom/NodeRareData.h:230:53: error: template argument 1 is invalid
../../Source/WebCore/dom/NodeRareData.h:231:17: error: ‘MutationObserverRegistration’ was not declared in this scope
../../Source/WebCore/dom/NodeRareData.h:231:46: error: template argument 1 is invalid
../../Source/WebCore/dom/NodeRareData.h:231:46: error: template argument 2 is invalid
../../Source/WebCore/dom/NodeRareData.h:231:46: error: template argument 3 is invalid
make[1]: *** [Source/WebCore/dom/libWebCore_la-ChildNodeList.lo] Error 1


in my laptop. The bots are fine, so perhaps this is GCC version dependent? (4.6.3 here)

[Ryosuke Niwa]

(In reply to comment #15)
> This seems to be causing:
> 
> In file included from ../../Source/WebCore/dom/ChildNodeList.cpp:27:0:
> ../../Source/WebCore/dom/NodeRareData.h:230:23: error: ‘MutationObserverRegistration’ was not declared in this scope
> ../../Source/WebCore/dom/NodeRareData.h:230:51: error: template argument 1 is invalid
> ../../Source/WebCore/dom/NodeRareData.h:230:53: error: template argument 1 is invalid
> ../../Source/WebCore/dom/NodeRareData.h:231:17: error: ‘MutationObserverRegistration’ was not declared in this scope
> ../../Source/WebCore/dom/NodeRareData.h:231:46: error: template argument 1 is invalid
> ../../Source/WebCore/dom/NodeRareData.h:231:46: error: template argument 2 is invalid
> ../../Source/WebCore/dom/NodeRareData.h:231:46: error: template argument 3 is invalid
> make[1]: *** [Source/WebCore/dom/libWebCore_la-ChildNodeList.lo] Error 1
> 
> 
> in my laptop. The bots are fine, so perhaps this is GCC version dependent? (4.6.3 here)

Oops, sorry, it’s missing if-def for ENABLE(MUTATION_OBSERVERS). Let me fix that.

[Ryosuke Niwa]

Fixed in http://trac.webkit.org/changeset/137024.

[Xan Lopez]

(In reply to comment #16)
> Oops, sorry, it’s missing if-def for ENABLE(MUTATION_OBSERVERS). Let me fix that.

Yeah, was about to commit that. There was a similar mistake for a Microdata-only section, so I just fixed that too.