Split off uncommonly used data from Node similar to ElementRareData

• Component: DOM
• Status: RESOLVED FIXED

[David Smith]

There are a number of fairly hot virtual functions on Node that could be made non-virtual if there was a free bit to put a flag in. Darin mentioned that he was considering making a NodeRareData, so I'm investigating doing that.

[David Smith]

Created attachment 23636 [details]
Draft patch

Current status is: passes run-webkit-tests, appears to be around a 1.2% regression on slickspeed, haven't tested anything else yet. Next up is cleanup and instrumenting the various bits on Node to see which ones are rare enough to move into RareData, then I'll use a newly free bit or two to get rid of perf regressions and it should be ready for review then.

[David Smith]

Created attachment 23637 [details]
Correct draft patch

Forgot to svn add NodeRareData.h

[Darin Adler]

Comment on attachment 23637 [details]
Correct draft patch

This looks really good!

+    virtual NodeRareData* createRareData();

The createRareData function itself shouldn't be virtual. Instead only the function that actually calls new should be a virtual function. That can be named something like newRareData().

+#ifndef NodeRareData_h

This file needs a copyright and license header.

I also think you're including too many headers in the file. Include only what's absolutely required.

We might want to consider having the RareData just be a struct or a class with public data rather than a class with accessor functions. The accessor functions could all be back in Node.

If we arrange things carefully, NodeListsNodeData doesn't have to be in the NodeRareData header. We might not be able to inline the access to it.

+        NodeRareData()
+        : m_nodeLists(0)
+        , m_tabIndex(0)
+        , m_tabIndexSetExplicitly(false)
+        {
+            
+        }

We normally indent the : and commas. There's no need to initialize an OwnPtr to 0 -- smart pointers initialize themselves. There shouldn't be a space between the brackets.

+        void setNodeLists(NodeListsNodeData * lists) { m_nodeLists.set(lists); }

No space before the *. The argument to setNodeLists should probably be an auto_ptr.

+        NodeListsNodeData *nodeLists() const { return m_nodeLists ? m_nodeLists.get() : 0; }

The * should be next to the type name. There's no need for the ternary operator here. The get() function already returns 0. for that case.

+        void setTabIndex(short idx) { m_tabIndex = idx; m_tabIndexSetExplicitly = true; }

Should say index here -- no need to abbreviate to idx.

+    if(hasRareData())

Missing space here. I think this would read nicer with the ternary operator.

+    NodeRareData *rd = createRareData();

The * needs to go next to the type, not the variable name. I suggest "data" as the name rather than "rd".

-    ASSERT(m_nodeLists);
+    ASSERT(rareData());

Should assert rareData()->nodeLists() too.

     // Perform error checking as required by spec for setting Node.prefix. Used by
-    // Element::setPrefix() and Attr::setPrefix()
+    // Node::setPrefix() and Attr::setPrefix()

Is this change correct?

-    ElementRareData(Element*);
+    ElementRareData(Node *);

This should not change. It should still take an Element.

-inline ElementRareData::ElementRareData(Element* element)
-    : m_minimumSizeForResizing(defaultMinimumSizeForResizing())
-    , m_computedStyle(0)
-    , m_needsFocusAppearanceUpdateSoonAfterAttach(false)
+    
+inline ElementRareData::ElementRareData(Node *node)
+: m_minimumSizeForResizing(defaultMinimumSizeForResizing())
+, m_computedStyle(0)
+, m_needsFocusAppearanceUpdateSoonAfterAttach(false)

None of this should be changing.

-inline ElementRareData* Element::rareData()
+inline ElementRareData* Element::elementRareData()

You shouldn't change the name to elementRareData(). Instead you should have this have the same name, but a different return type, than the rareData() function in the base class. That way, you can just call rareData() and get the object of the right type. You'll see -- it'll work well.

-    return hasRareData() ? rareDataFromMap(this) : 0;
+    return hasRareData() ? static_cast<ElementRareData*>(NodeRareData::rareDataFromMap(this)) : 0;

The implementation should just be this instead:

    inline ElementRareData* Element::rareData()
    {
        return static_cast<ElementRareData*>(Node::rareData());
    }

Also, this needs to be in a header file now that it's protected rather than private. It might be used by derived classes. I think the inline function definitions should go in ElementRareData.h.

The createRareData function should be inlined and non-virtual and should be overridden much the way I suggest overriding rareData() above.

Most of the changes in Element.cpp shouldn't be necessary if you follow that plan.

+    void setHasRareData(bool b = true) { m_hasRareData = b; }

That function should be private or non-existent.

Good luck. I can't wait to see another draft of this patch!

[Darin Adler]

(In reply to comment #1)
> appears to be around a 1.2% regression on slickspeed

Darn!

[David Smith]

Created attachment 23686 [details]
Improved patch

This addresses a lot of the comments above, as well as making a few other changes:

* createRareData was changed to ensureRareData, since that's what it actually does. The createRareData name is now used for the virtual function that creates the correct type of RareData.

* I couldn't figure out how to implement rareData() for Element in terms of Node's rareData(); it failed to link due to rareData() being inline in the cpp file. I tried moving it to the .h file, but that required including NodeRareData.h in Node.h, and that in turn required making NodeRareData.h a public header (since Node.h is used in WebKit). That seemed less than ideal.

* I also wasn't sure how to move NodeListsNodeData out of NodeRareData.h, since it appears to be necessary for typedef HashMap<const Node*, NodeRareData*> NodeRareDataMap; to work.

* I changed the hasRareData() check in rareData() to an ASSERT, since most callsites were already checking it anyway.

* I left the accessors in NodeRareData rather than just making things public, since it enforces updating m_tabIndexSetExplicitly when changing m_tabIndex

One nice thing is that due to removing redundant checks and being more careful to use hasRareData() instead of checking for rareData() being non-null, this appears to remove most/all of the performance downside. A few things got faster (non-queryselectorall slickspeed, xpath getelementsbyclassname), a few got slower (js getelementsbyclassname, queryselectorall slickspeed). I suspect that's mostly due to inaccuracies in the tests I'm using, although I suppose it could be due to Node being smaller now.

[David Smith]

Created attachment 23695 [details]
Make things go zoom

This moves another bit (focus) into rareData and uses the space to add an isContainer flag. This allows firstChild(), lastChild(), childNodeCount(), and childNode() to be converted from virtual to inline for a nice speedup:

0.5% on prototype slickspeed
11.7% on jquery slickspeed
5.1% on ext slickspeed
7.9% on qsa slickspeed

Presumably other things are sped up as well, since a lot of stuff uses these directly or indirectly.

[David Smith]

Created attachment 23700 [details]
Now with ChangeLog and a few cleanups

I did some instrumenting of Node and browsed around for a while, the only site that has a ton of rareData on it is digg.com, which appears to call childNodes on *almost every node on the entire site*. Even the ones that can't possibly have children, like CharacterData. However, even with that I couldn't get it to show up as a measurable regression in the pageload testing I did on digg.

This should probably be tested on the internal PLT.

[Darin Adler]

Comment on attachment 23700 [details]
Now with ChangeLog and a few cleanups

Great work!

 #include "Node.h"
+#include "ContainerNode.h"
 
We don't need both. The Node.h include can be replaced by the ContainerNode one. Same applies in the other places you made this change

-ContainerNode::ContainerNode(Document* doc, bool isElement)
-    : EventTargetNode(doc, isElement)
+ContainerNode::ContainerNode(Document* doc, bool isElement, bool isContainer)
+    : EventTargetNode(doc, isElement, isContainer)

+    ContainerNode(Document*, bool isElement = false, bool isContainer = true);

There's no need for the constructor of ContainerNode to take an isContainer argument. Just pass true!

+    if (hasRareData())
+    {   

Brace goes on the same line as the if in the Safari coding style.

 class CSSStyleDeclaration;
-class ElementRareData;
 class IntSize;
+class ElementRareData;

Please put this back into alphabetical order.

+    virtual NodeRareData* createRareData();

Please make this private instead of protected. There's no need for Element or any class derived from it to call this function.

+#ifndef ElementRareData_h

This file needs a header.

+#include "NodeRareData.h"
+#include "Element.h"

Sort includes alphabetically.

+static inline IntSize defaultMinimumSizeForResizing()
+{
+    return IntSize(INT_MAX, INT_MAX);
+}

Functions in headers should not be marked "static" because that gives internal linkage. Just make this inline.

+void ElementRareData::resetComputedStyle(Element* element)
+{
+    if (!m_computedStyle)
+        return;
+    m_computedStyle->deref(element->document()->renderArena());
+    m_computedStyle = 0;
+}

This function either needs to be marked inline or moved to a .cpp file.

+    void setTabIndex(short index) { m_tabIndex = index; m_tabIndexSetExplicitly = true; }

I think this should be named setTabIndexExplicitly.

+void Node::setFocus(bool b)
+{ 
+    ensureRareData()->m_focused = b;
+}

Maybe this have a short-circuit case if both hasRareData() and b are false?

+    if(hasRareData())

Needs a space after "if". Also, probably easier to read if written with &&.

-    virtual bool childTypeAllowed(NodeType) { return false; }
+    virtual bool childTypeAllowed(NodeType t) { return false; }

Argument name "t" should be omitted here. This should not have been changed.

+    virtual NodeRareData* createRareData();

This function should be private.

+    NodeRareData* rareData();
+    const NodeRareData* rareData() const;

One of these should be an inline that calls the other and does const_cast.

+#ifndef NodeRareData_h

This file needs a header.

+#include "StringHash.h"
+#include <wtf/HashSet.h>
+#include <wtf/OwnPtr.h>
+#include "DynamicNodeList.h"

Sort includes alphabetically.

review- because there are a lot of suggested changes, but this is looking good and getting close to ready to land.

[David Smith]

Created attachment 23828 [details]
Addresses review comments

[David Smith]

Created attachment 23829 [details]
Oops

That was an old version of the patch before. Infinite-recursed in rareData().

[Darin Adler]

Comment on attachment 23829 [details]
Oops

+inline Node *Node::containerChildNode(unsigned index) const

Need to move the * to the left so it's next to the return value type name.

In Node.cpp:

-
+    
 // --------

This is showing up because you added spaces on a blank line.

+    if(b || hasRareData())

Need a space after the if before the parenthesis.

+    NodeRareData *data = ensureRareData();

+    NodeRareData *data = ensureRareData();

Need to move the * to the left so it's by the type name.

r=me

[David Smith]

Committed in r36923