Cache and reuse the NodeList returned by Node::childNodes().

<http://webkit.org/b/76591>

Reviewed by Ryosuke Niwa.

Source/WebCore:

Instead of only caching the DynamicNodeList::Caches for .childNodes on NodeRareData,
cache the full ChildNodeList object. Lifetime management is left to wrappers who
invalidate the cached (raw) pointer via Node::removeCachedChildNodeList(), called
from ~ChildNodeList().

This is a slight behavior change, in that Node.childNodes === Node.childNodes will
now be true. This matches the behavior of both Firefox and Opera.

This reduces memory consumption by 192 kB (on 32-bit) when viewing the full
HTML5 spec at <http://whatwg.org/c>

Test: fast/dom/gc-9.html
      fast/dom/node-childNodes-idempotence.html

* dom/Node.cpp:
(WebCore::Node::childNodes):
* dom/NodeRareData.h:
(WebCore::NodeRareData::NodeRareData):
(WebCore::NodeRareData::childNodeList):
(WebCore::NodeRareData::setChildNodeList):

    Only construct one ChildNodeList per Node and store it on NodeRareData for
    retrieval across childNodes() calls.

* dom/ChildNodeList.h:
(WebCore::ChildNodeList::create):
* dom/ChildNodeList.cpp:
(WebCore::ChildNodeList::ChildNodeList):

    Construct the Caches at creation instead of passing it to the constructor.

(WebCore::ChildNodeList::reset):

    Added, resets the internal cache.

(WebCore::ChildNodeList::~ChildNodeList):

    Call Node::removeCachedChildNodeList().

* dom/DynamicNodeList.cpp:
* dom/DynamicNodeList.h:

    Have DynamicNodeList (and subclasses) respond "true" to isDynamicNodeList().
    Previously only DynamicSubtreeNodeList (and subclasses) were doing this.
    Without it, JSC may GC our ChildNodeLists prematurely (due to NodeList's
    isReachableFromOpaqueRoots() implementation checking isDynamicNodeList().)

* dom/Node.h:
* dom/Node.cpp:
(WebCore::Node::removeCachedChildNodeList):

    Added for ~ChildNodeList() to remove the pointer to itself from the Node.

(WebCore::NodeRareData::clearChildNodeListCache):

    Call ChildNodeList::reset().

LayoutTests:

Updated gc-9.html to document the new lifetime characteristics of a .childNodes with
custom properties. Also added a test to verify that .childNodes === .childNodes.

* fast/dom/gc-9-expected.txt:
* fast/dom/gc-9.html:
* fast/dom/node-childNodes-idempotence-expected.txt: Added.
* fast/dom/node-childNodes-idempotence.html: Added.

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

diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index e385174..7fcc074 100644 (file)
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -1,3 +1,18 @@
+2012-01-18  Andreas Kling  <awesomekling@apple.com>
+
+        Cache and reuse the NodeList returned by Node::childNodes().
+        <http://webkit.org/b/76591>
+
+        Reviewed by Ryosuke Niwa.
+
+        Updated gc-9.html to document the new lifetime characteristics of a .childNodes with
+        custom properties. Also added a test to verify that .childNodes === .childNodes.
+
+        * fast/dom/gc-9-expected.txt:
+        * fast/dom/gc-9.html:
+        * fast/dom/node-childNodes-idempotence-expected.txt: Added.
+        * fast/dom/node-childNodes-idempotence.html: Added.
+
 2012-01-18  Alexey Proskuryakov  <ap@apple.com>
 
         Need infrastructure to test Content-Disposition filename encoding support

diff --git a/LayoutTests/fast/dom/gc-9-expected.txt b/LayoutTests/fast/dom/gc-9-expected.txt
index 4d16f90..191f4dd 100644 (file)
--- a/LayoutTests/fast/dom/gc-9-expected.txt
+++ b/LayoutTests/fast/dom/gc-9-expected.txt
@@ -14,7 +14,7 @@ PASS: document.getElementsByTagName('canvas')[0].getContext('2d').myCustomProper
 PASS: document.getElementsByTagName('canvas')[0].getContext('2d').createLinearGradient(0, 0, 0, 0).myCustomProperty should be undefined and is.
 PASS: document.getElementsByTagName('canvas')[0].getContext('2d').createPattern(new Image(), 'no-repeat').myCustomProperty should be undefined and is.
 PASS: document.getElementsByTagName('select')[0].options.myCustomProperty should be 1 and is.
-PASS: document.body.childNodes.myCustomProperty should be undefined and is.
+PASS: document.body.childNodes.myCustomProperty should be 1 and is.
 PASS: document.all.myCustomProperty should be 1 and is.
 PASS: document.images.myCustomProperty should be 1 and is.
 PASS: document.embeds.myCustomProperty should be 1 and is.
@@ -50,7 +50,7 @@ PASS: document.getElementsByTagName('canvas')[0].getContext('2d').myCustomProper
 PASS: document.getElementsByTagName('canvas')[0].getContext('2d').createLinearGradient(0, 0, 0, 0).myCustomProperty should be undefined and is.
 PASS: document.getElementsByTagName('canvas')[0].getContext('2d').createPattern(new Image(), 'no-repeat').myCustomProperty should be undefined and is.
 PASS: document.getElementsByTagName('select')[0].options.myCustomProperty should be 1 and is.
-PASS: document.body.childNodes.myCustomProperty should be undefined and is.
+PASS: document.body.childNodes.myCustomProperty should be 1 and is.
 PASS: document.all.myCustomProperty should be 1 and is.
 PASS: document.images.myCustomProperty should be 1 and is.
 PASS: document.embeds.myCustomProperty should be 1 and is.

diff --git a/LayoutTests/fast/dom/gc-9.html b/LayoutTests/fast/dom/gc-9.html
index 1b1786c..49f2240 100644 (file)
--- a/LayoutTests/fast/dom/gc-9.html
+++ b/LayoutTests/fast/dom/gc-9.html
@@ -123,7 +123,7 @@ var objectsToTest = [
     [ "document.getElementsByTagName('canvas')[0].getContext('2d').createLinearGradient(0, 0, 0, 0)" ], // CanvasGradient
     [ "document.getElementsByTagName('canvas')[0].getContext('2d').createPattern(new Image(), 'no-repeat')" ], // CanvasPattern
     [ "document.getElementsByTagName('select')[0].options", "allow custom" ],
-    [ "document.body.childNodes" ],
+    [ "document.body.childNodes", "allow custom" ],
 
     [ "document.all", "allow custom" ],
     [ "document.images", "allow custom" ],

diff --git a/LayoutTests/fast/dom/node-childNodes-idempotence-expected.txt b/LayoutTests/fast/dom/node-childNodes-idempotence-expected.txt
new file mode 100644 (file)
index 0000000..ffcfc59
--- /dev/null
+++ b/LayoutTests/fast/dom/node-childNodes-idempotence-expected.txt
@@ -0,0 +1,11 @@
+This test verifies that Node.childNodes returns the same NodeList when called repeatedly.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS document.documentElement.childNodes is document.documentElement.childNodes
+PASS document.documentElement.childNodes === document.documentElement.childNodes is true
+PASS successfullyParsed is true
+
+TEST COMPLETE
+

diff --git a/LayoutTests/fast/dom/node-childNodes-idempotence.html b/LayoutTests/fast/dom/node-childNodes-idempotence.html
new file mode 100644 (file)
index 0000000..fdb90d3
--- /dev/null
+++ b/LayoutTests/fast/dom/node-childNodes-idempotence.html
@@ -0,0 +1,18 @@
+<!DOCTYPE html>
+<html>
+<head>
+<meta charset="utf-8">
+<script src="../js/resources/js-test-pre.js"></script>
+</head>
+<body>
+<script>
+
+description("This test verifies that Node.childNodes returns the same NodeList when called repeatedly.");
+
+shouldBe("document.documentElement.childNodes", "document.documentElement.childNodes");
+shouldBeTrue("document.documentElement.childNodes === document.documentElement.childNodes");
+
+</script>
+<script src="../js/resources/js-test-post.js"></script>
+</body>
+</html>

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index fe92bf5..45d9643 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,67 @@
+2012-01-18  Andreas Kling  <awesomekling@apple.com>
+
+        Cache and reuse the NodeList returned by Node::childNodes().
+        <http://webkit.org/b/76591>
+
+        Reviewed by Ryosuke Niwa.
+
+        Instead of only caching the DynamicNodeList::Caches for .childNodes on NodeRareData,
+        cache the full ChildNodeList object. Lifetime management is left to wrappers who
+        invalidate the cached (raw) pointer via Node::removeCachedChildNodeList(), called
+        from ~ChildNodeList().
+
+        This is a slight behavior change, in that Node.childNodes === Node.childNodes will
+        now be true. This matches the behavior of both Firefox and Opera.
+
+        This reduces memory consumption by 192 kB (on 32-bit) when viewing the full
+        HTML5 spec at <http://whatwg.org/c>
+
+        Test: fast/dom/gc-9.html
+              fast/dom/node-childNodes-idempotence.html
+
+        * dom/Node.cpp:
+        (WebCore::Node::childNodes):
+        * dom/NodeRareData.h:
+        (WebCore::NodeRareData::NodeRareData):
+        (WebCore::NodeRareData::childNodeList):
+        (WebCore::NodeRareData::setChildNodeList):
+
+            Only construct one ChildNodeList per Node and store it on NodeRareData for
+            retrieval across childNodes() calls.
+
+        * dom/ChildNodeList.h:
+        (WebCore::ChildNodeList::create):
+        * dom/ChildNodeList.cpp:
+        (WebCore::ChildNodeList::ChildNodeList):
+
+            Construct the Caches at creation instead of passing it to the constructor.
+
+        (WebCore::ChildNodeList::reset):
+
+            Added, resets the internal cache.
+
+        (WebCore::ChildNodeList::~ChildNodeList):
+
+            Call Node::removeCachedChildNodeList().
+
+        * dom/DynamicNodeList.cpp:
+        * dom/DynamicNodeList.h:
+
+            Have DynamicNodeList (and subclasses) respond "true" to isDynamicNodeList().
+            Previously only DynamicSubtreeNodeList (and subclasses) were doing this.
+            Without it, JSC may GC our ChildNodeLists prematurely (due to NodeList's
+            isReachableFromOpaqueRoots() implementation checking isDynamicNodeList().)
+
+        * dom/Node.h:
+        * dom/Node.cpp:
+        (WebCore::Node::removeCachedChildNodeList):
+
+            Added for ~ChildNodeList() to remove the pointer to itself from the Node.
+
+        (WebCore::NodeRareData::clearChildNodeListCache):
+
+            Call ChildNodeList::reset().
+
 2012-01-18  James Robinson  <jamesr@chromium.org>
 
         Unreviewed, rolling out r105366.

diff --git a/Source/WebCore/dom/ChildNodeList.cpp b/Source/WebCore/dom/ChildNodeList.cpp
index abb5401..865470b 100644 (file)
--- a/Source/WebCore/dom/ChildNodeList.cpp
+++ b/Source/WebCore/dom/ChildNodeList.cpp
@@ -27,12 +27,17 @@
 
 namespace WebCore {
 
-ChildNodeList::ChildNodeList(PassRefPtr<Node> node, DynamicNodeList::Caches* caches)
+ChildNodeList::ChildNodeList(PassRefPtr<Node> node)
     : DynamicNodeList(node)
-    , m_caches(caches)
+    , m_caches(Caches::create())
 {
 }
 
+ChildNodeList::~ChildNodeList()
+{
+    node()->removeCachedChildNodeList();
+}
+
 unsigned ChildNodeList::length() const
 {
     if (m_caches->isLengthCacheValid)

diff --git a/Source/WebCore/dom/ChildNodeList.h b/Source/WebCore/dom/ChildNodeList.h
index 137b6d6..85ec349 100644 (file)
--- a/Source/WebCore/dom/ChildNodeList.h
+++ b/Source/WebCore/dom/ChildNodeList.h
@@ -31,16 +31,20 @@ namespace WebCore {
 
     class ChildNodeList : public DynamicNodeList {
     public:
-        static PassRefPtr<ChildNodeList> create(PassRefPtr<Node> rootNode, Caches* caches)
+        static PassRefPtr<ChildNodeList> create(PassRefPtr<Node> rootNode)
         {
-            return adoptRef(new ChildNodeList(rootNode, caches));
+            return adoptRef(new ChildNodeList(rootNode));
         }
 
+        virtual ~ChildNodeList();
+
         virtual unsigned length() const;
         virtual Node* item(unsigned index) const;
 
+        void reset() { m_caches->reset(); }
+
     protected:
-        ChildNodeList(PassRefPtr<Node> rootNode, Caches*);
+        ChildNodeList(PassRefPtr<Node> rootNode);
 
         virtual bool nodeMatches(Element*) const;
 

diff --git a/Source/WebCore/dom/DynamicNodeList.cpp b/Source/WebCore/dom/DynamicNodeList.cpp
index 788ab6e..d07a823 100644 (file)
--- a/Source/WebCore/dom/DynamicNodeList.cpp
+++ b/Source/WebCore/dom/DynamicNodeList.cpp
@@ -132,11 +132,6 @@ Node* DynamicNodeList::itemWithName(const AtomicString& elementId) const
     return 0;
 }
 
-bool DynamicSubtreeNodeList::isDynamicNodeList() const
-{
-    return true;
-}
-
 void DynamicSubtreeNodeList::invalidateCache()
 {
     m_caches->reset();

diff --git a/Source/WebCore/dom/DynamicNodeList.h b/Source/WebCore/dom/DynamicNodeList.h
index 606fc64..7fc90d5 100644 (file)
--- a/Source/WebCore/dom/DynamicNodeList.h
+++ b/Source/WebCore/dom/DynamicNodeList.h
@@ -64,6 +64,9 @@ public:
 protected:
     virtual bool nodeMatches(Element*) const = 0;
     RefPtr<Node> m_node;
+
+private:
+    virtual bool isDynamicNodeList() const OVERRIDE { return true; }
 };
 
 class DynamicSubtreeNodeList : public DynamicNodeList {
@@ -79,7 +82,6 @@ protected:
     mutable RefPtr<Caches> m_caches;
 
 private:
-    virtual bool isDynamicNodeList() const;
     Node* itemForwardsFromCurrent(Node* start, unsigned offset, int remainingOffset) const;
     Node* itemBackwardsFromCurrent(Node* start, unsigned offset, int remainingOffset) const;
 };

diff --git a/Source/WebCore/dom/Node.cpp b/Source/WebCore/dom/Node.cpp
index a1c7624..eac0c95 100644 (file)
--- a/Source/WebCore/dom/Node.cpp
+++ b/Source/WebCore/dom/Node.cpp
@@ -552,7 +552,13 @@ void Node::setNodeValue(const String& /*nodeValue*/, ExceptionCode& ec)
 
 PassRefPtr<NodeList> Node::childNodes()
 {
-    return ChildNodeList::create(this, ensureRareData()->ensureChildNodeListCache());
+    NodeRareData* data = ensureRareData();
+    if (data->childNodeList())
+        return PassRefPtr<NodeList>(data->childNodeList());
+
+    RefPtr<ChildNodeList> list = ChildNodeList::create(this);
+    data->setChildNodeList(list.get());
+    return list.release();
 }
 
 Node *Node::lastDescendant() const
@@ -1074,6 +1080,12 @@ void Node::removeCachedLabelsNodeList(DynamicSubtreeNodeList* list)
     data->m_labelsNodeListCache = 0;
 }
 
+void Node::removeCachedChildNodeList()
+{
+    ASSERT(rareData());
+    rareData()->setChildNodeList(0);
+}
+
 Node* Node::traverseNextNode(const Node* stayWithin) const
 {
     if (firstChild())
@@ -2935,13 +2947,8 @@ void NodeRareData::createNodeLists(Node* node)
 
 void NodeRareData::clearChildNodeListCache()
 {
-    if (!m_childNodeListCache)
-        return;
-
-    if (m_childNodeListCache->hasOneRef())
-        m_childNodeListCache.clear();
-    else
-        m_childNodeListCache->reset();
+    if (m_childNodeList)
+        m_childNodeList->reset();
 }
 
 } // namespace WebCore

diff --git a/Source/WebCore/dom/Node.h b/Source/WebCore/dom/Node.h
index 9d6068b..09709e0 100644 (file)
--- a/Source/WebCore/dom/Node.h
+++ b/Source/WebCore/dom/Node.h
@@ -553,6 +553,8 @@ public:
     void removeCachedTagNodeList(TagNodeList*, const QualifiedName&);
     void removeCachedLabelsNodeList(DynamicSubtreeNodeList*);
 
+    void removeCachedChildNodeList();
+
     PassRefPtr<NodeList> getElementsByTagName(const AtomicString&);
     PassRefPtr<NodeList> getElementsByTagNameNS(const AtomicString& namespaceURI, const AtomicString& localName);
     PassRefPtr<NodeList> getElementsByName(const String& elementName);

diff --git a/Source/WebCore/dom/NodeRareData.h b/Source/WebCore/dom/NodeRareData.h
index 362135c..ef17b86 100644 (file)
--- a/Source/WebCore/dom/NodeRareData.h
+++ b/Source/WebCore/dom/NodeRareData.h
@@ -95,6 +95,7 @@ class NodeRareData {
 public:    
     NodeRareData()
         : m_treeScope(0)
+        , m_childNodeList(0)
         , m_tabIndex(0)
         , m_tabIndexWasSetExplicitly(false)
         , m_isFocused(false)
@@ -132,12 +133,9 @@ public:
         return m_nodeLists.get();
     }
     void clearChildNodeListCache();
-    DynamicNodeList::Caches* ensureChildNodeListCache()
-    {
-        if (!m_childNodeListCache)
-            m_childNodeListCache = DynamicNodeList::Caches::create();
-        return m_childNodeListCache.get();
-    }
+
+    ChildNodeList* childNodeList() const { return m_childNodeList; }
+    void setChildNodeList(ChildNodeList* list) { m_childNodeList = list; }
 
     short tabIndex() const { return m_tabIndex; }
     void setTabIndexExplicitly(short index) { m_tabIndex = index; m_tabIndexWasSetExplicitly = true; }
@@ -241,7 +239,7 @@ private:
 
     TreeScope* m_treeScope;
     OwnPtr<NodeListsNodeData> m_nodeLists;
-    RefPtr<DynamicNodeList::Caches> m_childNodeListCache;
+    ChildNodeList* m_childNodeList;
     OwnPtr<EventTargetData> m_eventTargetData;
     short m_tabIndex;
     bool m_tabIndexWasSetExplicitly : 1;