Changeset 133372 in webkit

Timestamp:

Nov 2, 2012 4:39:42 PM (11 years ago)

Author:

commit-queue@webkit.org

Message:

Replace NodeRareData hash map with a union on m_renderer
​https://bugs.webkit.org/show_bug.cgi?id=100057

Patch by Elliott Sprehn <​esprehn@chromium.org> on 2012-11-02
Reviewed by Eric Seidel.

Use a union on Node::m_renderer between NodeRareData* and RenderObject*. This removes
the overhead of accessing rare data and the memory from the map.

This is an 8% improvement on Bindings/get-elements-by-tag-name.html which tested
document.getElementsByTagName and was previously optimized in Bug 90059 for a 5%
improvement. As this is better than even the special casing for document that was
done in that bug, general node list access should see an even greater win.

This reduces the memory usage on nytimes.com by 250k per Bug 101052 by
removing the rare data map overhead.

This is also a 15% improvement on Parser/textarea-parsing.html

By removing the performance overhead of rareData() this patch addresses the performance
issues raised in Bugs 73853, 87034 and 89635.

I ran Parser/html5-full-render.html and there was no performance regression after
tuning Text::recalcTextStyle and the refactor that was done in r132684.

No new tests, this is just a refactor.

• dom/Document.cpp:

(WebCore::Document::Document):

• dom/Document.h:

(WebCore::Node::Node):

• dom/Element.cpp:

(WebCore::Element::elementRareData):

• dom/Node.cpp:

(WebCore::Node::rareData):
(WebCore::Node::ensureRareData):
(WebCore::Node::clearRareData):
(WebCore::Node::renderBox):
(WebCore::Node::renderBoxModelObject):
(WebCore::Node::reportMemoryUsage):

• dom/Node.h:

(NodeRareDataBase):

Base class for NodeRareData that knows about the renderer so we can
inline the accesses in Node.h

(WebCore::NodeRareDataBase::renderer):
(WebCore::NodeRareDataBase::setRenderer):
(WebCore::NodeRareDataBase::~NodeRareDataBase):
(WebCore::NodeRareDataBase::NodeRareDataBase):
(WebCore):
(WebCore::Node::renderer):
(WebCore::Node::setRenderer):
(Node):

• dom/NodeRareData.h:
• dom/NodeRenderStyle.h:

(WebCore::Node::renderStyle):

• dom/Text.cpp:

(WebCore::Text::recalcTextStyle):

This method appears very hot in html5-full-render.html and accessing the
renderer 4 times caused a 2% performance regression with this patch. I
reduced it to 1 access and there's no longer any performance regression.

• dom/WebCoreMemoryInstrumentation.cpp: Removed tracking of the rare data map memory usage as there is no longer a map to track.
• dom/WebCoreMemoryInstrumentation.h:
• inspector/InspectorMemoryAgent.cpp:

(WebCore::InspectorMemoryAgent::getProcessMemoryDistribution):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• dom/Document.cpp (modified) (2 diffs)
• dom/Document.h (modified) (4 diffs)
• dom/Element.cpp (modified) (1 diff)
• dom/Node.cpp (modified) (5 diffs)
• dom/Node.h (modified) (3 diffs)
• dom/NodeRareData.h (modified) (2 diffs)
• dom/NodeRenderStyle.h (modified) (1 diff)
• dom/Text.cpp (modified) (1 diff)
• dom/WebCoreMemoryInstrumentation.cpp (modified) (2 diffs)
• dom/WebCoreMemoryInstrumentation.h (modified) (1 diff)
• inspector/InspectorMemoryAgent.cpp (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2012-11-02  Elliott Sprehn  <esprehn@chromium.org>
+
+        Replace NodeRareData hash map with a union on m_renderer
+        https://bugs.webkit.org/show_bug.cgi?id=100057
+
+        Reviewed by Eric Seidel.
+
+        Use a union on Node::m_renderer between NodeRareData* and RenderObject*. This removes
+        the overhead of accessing rare data and the memory from the map.
+
+        This is an 8% improvement on Bindings/get-elements-by-tag-name.html which tested
+        document.getElementsByTagName and was previously optimized in Bug 90059 for a 5% 
+        improvement. As this is better than even the special casing for document that was
+        done in that bug, general node list access should see an even greater win.
+
+        This reduces the memory usage on nytimes.com by 250k per Bug 101052 by
+        removing the rare data map overhead.
+
+        This is also a 15% improvement on Parser/textarea-parsing.html
+
+        By removing the performance overhead of rareData() this patch addresses the performance
+        issues raised in Bugs 73853, 87034 and 89635.
+
+        I ran Parser/html5-full-render.html and there was no performance regression after
+        tuning Text::recalcTextStyle and the refactor that was done in r132684.
+
+        No new tests, this is just a refactor.
+
+        * dom/Document.cpp:
+        (WebCore::Document::Document):
+        * dom/Document.h:
+        (WebCore::Node::Node):
+        * dom/Element.cpp:
+        (WebCore::Element::elementRareData):
+        * dom/Node.cpp:
+        (WebCore::Node::rareData):
+        (WebCore::Node::ensureRareData):
+        (WebCore::Node::clearRareData):
+        (WebCore::Node::renderBox):
+        (WebCore::Node::renderBoxModelObject):
+        (WebCore::Node::reportMemoryUsage):
+        * dom/Node.h:
+        (NodeRareDataBase):
+          Base class for NodeRareData that knows about the renderer so we can
+          inline the accesses in Node.h
+        (WebCore::NodeRareDataBase::renderer):
+        (WebCore::NodeRareDataBase::setRenderer):
+        (WebCore::NodeRareDataBase::~NodeRareDataBase):
+        (WebCore::NodeRareDataBase::NodeRareDataBase):
+        (WebCore):
+        (WebCore::Node::renderer):
+        (WebCore::Node::setRenderer):
+        (Node):
+        * dom/NodeRareData.h:
+        * dom/NodeRenderStyle.h:
+        (WebCore::Node::renderStyle):
+        * dom/Text.cpp:
+        (WebCore::Text::recalcTextStyle):
+          This method appears very hot in html5-full-render.html and accessing the
+          renderer 4 times caused a 2% performance regression with this patch. I
+          reduced it to 1 access and there's no longer any performance regression.
+        * dom/WebCoreMemoryInstrumentation.cpp:
+          Removed tracking of the rare data map memory usage as there is no longer
+          a map to track.
+        * dom/WebCoreMemoryInstrumentation.h:
+        * inspector/InspectorMemoryAgent.cpp:
+        (WebCore::InspectorMemoryAgent::getProcessMemoryDistribution):
+
 2012-11-02  Alexey Proskuryakov  <ap@apple.com>
 

trunk/Source/WebCore/dom/Document.cpp

@@ -479 +479 @@
     , m_sawElementsInKnownNamespaces(false)
     , m_isSrcdocDocument(false)
-    , m_documentRareData(0)
     , m_eventQueue(DocumentEventQueue::create(this))
     , m_weakReference(DocumentWeakReference::create(this))
@@ -2025 +2024 @@
     marginBottom = style->marginBottom().isAuto() ? marginBottom : intValueForLength(style->marginBottom(), width, view);
     marginLeft = style->marginLeft().isAuto() ? marginLeft : intValueForLength(style->marginLeft(), width, view);
-}
-
-void Document::setDocumentRareData(NodeRareData* rareData)
-{
-    m_documentRareData = rareData;
 }
 

trunk/Source/WebCore/dom/Document.h

@@ -117 +117 @@
 class NodeFilter;
 class NodeIterator;
-class NodeRareData;
 class Page;
 class PlatformMouseEvent;
@@ -467 +466 @@
     bool isSrcdocDocument() const { return m_isSrcdocDocument; }
 
-    NodeRareData* documentRareData() const { return m_documentRareData; };
-    void setDocumentRareData(NodeRareData*);
-
     StyleResolver* styleResolverIfExists() const { return m_styleResolver.get(); }
 
@@ -1430 +1426 @@
     bool m_isSrcdocDocument;
 
-    NodeRareData* m_documentRareData;
-
     RefPtr<DocumentEventQueue> m_eventQueue;
 
@@ -1527 +1521 @@
     , m_previous(0)
     , m_next(0)
-    , m_renderer(0)
 {
     if (document)

trunk/Source/WebCore/dom/Element.cpp

@@ -155 +155 @@
 {
     ASSERT(hasRareData());
-    return static_cast<ElementRareData*>(NodeRareData::rareDataFromMap(this));
-}
-    
+    return static_cast<ElementRareData*>(rareData());
+}
+
 inline ElementRareData* Element::ensureElementRareData()
 {

trunk/Source/WebCore/dom/Node.cpp

@@ -471 +471 @@
 {
     ASSERT(hasRareData());
-    NodeRareData* data = isDocumentNode() ? static_cast<const Document*>(this)->documentRareData() : NodeRareData::rareDataFromMap(this);
-    ASSERT(data);
-    return data;
+    return static_cast<NodeRareData*>(m_data.m_rareData);
 }
 
@@ -482 +480 @@
 
     NodeRareData* data = createRareData().leakPtr();
-    if (isDocumentNode()) {
-        // Fast path for a Document. A Document knows a pointer to NodeRareData.
-        ASSERT(!static_cast<Document*>(this)->documentRareData());
-        static_cast<Document*>(this)->setDocumentRareData(data);
-    } else {
-        ASSERT(!NodeRareData::rareDataMap().contains(this));
-        NodeRareData::rareDataMap().set(this, data);
-    }
+    ASSERT(data);
+    data->setRenderer(m_data.m_renderer);
+    m_data.m_rareData = data;
     setFlag(HasRareDataFlag);
     return data;
 }
-    
+
 OwnPtr<NodeRareData> Node::createRareData()
 {
@@ -507 +500 @@
 #endif
 
-    if (isDocumentNode()) {
-        Document* document = static_cast<Document*>(this);
-        NodeRareData* data = document->documentRareData();
-        ASSERT(data);
-        delete data;
-        document->setDocumentRareData(0);
-    } else {
-        NodeRareData::NodeRareDataMap& dataMap = NodeRareData::rareDataMap();
-        NodeRareData::NodeRareDataMap::iterator it = dataMap.find(this);
-        ASSERT(it != dataMap.end());
-        delete it->value;
-        dataMap.remove(it);
-    }
+    RenderObject* renderer = m_data.m_rareData->renderer();
+    delete m_data.m_rareData;
+    m_data.m_renderer = renderer;
     clearFlag(HasRareDataFlag);
 }
@@ -799 +782 @@
 RenderBox* Node::renderBox() const
 {
-    return m_renderer && m_renderer->isBox() ? toRenderBox(m_renderer) : 0;
+    RenderObject* renderer = this->renderer();
+    return renderer && renderer->isBox() ? toRenderBox(renderer) : 0;
 }
 
 RenderBoxModelObject* Node::renderBoxModelObject() const
 {
-    return m_renderer && m_renderer->isBoxModelObject() ? toRenderBoxModelObject(m_renderer) : 0;
+    RenderObject* renderer = this->renderer();
+    return renderer && renderer->isBoxModelObject() ? toRenderBoxModelObject(renderer) : 0;
 }
 
@@ -2831 +2816 @@
     info.addMember(m_next);
     info.addMember(m_previous);
-    if (m_renderer)
-        info.addMember(m_renderer->style());
+    if (RenderObject* renderer = this->renderer())
+        info.addMember(renderer->style());
     if (hasRareData())
         info.addMember(rareData());

trunk/Source/WebCore/dom/Node.h

@@ -112 +112 @@
 };
 
+class NodeRareDataBase {
+public:
+    RenderObject* renderer() const { return m_renderer; }
+    void setRenderer(RenderObject* renderer) { m_renderer = renderer; }
+    virtual ~NodeRareDataBase() { }
+protected:
+    NodeRareDataBase() { }
+private:
+    RenderObject* m_renderer;
+};
+
 class Node : public EventTarget, public ScriptWrappable, public TreeShared<Node, ContainerNode> {
     friend class Document;
@@ -500 +511 @@
     // Integration with rendering tree
 
-    RenderObject* renderer() const { return m_renderer; }
-    void setRenderer(RenderObject* renderer) { m_renderer = renderer; }
-    
+    // As renderer() includes a branch you should avoid calling it repeatedly in hot code paths.
+    RenderObject* renderer() const { return hasRareData() ? m_data.m_rareData->renderer() : m_data.m_renderer; };
+    void setRenderer(RenderObject* renderer)
+    {
+        if (hasRareData())
+            m_data.m_rareData->setRenderer(renderer);
+        else
+            m_data.m_renderer = renderer;
+    }
+
     // Use these two methods with caution.
     RenderBox* renderBox() const;
@@ -810 +828 @@
     Node* m_previous;
     Node* m_next;
-    RenderObject* m_renderer;
+    // When a node has rare data we move the renderer into the rare data.
+    union DataUnion {
+        DataUnion() : m_renderer(0) { }
+        RenderObject* m_renderer;
+        NodeRareDataBase* m_rareData;
+    } m_data;
 
 protected:

trunk/Source/WebCore/dom/NodeRareData.h

@@ -179 +179 @@
 };
 
-class NodeRareData {
+class NodeRareData : public NodeRareDataBase {
     WTF_MAKE_NONCOPYABLE(NodeRareData); WTF_MAKE_FAST_ALLOCATED;
 public:    
@@ -201 +201 @@
     }
 
-    typedef HashMap<const Node*, NodeRareData*> NodeRareDataMap;
-    
-    static NodeRareDataMap& rareDataMap()
-    {
-        static NodeRareDataMap* dataMap = new NodeRareDataMap;
-        return *dataMap;
-    }
-    
-    static NodeRareData* rareDataFromMap(const Node* node)
-    {
-        return rareDataMap().get(node);
-    }
-
     TreeScope* treeScope() const { return m_treeScope; }
     void setTreeScope(TreeScope* treeScope) { m_treeScope = treeScope; }

trunk/Source/WebCore/dom/NodeRenderStyle.h

@@ -36 +36 @@
     // Using a ternary here confuses the Solaris Studio 12/12.1/12.2 compilers:
     // Bug is CR 6569194, "Problem with question operator binding in inline function"
-    if (m_renderer)
-        return m_renderer->style();
+    if (RenderObject* renderer = this->renderer())
+        return renderer->style();
     return nonRendererStyle();
 }

trunk/Source/WebCore/dom/Text.cpp

@@ -257 +257 @@
         willRecalcTextStyle(change);
 
-    if (change != NoChange && parentNode() && parentNode()->renderer()) {
-        if (renderer())
-            renderer()->setStyle(parentNode()->renderer()->style());
-    }
+    RenderObject* renderer = this->renderer();
+
+    // The only time we have a renderer and our parent doesn't is if our parent
+    // is a shadow root.
+    if (change != NoChange && renderer && !parentNode()->isShadowRoot())
+        renderer->setStyle(parentNode()->renderer()->style());
+
     if (needsStyleRecalc()) {
-        if (renderer()) {
-            if (renderer()->isText())
-                toRenderText(renderer())->setText(dataImpl());
+        if (renderer) {
+            if (renderer->isText())
+                toRenderText(renderer)->setText(dataImpl());
         } else
             reattach();

trunk/Source/WebCore/dom/WebCoreMemoryInstrumentation.cpp

@@ -31 +31 @@
 #include "config.h"
 #include "WebCoreMemoryInstrumentation.h"
-
-#include "Node.h"
-#include "NodeRareData.h"
-#include <wtf/MemoryInstrumentationHashMap.h>
 
 namespace WebCore {
@@ -77 +73 @@
 MemoryObjectType WebCoreMemoryTypes::ProcessPrivateMemory = "ProcessPrivateMemory";
 
-void WebCoreMemoryInstrumentation::reportMemoryUsage(MemoryInstrumentation* memoryInstrumentation)
-{
-    memoryInstrumentation->addRootObject(NodeRareData::rareDataMap(), WebCoreMemoryTypes::DOM);
-}
-
 } // namespace WebCore

trunk/Source/WebCore/dom/WebCoreMemoryInstrumentation.h

@@ -82 +82 @@
 };
 
-class WebCoreMemoryInstrumentation {
-public:
-    static void reportMemoryUsage(MemoryInstrumentation*);
-};
-
 } // namespace WebCore
 

trunk/Source/WebCore/inspector/InspectorMemoryAgent.cpp

@@ -537 +537 @@
     RefPtr<InspectorMemoryBlocks> children = InspectorMemoryBlocks::create();
     addPlatformComponentsInfo(children);
-    WebCoreMemoryInstrumentation::reportMemoryUsage(&memoryInstrumentation);
 
     memoryInstrumentation.addRootObject(this);