Make elements with attributes smaller by eliminating the m_element back pointer in...
authorcaio.oliveira@openbossa.org <caio.oliveira@openbossa.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 8 Mar 2012 17:42:28 +0000 (17:42 +0000)
committercaio.oliveira@openbossa.org <caio.oliveira@openbossa.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 8 Mar 2012 17:42:28 +0000 (17:42 +0000)
https://bugs.webkit.org/show_bug.cgi?id=75069

Reviewed by Ryosuke Niwa.

Source/WebCore:

NamedNodeMap is an exposed DOM representation of an element's attribute storage. As part of
its implementation it keeps a pointer to its associated Element plus all the attribute
storage.

This commit separate the two things: NamedNodeMap is now a wrapper to Element, containing
only the pointer, and the attribute storage is now owned by Element directly. Since usage
of NamedNodeMap is not very common, it can be stored in ElementRareData. As a result, most
elements with attributes now don't need to allocate memory for that extra pointer in
NamedNodeMap.

One consequence of this implementation is that now we explicitly don't support
DocumentType.notations and DocumentType.entities. They weren't supported before, a
NamedNodeMap was never created for those attributes -- and some NamedNodeMap functions
wouldn't work correctly without an associated Element.

NamedNodeMap itself was cleaned up, as well as unnecessary references to it removed in the
code and comments.

No new tests and should not change results for existing tests.

* dom/Attribute.h:
(WebCore):
* dom/DocumentType.h:
(DocumentType): Point out that we don't support does attributes yet.
* dom/Element.cpp:
(WebCore::Element::~Element): Detaching the NamedNodeMap is no longer necessary because it
will be destroyed. We still detach the potential Attrs inside our Attributes by using
clearAttributes().
(WebCore::Element::attributes): Looks in ElementRareData now. Note we ensure the creation
of the attribute storage.
(WebCore):
(WebCore::Element::getAttribute):
(WebCore::Element::setAttributeInternal):
(WebCore::Element::parserSetAttributes):
(WebCore::Element::hasAttributes):
(WebCore::Element::createAttributeData):
(WebCore::Element::insertedIntoDocument):
(WebCore::Element::removedFromDocument):
(WebCore::Element::getURLAttribute):
(WebCore::Element::getNonEmptyURLAttribute):
(WebCore::Element::hasNamedNodeMap): Helper function for Node::dumpStatistics().
* dom/Element.h:
(Element):
(WebCore::Element::attributeData):
(WebCore::Element::ensureAttributeData):
(WebCore::Element::fastHasAttribute):
(WebCore::Element::fastGetAttribute):
(WebCore::Element::hasAttributesWithoutUpdate):
(WebCore::Element::idForStyleResolution):
(WebCore::Element::attributeCount):
(WebCore::Element::attributeItem):
(WebCore::Element::getAttributeItem):
* dom/ElementAttributeData.h:
(WebCore::ElementAttributeData::create):
(ElementAttributeData):
* dom/ElementRareData.h:
(ElementRareData):
* dom/NamedNodeMap.cpp: Rewriting now that m_attributeData is not a member, using m_element
methods when possible.
(WebCore::NamedNodeMap::ref):
(WebCore::NamedNodeMap::deref):
(WebCore::NamedNodeMap::getNamedItem):
(WebCore::NamedNodeMap::getNamedItemNS):
(WebCore::NamedNodeMap::removeNamedItem):
(WebCore::NamedNodeMap::removeNamedItemNS):
(WebCore::NamedNodeMap::setNamedItem):
(WebCore::NamedNodeMap::item):
(WebCore::NamedNodeMap::length):
* dom/NamedNodeMap.h:
(WebCore):
(WebCore::NamedNodeMap::create):
(NamedNodeMap):
(WebCore::NamedNodeMap::NamedNodeMap): Instead of asserting m_element in every function, we
now assert only in the constructor.
* dom/Node.cpp:
(WebCore::Node::dumpStatistics): Add a counter for elements with rare data, this allows us
compare more clearly the impact of moving NamedNodeMap there.
(WebCore::Node::isEqualNode): Remove use of mapsEquivalent(). It was dead code, because
both entities and notations were always NULL.
(WebCore::Node::compareDocumentPosition):
* inspector/DOMPatchSupport.h:
(WebCore):
* svg/SVGElement.cpp:
(WebCore::SVGElement::attributeChanged):

Source/WebKit/chromium:

* src/WebElement.cpp: Include NamedNodeMap.h since Element.h doesn't include it anymore.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@110172 268f45cc-cd09-0410-ab3c-d52691b4dbfc

14 files changed:
Source/WebCore/ChangeLog
Source/WebCore/dom/Attribute.h
Source/WebCore/dom/DocumentType.h
Source/WebCore/dom/Element.cpp
Source/WebCore/dom/Element.h
Source/WebCore/dom/ElementAttributeData.h
Source/WebCore/dom/ElementRareData.h
Source/WebCore/dom/NamedNodeMap.cpp
Source/WebCore/dom/NamedNodeMap.h
Source/WebCore/dom/Node.cpp
Source/WebCore/inspector/DOMPatchSupport.h
Source/WebCore/svg/SVGElement.cpp
Source/WebKit/chromium/ChangeLog
Source/WebKit/chromium/src/WebElement.cpp

index aa9f9fd..c58313f 100644 (file)
@@ -1,3 +1,95 @@
+2012-03-08  Caio Marcelo de Oliveira Filho  <caio.oliveira@openbossa.org>
+
+        Make elements with attributes smaller by eliminating the m_element back pointer in NamedNodeMap
+        https://bugs.webkit.org/show_bug.cgi?id=75069
+
+        Reviewed by Ryosuke Niwa.
+
+        NamedNodeMap is an exposed DOM representation of an element's attribute storage. As part of
+        its implementation it keeps a pointer to its associated Element plus all the attribute
+        storage.
+
+        This commit separate the two things: NamedNodeMap is now a wrapper to Element, containing
+        only the pointer, and the attribute storage is now owned by Element directly. Since usage
+        of NamedNodeMap is not very common, it can be stored in ElementRareData. As a result, most
+        elements with attributes now don't need to allocate memory for that extra pointer in
+        NamedNodeMap.
+
+        One consequence of this implementation is that now we explicitly don't support
+        DocumentType.notations and DocumentType.entities. They weren't supported before, a
+        NamedNodeMap was never created for those attributes -- and some NamedNodeMap functions
+        wouldn't work correctly without an associated Element.
+
+        NamedNodeMap itself was cleaned up, as well as unnecessary references to it removed in the
+        code and comments.
+
+        No new tests and should not change results for existing tests.
+
+        * dom/Attribute.h:
+        (WebCore):
+        * dom/DocumentType.h:
+        (DocumentType): Point out that we don't support does attributes yet.
+        * dom/Element.cpp:
+        (WebCore::Element::~Element): Detaching the NamedNodeMap is no longer necessary because it
+        will be destroyed. We still detach the potential Attrs inside our Attributes by using
+        clearAttributes().
+        (WebCore::Element::attributes): Looks in ElementRareData now. Note we ensure the creation
+        of the attribute storage.
+        (WebCore):
+        (WebCore::Element::getAttribute):
+        (WebCore::Element::setAttributeInternal):
+        (WebCore::Element::parserSetAttributes):
+        (WebCore::Element::hasAttributes):
+        (WebCore::Element::createAttributeData):
+        (WebCore::Element::insertedIntoDocument):
+        (WebCore::Element::removedFromDocument):
+        (WebCore::Element::getURLAttribute):
+        (WebCore::Element::getNonEmptyURLAttribute):
+        (WebCore::Element::hasNamedNodeMap): Helper function for Node::dumpStatistics().
+        * dom/Element.h:
+        (Element):
+        (WebCore::Element::attributeData):
+        (WebCore::Element::ensureAttributeData):
+        (WebCore::Element::fastHasAttribute):
+        (WebCore::Element::fastGetAttribute):
+        (WebCore::Element::hasAttributesWithoutUpdate):
+        (WebCore::Element::idForStyleResolution):
+        (WebCore::Element::attributeCount):
+        (WebCore::Element::attributeItem):
+        (WebCore::Element::getAttributeItem):
+        * dom/ElementAttributeData.h:
+        (WebCore::ElementAttributeData::create):
+        (ElementAttributeData):
+        * dom/ElementRareData.h:
+        (ElementRareData):
+        * dom/NamedNodeMap.cpp: Rewriting now that m_attributeData is not a member, using m_element
+        methods when possible.
+        (WebCore::NamedNodeMap::ref):
+        (WebCore::NamedNodeMap::deref):
+        (WebCore::NamedNodeMap::getNamedItem):
+        (WebCore::NamedNodeMap::getNamedItemNS):
+        (WebCore::NamedNodeMap::removeNamedItem):
+        (WebCore::NamedNodeMap::removeNamedItemNS):
+        (WebCore::NamedNodeMap::setNamedItem):
+        (WebCore::NamedNodeMap::item):
+        (WebCore::NamedNodeMap::length):
+        * dom/NamedNodeMap.h:
+        (WebCore):
+        (WebCore::NamedNodeMap::create):
+        (NamedNodeMap):
+        (WebCore::NamedNodeMap::NamedNodeMap): Instead of asserting m_element in every function, we
+        now assert only in the constructor.
+        * dom/Node.cpp:
+        (WebCore::Node::dumpStatistics): Add a counter for elements with rare data, this allows us
+        compare more clearly the impact of moving NamedNodeMap there.
+        (WebCore::Node::isEqualNode): Remove use of mapsEquivalent(). It was dead code, because
+        both entities and notations were always NULL.
+        (WebCore::Node::compareDocumentPosition):
+        * inspector/DOMPatchSupport.h:
+        (WebCore):
+        * svg/SVGElement.cpp:
+        (WebCore::SVGElement::attributeChanged):
+
 2012-03-08  Robin Cao  <robin.cao@torchmobile.com.cn>
 
         [BlackBerry] Upstream WebGL related files from platform/graphics
index 8658775..8c59d1c 100644 (file)
@@ -31,7 +31,6 @@ namespace WebCore {
 
 class Attr;
 class Element;
-class NamedNodeMap;
 
 // This has no counterpart in DOM.
 // It is an internal representation of the node value of an Attr.
index 8fd11f0..fd6c891 100644 (file)
@@ -37,6 +37,7 @@ public:
         return adoptRef(new DocumentType(document, name, publicId, systemId));
     }
 
+    // FIXME: We never fill m_entities and m_notations. Current implementation of NamedNodeMap doesn't work without an associated Element yet.
     NamedNodeMap* entities() const { return m_entities.get(); }
     NamedNodeMap* notations() const { return m_notations.get(); }
 
index 615c1c5..3d5c5ce 100644 (file)
@@ -52,6 +52,7 @@
 #include "InspectorInstrumentation.h"
 #include "MutationObserverInterestGroup.h"
 #include "MutationRecord.h"
+#include "NamedNodeMap.h"
 #include "NodeList.h"
 #include "NodeRenderStyle.h"
 #include "NodeRenderingContext.h"
@@ -125,8 +126,8 @@ Element::~Element()
 {
     if (shadowTree())
         rareData()->m_shadowTree.clear();
-    if (m_attributeMap)
-        m_attributeMap->detachFromElement();
+    if (m_attributeData)
+        m_attributeData->clearAttributes();
 }
 
 inline ElementRareData* Element::rareData() const
@@ -199,6 +200,17 @@ void Element::setBooleanAttribute(const QualifiedName& name, bool value)
         removeAttribute(name);
 }
 
+NamedNodeMap* Element::attributes() const
+{
+    ensureUpdatedAttributeData();
+    ElementRareData* rareData = const_cast<Element*>(this)->ensureRareData();
+    if (NamedNodeMap* attributeMap = rareData->m_attributeMap.get())
+        return attributeMap;
+
+    rareData->m_attributeMap = NamedNodeMap::create(const_cast<Element*>(this));
+    return rareData->m_attributeMap.get();
+}
+
 Node::NodeType Element::nodeType() const
 {
     return ELEMENT_NODE;
@@ -219,7 +231,7 @@ const AtomicString& Element::getAttribute(const QualifiedName& name) const
         updateAnimatedSVGAttribute(name);
 #endif
 
-    if (m_attributeMap) {
+    if (m_attributeData) {
         if (Attribute* attribute = getAttributeItem(name))
             return attribute->value();
     }
@@ -596,8 +608,8 @@ const AtomicString& Element::getAttribute(const String& name) const
     }
 #endif
 
-    if (m_attributeMap) {
-        if (Attribute* attribute = m_attributeMap->attributeData()->getAttributeItem(name, ignoreCase))
+    if (m_attributeData) {
+        if (Attribute* attribute = m_attributeData->getAttributeItem(name, ignoreCase))
             return attribute->value();
     }
 
@@ -630,16 +642,15 @@ void Element::setAttribute(const QualifiedName& name, const AtomicString& value,
 
 inline void Element::setAttributeInternal(size_t index, const QualifiedName& name, const AtomicString& value, bool notifyChanged)
 {
-    ElementAttributeData* attributeData = &m_attributeMap->m_attributeData;
-    Attribute* old = index != notFound ? attributeData->attributeItem(index) : 0;
+    Attribute* old = index != notFound ? m_attributeData->attributeItem(index) : 0;
     if (value.isNull()) {
         if (old)
-            attributeData->removeAttribute(index, this);
+            m_attributeData->removeAttribute(index, this);
         return;
     }
 
     if (!old) {
-        attributeData->addAttribute(Attribute::create(name, value), this);
+        m_attributeData->addAttribute(Attribute::create(name, value), this);
         return;
     }
 
@@ -733,37 +744,35 @@ void Element::parserSetAttributes(PassOwnPtr<AttributeVector> attributeVector, F
 
     document()->incDOMTreeVersion();
 
-    ASSERT(!m_attributeMap);
+    ASSERT(!m_attributeData);
 
     if (!attributeVector)
         return;
 
-    m_attributeMap = NamedNodeMap::create(this);
-    ElementAttributeData* attributeData = m_attributeMap->attributeData();
-    attributeData->m_attributes.swap(*attributeVector);
+    createAttributeData();
+    m_attributeData->m_attributes.swap(*attributeVector);
 
-    m_attributeMap->m_element = this;
     // If the element is created as result of a paste or drag-n-drop operation
     // we want to remove all the script and event handlers.
     if (scriptingPermission == FragmentScriptingNotAllowed) {
         unsigned i = 0;
-        while (i < m_attributeMap->length()) {
-            const QualifiedName& attributeName = attributeData->m_attributes[i]->name();
+        while (i < m_attributeData->length()) {
+            const QualifiedName& attributeName = m_attributeData->m_attributes[i]->name();
             if (isEventHandlerAttribute(attributeName)) {
-                attributeData->m_attributes.remove(i);
+                m_attributeData->m_attributes.remove(i);
                 continue;
             }
 
-            if (isAttributeToRemove(attributeName, attributeData->m_attributes[i]->value()))
-                attributeData->m_attributes[i]->setValue(nullAtom);
+            if (isAttributeToRemove(attributeName, m_attributeData->m_attributes[i]->value()))
+                m_attributeData->m_attributes[i]->setValue(nullAtom);
             i++;
         }
     }
 
     // Store the set of attributes that changed on the stack in case
-    // attributeChanged mutates m_attributeMap.
+    // attributeChanged mutates m_attributeData.
     Vector<RefPtr<Attribute> > attributes;
-    attributeData->copyAttributesToVector(attributes);
+    m_attributeData->copyAttributesToVector(attributes);
     for (Vector<RefPtr<Attribute> >::iterator iter = attributes.begin(); iter != attributes.end(); ++iter)
         attributeChanged(iter->get());
 }
@@ -771,7 +780,7 @@ void Element::parserSetAttributes(PassOwnPtr<AttributeVector> attributeVector, F
 bool Element::hasAttributes() const
 {
     updateInvalidAttributes();
-    return m_attributeMap && m_attributeMap->length();
+    return m_attributeData && m_attributeData->length();
 }
 
 bool Element::hasEquivalentAttributes(const Element* other) const
@@ -823,9 +832,9 @@ KURL Element::baseURI() const
     return KURL(parentBase, baseAttribute);
 }
 
-void Element::createAttributeMap() const
+void Element::createAttributeData() const
 {
-    m_attributeMap = NamedNodeMap::create(const_cast<Element*>(this));
+    m_attributeData = ElementAttributeData::create();
 }
 
 bool Element::isURLAttribute(Attribute*) const
@@ -878,7 +887,7 @@ void Element::insertedIntoDocument()
     if (ShadowTree* tree = shadowTree())
         tree->insertedIntoDocument();
 
-    if (m_attributeMap) {
+    if (m_attributeData) {
         if (hasID()) {
             Attribute* idItem = getAttributeItem(document()->idAttributeName());
             if (idItem && !idItem->isNull())
@@ -894,7 +903,7 @@ void Element::insertedIntoDocument()
 
 void Element::removedFromDocument()
 {
-    if (m_attributeMap) {
+    if (m_attributeData) {
         if (hasID()) {
             Attribute* idItem = getAttributeItem(document()->idAttributeName());
             if (idItem && !idItem->isNull())
@@ -1789,7 +1798,7 @@ DOMStringMap* Element::dataset()
 KURL Element::getURLAttribute(const QualifiedName& name) const
 {
 #if !ASSERT_DISABLED
-    if (m_attributeMap) {
+    if (m_attributeData) {
         if (Attribute* attribute = getAttributeItem(name))
             ASSERT(isURLAttribute(attribute));
     }
@@ -1800,7 +1809,7 @@ KURL Element::getURLAttribute(const QualifiedName& name) const
 KURL Element::getNonEmptyURLAttribute(const QualifiedName& name) const
 {
 #if !ASSERT_DISABLED
-    if (m_attributeMap) {
+    if (m_attributeData) {
         if (Attribute* attribute = getAttributeItem(name))
             ASSERT(isURLAttribute(attribute));
     }
@@ -1968,6 +1977,13 @@ bool Element::fastAttributeLookupAllowed(const QualifiedName& name) const
 }
 #endif
 
+#ifdef DUMP_NODE_STATISTICS
+bool Element::hasNamedNodeMap() const
+{
+    return hasRareData() && rareData()->m_attributeMap;
+}
+#endif
+
 void Element::willModifyAttribute(const QualifiedName& name, const AtomicString& oldValue, const AtomicString& newValue)
 {
     document()->incDOMTreeVersion();
index 8eebb02..8c82507 100644 (file)
@@ -27,9 +27,9 @@
 
 #include "CollectionType.h"
 #include "Document.h"
+#include "ElementAttributeData.h"
 #include "FragmentScriptingPermission.h"
 #include "HTMLNames.h"
-#include "NamedNodeMap.h"
 #include "ScrollTypes.h"
 
 namespace WebCore {
@@ -130,6 +130,9 @@ public:
     bool fastAttributeLookupAllowed(const QualifiedName&) const;
 #endif
 
+#ifdef DUMP_NODE_STATISTICS
+    bool hasNamedNodeMap() const;
+#endif
     bool hasAttributes() const;
     // This variant will not update the potentially invalid attributes. To be used when not interested
     // in style attribute or one of the SVG animation attributes.
@@ -229,15 +232,13 @@ public:
     // For exposing to DOM only.
     NamedNodeMap* attributes() const;
 
-    NamedNodeMap* updatedAttributes() const;
-
     // This method is called whenever an attribute is added, changed or removed.
     virtual void attributeChanged(Attribute*);
 
     // Only called by the parser immediately after element construction.
     void parserSetAttributes(PassOwnPtr<AttributeVector>, FragmentScriptingPermission);
 
-    ElementAttributeData* attributeData() const { return m_attributeMap ? m_attributeMap->attributeData() : 0; }
+    ElementAttributeData* attributeData() const { return m_attributeData.get(); }
     ElementAttributeData* ensureAttributeData() const;
     ElementAttributeData* updatedAttributeData() const;
     ElementAttributeData* ensureUpdatedAttributeData() const;
@@ -436,7 +437,7 @@ private:
 
     bool pseudoStyleCacheIsInvalid(const RenderStyle* currentStyle, RenderStyle* newStyle);
 
-    void createAttributeMap() const;
+    void createAttributeData() const;
 
     virtual void updateStyleAttribute() const { }
 
@@ -468,7 +469,7 @@ private:
     void updateExtraNamedItemRegistration(const AtomicString& oldName, const AtomicString& newName);
 
 private:
-    mutable OwnPtr<NamedNodeMap> m_attributeMap;
+    mutable OwnPtr<ElementAttributeData> m_attributeData;
 };
     
 inline Element* toElement(Node* node)
@@ -528,25 +529,11 @@ inline Element* Element::nextElementSibling() const
     return static_cast<Element*>(n);
 }
 
-inline NamedNodeMap* Element::attributes() const
-{
-    updateInvalidAttributes();
-    if (!m_attributeMap)
-        createAttributeMap();
-    return m_attributeMap.get();
-}
-
-inline NamedNodeMap* Element::updatedAttributes() const
-{
-    updateInvalidAttributes();
-    return m_attributeMap.get();
-}
-
 inline ElementAttributeData* Element::ensureAttributeData() const
 {
-    if (!m_attributeMap)
-        createAttributeMap();
-    return m_attributeMap->attributeData();
+    if (!m_attributeData)
+        createAttributeData();
+    return m_attributeData.get();
 }
 
 inline ElementAttributeData* Element::updatedAttributeData() const
@@ -606,13 +593,13 @@ inline void Element::willRemoveAttribute(const QualifiedName& name, const Atomic
 inline bool Element::fastHasAttribute(const QualifiedName& name) const
 {
     ASSERT(fastAttributeLookupAllowed(name));
-    return m_attributeMap && getAttributeItem(name);
+    return m_attributeData && getAttributeItem(name);
 }
 
 inline const AtomicString& Element::fastGetAttribute(const QualifiedName& name) const
 {
     ASSERT(fastAttributeLookupAllowed(name));
-    if (m_attributeMap) {
+    if (m_attributeData) {
         if (Attribute* attribute = getAttributeItem(name))
             return attribute->value();
     }
@@ -621,13 +608,13 @@ inline const AtomicString& Element::fastGetAttribute(const QualifiedName& name)
 
 inline bool Element::hasAttributesWithoutUpdate() const
 {
-    return m_attributeMap && !m_attributeMap->attributeData()->isEmpty();
+    return m_attributeData && !m_attributeData->isEmpty();
 }
 
 inline const AtomicString& Element::idForStyleResolution() const
 {
     ASSERT(hasID());
-    return attributeData()->idForStyleResolution();
+    return m_attributeData->idForStyleResolution();
 }
 
 inline bool Element::isIdAttributeName(const QualifiedName& attributeName) const
@@ -656,20 +643,20 @@ inline void Element::setIdAttribute(const AtomicString& value)
 
 inline size_t Element::attributeCount() const
 {
-    ASSERT(m_attributeMap);
-    return m_attributeMap->length();
+    ASSERT(m_attributeData);
+    return m_attributeData->length();
 }
 
 inline Attribute* Element::attributeItem(unsigned index) const
 {
-    ASSERT(m_attributeMap);
-    return m_attributeMap->attributeData()->attributeItem(index);
+    ASSERT(m_attributeData);
+    return m_attributeData->attributeItem(index);
 }
 
 inline Attribute* Element::getAttributeItem(const QualifiedName& name) const
 {
-    ASSERT(m_attributeMap);
-    return m_attributeMap->attributeData()->getAttributeItem(name);
+    ASSERT(m_attributeData);
+    return m_attributeData->getAttributeItem(name);
 }
 
 inline void Element::updateInvalidAttributes() const
index 5602ab2..1fbf3ee 100644 (file)
@@ -82,6 +82,11 @@ inline void AttributeVector::insertAttribute(PassRefPtr<Attribute> newAttribute)
 
 class ElementAttributeData {
 public:
+    static PassOwnPtr<ElementAttributeData> create()
+    {
+        return adoptPtr(new ElementAttributeData);
+    }
+
     ~ElementAttributeData();
 
     void clearClass() { m_classNames.clear(); }
@@ -108,6 +113,7 @@ public:
     Attribute* attributeItem(unsigned index) const { return m_attributes.attributeItem(index); }
     Attribute* getAttributeItem(const QualifiedName& name) const { return m_attributes.getAttributeItem(name); }
     size_t getAttributeItemIndex(const QualifiedName& name) const { return m_attributes.getAttributeItemIndex(name); }
+    size_t getAttributeItemIndex(const String& name, bool shouldIgnoreAttributeCase) const;
 
     // These functions do no error checking.
     void addAttribute(PassRefPtr<Attribute>, Element*);
@@ -122,7 +128,6 @@ public:
 
 private:
     friend class Element;
-    friend class NamedNodeMap;
 
     ElementAttributeData()
     {
@@ -131,7 +136,6 @@ private:
     void detachAttributesFromElement();
     void copyAttributesToVector(Vector<RefPtr<Attribute> >&);
     Attribute* getAttributeItem(const String& name, bool shouldIgnoreAttributeCase) const;
-    size_t getAttributeItemIndex(const String& name, bool shouldIgnoreAttributeCase) const;
     size_t getAttributeItemIndexSlowCase(const String& name, bool shouldIgnoreAttributeCase) const;
     void setAttributes(const ElementAttributeData& other, Element*);
     void clearAttributes();
index 11c8257..cd2384d 100644 (file)
@@ -26,6 +26,7 @@
 #include "DatasetDOMStringMap.h"
 #include "Element.h"
 #include "HTMLCollection.h"
+#include "NamedNodeMap.h"
 #include "NodeRareData.h"
 #include "ShadowTree.h"
 #include <wtf/OwnPtr.h>
@@ -69,6 +70,7 @@ public:
     OwnPtr<DatasetDOMStringMap> m_datasetDOMStringMap;
     OwnPtr<ClassList> m_classList;
     OwnPtr<ShadowTree> m_shadowTree;
+    OwnPtr<NamedNodeMap> m_attributeMap;
 
     bool m_styleAffectedByEmpty;
 
index 0b7b303..942c40c 100644 (file)
@@ -42,47 +42,53 @@ static inline bool shouldIgnoreAttributeCase(const Element* e)
 
 void NamedNodeMap::ref()
 {
-    ASSERT(m_element);
     m_element->ref();
 }
 
 void NamedNodeMap::deref()
 {
-    ASSERT(m_element);
     m_element->deref();
 }
 
 PassRefPtr<Node> NamedNodeMap::getNamedItem(const String& name) const
 {
-    return m_attributeData.getAttributeNode(name, shouldIgnoreAttributeCase(m_element), m_element);
+    return m_element->attributeData()->getAttributeNode(name, shouldIgnoreAttributeCase(m_element), m_element);
 }
 
 PassRefPtr<Node> NamedNodeMap::getNamedItemNS(const String& namespaceURI, const String& localName) const
 {
-    return m_attributeData.getAttributeNode(QualifiedName(nullAtom, localName, namespaceURI), m_element);
+    return m_element->attributeData()->getAttributeNode(QualifiedName(nullAtom, localName, namespaceURI), m_element);
 }
 
 PassRefPtr<Node> NamedNodeMap::removeNamedItem(const String& name, ExceptionCode& ec)
 {
-    ASSERT(m_element);
+    ElementAttributeData* attributeData = m_element->attributeData();
 
-    size_t index = m_attributeData.getAttributeItemIndex(name, shouldIgnoreAttributeCase(m_element));
+    size_t index = attributeData->getAttributeItemIndex(name, shouldIgnoreAttributeCase(m_element));
     if (index == notFound) {
         ec = NOT_FOUND_ERR;
         return 0;
     }
     
-    return m_attributeData.takeAttribute(index, m_element);
+    return attributeData->takeAttribute(index, m_element);
 }
 
 PassRefPtr<Node> NamedNodeMap::removeNamedItemNS(const String& namespaceURI, const String& localName, ExceptionCode& ec)
 {
-    return removeNamedItem(QualifiedName(nullAtom, localName, namespaceURI), ec);
+    ElementAttributeData* attributeData = m_element->attributeData();
+
+    size_t index = attributeData->getAttributeItemIndex(QualifiedName(nullAtom, localName, namespaceURI));
+    if (index == notFound) {
+        ec = NOT_FOUND_ERR;
+        return 0;
+    }
+
+    return attributeData->takeAttribute(index, m_element);
 }
 
 PassRefPtr<Node> NamedNodeMap::setNamedItem(Node* node, ExceptionCode& ec)
 {
-    if (!m_element || !node) {
+    if (!node) {
         ec = NOT_FOUND_ERR;
         return 0;
     }
@@ -101,42 +107,16 @@ PassRefPtr<Node> NamedNodeMap::setNamedItemNS(Node* node, ExceptionCode& ec)
     return setNamedItem(node, ec);
 }
 
-PassRefPtr<Node> NamedNodeMap::removeNamedItem(const QualifiedName& name, ExceptionCode& ec)
-{
-    ASSERT(m_element);
-
-    size_t index = m_attributeData.getAttributeItemIndex(name);
-    if (index == notFound) {
-        ec = NOT_FOUND_ERR;
-        return 0;
-    }
-
-    return m_attributeData.takeAttribute(index, m_element);
-}
-
 PassRefPtr<Node> NamedNodeMap::item(unsigned index) const
 {
     if (index >= length())
         return 0;
-
-    return m_attributeData.m_attributes[index]->createAttrIfNeeded(m_element);
-}
-
-void NamedNodeMap::detachFromElement()
-{
-    // This can't happen if the holder of the map is JavaScript, because we mark the
-    // element if the map is alive. So it has no impact on web page behavior. Because
-    // of that, we can simply clear all the attributes to avoid accessing stale
-    // pointers to do things like create Attr objects.
-    m_element = 0;
-    m_attributeData.clearAttributes();
+    return m_element->attributeItem(index)->createAttrIfNeeded(m_element);
 }
 
-bool NamedNodeMap::mapsEquivalent(const NamedNodeMap* otherMap) const
+size_t NamedNodeMap::length() const
 {
-    if (!otherMap)
-        return m_attributeData.isEmpty();
-    return m_attributeData.isEquivalent(otherMap->attributeData());
+    return m_element->attributeCount();
 }
 
 } // namespace WebCore
index dad3198..d42d52d 100644 (file)
 #ifndef NamedNodeMap_h
 #define NamedNodeMap_h
 
-#include "ElementAttributeData.h"
-#include "SpaceSplitString.h"
+#include <wtf/PassOwnPtr.h>
+#include <wtf/PassRefPtr.h>
+#include <wtf/text/WTFString.h>
 
 namespace WebCore {
 
 class Node;
+class Element;
 
 typedef int ExceptionCode;
 
 class NamedNodeMap {
     friend class Element;
 public:
-    static PassOwnPtr<NamedNodeMap> create(Element* element = 0)
+    static PassOwnPtr<NamedNodeMap> create(Element* element)
     {
         return adoptPtr(new NamedNodeMap(element));
     }
@@ -53,37 +55,22 @@ public:
     PassRefPtr<Node> getNamedItemNS(const String& namespaceURI, const String& localName) const;
     PassRefPtr<Node> removeNamedItemNS(const String& namespaceURI, const String& localName, ExceptionCode&);
 
-    PassRefPtr<Node> removeNamedItem(const QualifiedName& name, ExceptionCode&);
     PassRefPtr<Node> setNamedItem(Node*, ExceptionCode&);
     PassRefPtr<Node> setNamedItemNS(Node*, ExceptionCode&);
 
     PassRefPtr<Node> item(unsigned index) const;
-    size_t length() const { return m_attributeData.length(); }
-
-    bool mapsEquivalent(const NamedNodeMap* otherMap) const;
-
-    // These functions do no error checking.
-    void addAttribute(PassRefPtr<Attribute> attribute) { m_attributeData.addAttribute(attribute, m_element); }
+    size_t length() const;
 
     Element* element() const { return m_element; }
 
-    ElementAttributeData* attributeData() { return &m_attributeData; }
-    const ElementAttributeData* attributeData() const { return &m_attributeData; }
-
 private:
     NamedNodeMap(Element* element)
         : m_element(element)
     {
+        // Only supports NamedNodeMaps with Element associated, DocumentType.entities and DocumentType.notations are not supported yet.
+        ASSERT(m_element);
     }
 
-    void detachFromElement();
-    Attribute* getAttributeItem(const String& name, bool shouldIgnoreAttributeCase) const { return m_attributeData.getAttributeItem(name, shouldIgnoreAttributeCase); }
-
-    // FIXME: NamedNodeMap is being broken up into two classes, one containing data
-    //        for elements with attributes, and one for exposure to the DOM.
-    //        See <http://webkit.org/b/75069> for more information.
-    ElementAttributeData m_attributeData;
-
     Element* m_element;
 };
 
index 3b716aa..f06d9bd 100644 (file)
@@ -160,13 +160,20 @@ void Node::dumpStatistics()
     size_t attributes = 0;
     size_t attributesWithAttr = 0;
     size_t elementsWithAttributeStorage = 0;
+    size_t elementsWithRareData = 0;
     size_t elementsWithNamedNodeMap = 0;
 
     for (HashSet<Node*>::iterator it = liveNodeSet.begin(); it != liveNodeSet.end(); ++it) {
         Node* node = *it;
 
-        if (node->hasRareData())
+        if (node->hasRareData()) {
             ++nodesWithRareData;
+            if (node->isElementNode()) {
+                ++elementsWithRareData;
+                if (toElement(node)->hasNamedNodeMap())
+                    ++elementsWithNamedNodeMap;
+            }
+        }
 
         switch (node->nodeType()) {
             case ELEMENT_NODE: {
@@ -181,8 +188,6 @@ void Node::dumpStatistics()
                 if (ElementAttributeData* attributeData = element->attributeData()) {
                     attributes += attributeData->length();
                     ++elementsWithAttributeStorage;
-                    // FIXME: This will change once attribute storage goes out of NamedNodeMap.
-                    ++elementsWithNamedNodeMap;
                     for (unsigned i = 0; i < attributeData->length(); ++i) {
                         Attribute* attr = attributeData->attributeItem(i);
                         if (attr->attr())
@@ -272,6 +277,7 @@ void Node::dumpStatistics()
     printf("  Number of Attributes (non-Node and Node): %zu [%zu]\n", attributes, sizeof(Attribute));
     printf("  Number of Attributes with an Attr: %zu\n", attributesWithAttr);
     printf("  Number of Elements with attribute storage: %zu [%zu]\n", elementsWithAttributeStorage, sizeof(ElementAttributeData));
+    printf("  Number of Elements with RareData: %zu\n", elementsWithRareData);
     printf("  Number of Elements with NamedNodeMap: %zu [%zu]\n", elementsWithNamedNodeMap, sizeof(NamedNodeMap));
 #endif
 }
@@ -1803,19 +1809,7 @@ bool Node::isEqualNode(Node* other) const
         if (documentTypeThis->internalSubset() != documentTypeOther->internalSubset())
             return false;
 
-        NamedNodeMap* entities = documentTypeThis->entities();
-        NamedNodeMap* otherEntities = documentTypeOther->entities();
-        if (!entities && otherEntities)
-            return false;
-        if (entities && !entities->mapsEquivalent(otherEntities))
-            return false;
-
-        NamedNodeMap* notations = documentTypeThis->notations();
-        NamedNodeMap* otherNotations = documentTypeOther->notations();
-        if (!notations && otherNotations)
-            return false;
-        if (notations && !notations->mapsEquivalent(otherNotations))
-            return false;
+        // FIXME: We don't compare entities or notations because currently both are always empty.
     }
     
     return true;
@@ -2114,7 +2108,7 @@ unsigned short Node::compareDocumentPosition(Node* otherNode)
     if (attr1 && attr2 && start1 == start2 && start1) {
         // We are comparing two attributes on the same node. Crawl our attribute map and see which one we hit first.
         Element* owner1 = attr1->ownerElement();
-        owner1->updatedAttributes(); // Force update invalid attributes.
+        owner1->updatedAttributeData(); // Force update invalid attributes.
         unsigned length = owner1->attributeCount();
         for (unsigned i = 0; i < length; ++i) {
             // If neither of the two determining nodes is a child node and nodeType is the same for both determining nodes, then an 
index 4d040f0..dd67a63 100644 (file)
@@ -44,7 +44,6 @@ namespace WebCore {
 class ContainerNode;
 class DOMEditor;
 class Document;
-class NamedNodeMap;
 class Node;
 
 #if ENABLE(INSPECTOR)
index e65f5ab..f0486a8 100644 (file)
@@ -415,7 +415,7 @@ void SVGElement::attributeChanged(Attribute* attr)
 
     // When an animated SVG property changes through SVG DOM, svgAttributeChanged() is called, not attributeChanged().
     // Next time someone tries to access the XML attributes, the synchronization code starts. During that synchronization
-    // SVGAnimatedPropertySynchronizer may call NamedNodeMap::removeAttribute(), which in turn calls attributeChanged().
+    // SVGAnimatedPropertySynchronizer may call ElementAttributeData::removeAttribute(), which in turn calls attributeChanged().
     // At this point we're not allowed to call svgAttributeChanged() again - it may lead to extra work being done, or crashes
     // see bug https://bugs.webkit.org/show_bug.cgi?id=40994.
     if (isSynchronizingSVGAttributes())
index 76073af..38d2ca7 100644 (file)
@@ -1,3 +1,12 @@
+2012-03-08  Caio Marcelo de Oliveira Filho  <caio.oliveira@openbossa.org>
+
+        Make elements with attributes smaller by eliminating the m_element back pointer in NamedNodeMap
+        https://bugs.webkit.org/show_bug.cgi?id=75069
+
+        Reviewed by Ryosuke Niwa.
+
+        * src/WebElement.cpp: Include NamedNodeMap.h since Element.h doesn't include it anymore.
+
 2012-03-07  Kent Tamura  <tkent@chromium.org>
 
         Remove meaningless code in RenderTextControlSingleLine::preferredContentWidth()
index 43e51d4..e6a1b6e 100644 (file)
@@ -34,6 +34,7 @@
 #include "WebDocument.h"
 
 #include "Element.h"
+#include "NamedNodeMap.h"
 #include "RenderBoxModelObject.h"
 #include "RenderObject.h"
 #include <wtf/PassRefPtr.h>