Cache and reuse HTMLCollections exposed by Document.
authorkling@webkit.org <kling@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 16 Dec 2011 23:11:11 +0000 (23:11 +0000)
committerkling@webkit.org <kling@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 16 Dec 2011 23:11:11 +0000 (23:11 +0000)
<http://webkit.org/b/71956>

Reviewed by Antti Koivisto.

Source/WebCore:

Let Document cache the various HTMLCollection objects it exposes.
This is a behavior change in two ways:

1) The lifetime of returned collections is now tied to the lifetime
   of the Document. This matches the behavior of Firefox and Opera.

2) The cached collections returned by document are now exactly equal
   to those returned by subsequent calls to the same getters.

This reduces memory consumption by ~800 kB (on 64-bit) when loading
the full HTML5 spec. document.links was called 34001 times, yielding
34001 separate HTMLCollections, and now we only need 1.

The document.all collection retains the old behavior, as caching it
will be a bit more complicated.

To avoid a reference cycle between Document and HTMLCollection,
collections that are cached on Document do not retained their base
node pointer (controlled by a m_baseIsRetained flag.)

Tests: fast/dom/document-collection-idempotence.html
       fast/dom/gc-9.html

* dom/Document.cpp:
(WebCore::Document::detach):
(WebCore::Document::cachedCollection):
(WebCore::Document::images):
(WebCore::Document::applets):
(WebCore::Document::embeds):
(WebCore::Document::plugins):
(WebCore::Document::objects):
(WebCore::Document::scripts):
(WebCore::Document::links):
(WebCore::Document::forms):
(WebCore::Document::anchors):
* dom/Document.h:
* html/HTMLCollection.cpp:
(WebCore::HTMLCollection::HTMLCollection):
(WebCore::HTMLCollection::createForCachingOnDocument):
(WebCore::HTMLCollection::~HTMLCollection):
(WebCore::HTMLCollection::itemAfter):
* html/HTMLCollection.h:
(WebCore::HTMLCollection::base):

LayoutTests:

Added a test to verify that collections returned by document (excluding .all)
are equal to the collections returned by subsequent calls to the same getters.

Also update fast/dom/gc-9.html to cover the new lifetime behavior of
HTMLCollection objects returned by document.

* fast/dom/document-collection-idempotence-expected.txt: Added.
* fast/dom/document-collection-idempotence.html: Added.
* fast/dom/gc-9-expected.txt:
* fast/dom/gc-9.html:

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

LayoutTests/ChangeLog
LayoutTests/fast/dom/document-collection-idempotence-expected.txt [new file with mode: 0644]
LayoutTests/fast/dom/document-collection-idempotence.html [new file with mode: 0644]
LayoutTests/fast/dom/gc-9-expected.txt
LayoutTests/fast/dom/gc-9.html
Source/WebCore/ChangeLog
Source/WebCore/dom/Document.cpp
Source/WebCore/dom/Document.h
Source/WebCore/html/HTMLCollection.cpp
Source/WebCore/html/HTMLCollection.h

index 60d5d05..626205f 100644 (file)
@@ -1,3 +1,21 @@
+2011-12-16  Andreas Kling  <kling@webkit.org>
+
+        Cache and reuse HTMLCollections exposed by Document.
+        <http://webkit.org/b/71956>
+
+        Reviewed by Antti Koivisto.
+
+        Added a test to verify that collections returned by document (excluding .all)
+        are equal to the collections returned by subsequent calls to the same getters.
+
+        Also update fast/dom/gc-9.html to cover the new lifetime behavior of
+        HTMLCollection objects returned by document.
+
+        * fast/dom/document-collection-idempotence-expected.txt: Added.
+        * fast/dom/document-collection-idempotence.html: Added.
+        * fast/dom/gc-9-expected.txt:
+        * fast/dom/gc-9.html:
+
 2011-12-16  Adrienne Walker  <enne@google.com>
 
         [chromium] Mark more worker regressions from r103095
diff --git a/LayoutTests/fast/dom/document-collection-idempotence-expected.txt b/LayoutTests/fast/dom/document-collection-idempotence-expected.txt
new file mode 100644 (file)
index 0000000..11e2be3
--- /dev/null
@@ -0,0 +1,18 @@
+This test verifies that the HTMLCollection getters on document (excluding .all) returns the same object when called repeatedly.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS document.all === document.all is false
+PASS document.images === document.images is true
+PASS document.embeds === document.embeds is true
+PASS document.plugins === document.plugins is true
+PASS document.applets === document.applets is true
+PASS document.links === document.links is true
+PASS document.forms === document.forms is true
+PASS document.anchors === document.anchors is true
+PASS document.scripts === document.scripts is true
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/dom/document-collection-idempotence.html b/LayoutTests/fast/dom/document-collection-idempotence.html
new file mode 100644 (file)
index 0000000..458707d
--- /dev/null
@@ -0,0 +1,24 @@
+<!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 the HTMLCollection getters on document (excluding .all) returns the same object when called repeatedly.");
+
+var collections = [ "images", "embeds", "plugins", "applets", "links", "forms", "anchors", "scripts" ];
+
+shouldBe("document.all === document.all", "false");
+
+for (i = 0; i < collections.length; ++i) {
+    var collection = collections[i];
+    shouldBe("document." + collection + " === document." + collection, "true");
+}
+
+</script>
+<script src="../js/resources/js-test-post.js"></script>
+</body>
+</html>
index 414e16f..cb5b42c 100644 (file)
@@ -16,13 +16,13 @@ PASS: document.getElementsByTagName('canvas')[0].getContext('2d').createPattern(
 PASS: document.getElementsByTagName('select')[0].options.myCustomProperty should be undefined and is.
 PASS: document.all.myCustomProperty should be undefined and is.
 PASS: document.body.childNodes.myCustomProperty should be undefined and is.
-PASS: document.images.myCustomProperty should be undefined and is.
-PASS: document.embeds.myCustomProperty should be undefined and is.
-PASS: document.applets.myCustomProperty should be undefined and is.
-PASS: document.links.myCustomProperty should be undefined and is.
-PASS: document.forms.myCustomProperty should be undefined and is.
-PASS: document.anchors.myCustomProperty should be undefined and is.
-PASS: document.scripts.myCustomProperty should be undefined and is.
+PASS: document.images.myCustomProperty should be 1 and is.
+PASS: document.embeds.myCustomProperty should be 1 and is.
+PASS: document.applets.myCustomProperty should be 1 and is.
+PASS: document.links.myCustomProperty should be 1 and is.
+PASS: document.forms.myCustomProperty should be 1 and is.
+PASS: document.anchors.myCustomProperty should be 1 and is.
+PASS: document.scripts.myCustomProperty should be 1 and is.
 PASS: document.getElementsByTagName('form')[0].elements.myCustomProperty should be undefined and is.
 PASS: document.getElementsByTagName('table')[0].rows.myCustomProperty should be undefined and is.
 PASS: document.getElementsByTagName('table')[0].rows[0].cells.myCustomProperty should be undefined and is.
@@ -52,13 +52,13 @@ PASS: document.getElementsByTagName('canvas')[0].getContext('2d').createPattern(
 PASS: document.getElementsByTagName('select')[0].options.myCustomProperty should be undefined and is.
 PASS: document.all.myCustomProperty should be undefined and is.
 PASS: document.body.childNodes.myCustomProperty should be undefined and is.
-PASS: document.images.myCustomProperty should be undefined and is.
-PASS: document.embeds.myCustomProperty should be undefined and is.
-PASS: document.applets.myCustomProperty should be undefined and is.
-PASS: document.links.myCustomProperty should be undefined and is.
-PASS: document.forms.myCustomProperty should be undefined and is.
-PASS: document.anchors.myCustomProperty should be undefined and is.
-PASS: document.scripts.myCustomProperty should be undefined and is.
+PASS: document.images.myCustomProperty should be 1 and is.
+PASS: document.embeds.myCustomProperty should be 1 and is.
+PASS: document.applets.myCustomProperty should be 1 and is.
+PASS: document.links.myCustomProperty should be 1 and is.
+PASS: document.forms.myCustomProperty should be 1 and is.
+PASS: document.anchors.myCustomProperty should be 1 and is.
+PASS: document.scripts.myCustomProperty should be 1 and is.
 PASS: document.getElementsByTagName('form')[0].elements.myCustomProperty should be undefined and is.
 PASS: document.getElementsByTagName('table')[0].rows.myCustomProperty should be undefined and is.
 PASS: document.getElementsByTagName('table')[0].rows[0].cells.myCustomProperty should be undefined and is.
index 3eda3ae..c8fd4dc 100644 (file)
@@ -126,13 +126,13 @@ var objectsToTest = [
     [ "document.all" ],
     [ "document.body.childNodes" ],
 
-    [ "document.images" ],
-    [ "document.embeds" ],
-    [ "document.applets" ],
-    [ "document.links" ],
-    [ "document.forms" ],
-    [ "document.anchors" ],
-    [ "document.scripts" ],
+    [ "document.images", "allow custom" ],
+    [ "document.embeds", "allow custom" ],
+    [ "document.applets", "allow custom" ],
+    [ "document.links", "allow custom" ],
+    [ "document.forms", "allow custom" ],
+    [ "document.anchors", "allow custom" ],
+    [ "document.scripts", "allow custom" ],
 
     [ "document.getElementsByTagName('form')[0].elements" ],
     [ "document.getElementsByTagName('table')[0].rows" ],
index e3bf1a4..95fcb36 100644 (file)
@@ -1,3 +1,54 @@
+2011-12-16  Andreas Kling  <kling@webkit.org>
+
+        Cache and reuse HTMLCollections exposed by Document.
+        <http://webkit.org/b/71956>
+
+        Reviewed by Antti Koivisto.
+
+        Let Document cache the various HTMLCollection objects it exposes.
+        This is a behavior change in two ways:
+
+        1) The lifetime of returned collections is now tied to the lifetime
+           of the Document. This matches the behavior of Firefox and Opera.
+
+        2) The cached collections returned by document are now exactly equal
+           to those returned by subsequent calls to the same getters.
+
+        This reduces memory consumption by ~800 kB (on 64-bit) when loading
+        the full HTML5 spec. document.links was called 34001 times, yielding
+        34001 separate HTMLCollections, and now we only need 1.
+
+        The document.all collection retains the old behavior, as caching it
+        will be a bit more complicated.
+
+        To avoid a reference cycle between Document and HTMLCollection,
+        collections that are cached on Document do not retained their base
+        node pointer (controlled by a m_baseIsRetained flag.)
+
+        Tests: fast/dom/document-collection-idempotence.html
+               fast/dom/gc-9.html
+
+        * dom/Document.cpp:
+        (WebCore::Document::detach):
+        (WebCore::Document::cachedCollection):
+        (WebCore::Document::images):
+        (WebCore::Document::applets):
+        (WebCore::Document::embeds):
+        (WebCore::Document::plugins):
+        (WebCore::Document::objects):
+        (WebCore::Document::scripts):
+        (WebCore::Document::links):
+        (WebCore::Document::forms):
+        (WebCore::Document::anchors):
+        * dom/Document.h:
+        * html/HTMLCollection.cpp:
+        (WebCore::HTMLCollection::HTMLCollection):
+        (WebCore::HTMLCollection::createForCachingOnDocument):
+        (WebCore::HTMLCollection::~HTMLCollection):
+        (WebCore::HTMLCollection::itemAfter):
+        * html/HTMLCollection.h:
+        (WebCore::HTMLCollection::base):
+
 2011-12-16  Anders Carlsson  <andersca@apple.com>
 
         Add a pretty dumb tile cache to WebTileCacheLayer
index df4b57a..4627dce 100644 (file)
@@ -4190,50 +4190,58 @@ bool Document::hasSVGRootNode() const
 }
 #endif
 
+const RefPtr<HTMLCollection>& Document::cachedCollection(CollectionType type)
+{
+    ASSERT(type < NumUnnamedDocumentCachedTypes);
+    if (!m_collections[type])
+        m_collections[type] = HTMLCollection::createForCachingOnDocument(this, type);
+    return m_collections[type];
+}
+
 PassRefPtr<HTMLCollection> Document::images()
 {
-    return HTMLCollection::create(this, DocImages);
+    return cachedCollection(DocImages);
 }
 
 PassRefPtr<HTMLCollection> Document::applets()
 {
-    return HTMLCollection::create(this, DocApplets);
+    return cachedCollection(DocApplets);
 }
 
 PassRefPtr<HTMLCollection> Document::embeds()
 {
-    return HTMLCollection::create(this, DocEmbeds);
+    return cachedCollection(DocEmbeds);
 }
 
 PassRefPtr<HTMLCollection> Document::plugins()
 {
     // This is an alias for embeds() required for the JS DOM bindings.
-    return HTMLCollection::create(this, DocEmbeds);
+    return cachedCollection(DocEmbeds);
 }
 
 PassRefPtr<HTMLCollection> Document::objects()
 {
-    return HTMLCollection::create(this, DocObjects);
+    return cachedCollection(DocObjects);
 }
 
 PassRefPtr<HTMLCollection> Document::scripts()
 {
-    return HTMLCollection::create(this, DocScripts);
+    return cachedCollection(DocScripts);
 }
 
 PassRefPtr<HTMLCollection> Document::links()
 {
-    return HTMLCollection::create(this, DocLinks);
+    return cachedCollection(DocLinks);
 }
 
 PassRefPtr<HTMLCollection> Document::forms()
 {
-    return HTMLCollection::create(this, DocForms);
+    return cachedCollection(DocForms);
 }
 
 PassRefPtr<HTMLCollection> Document::anchors()
 {
-    return HTMLCollection::create(this, DocAnchors);
+    return cachedCollection(DocAnchors);
 }
 
 PassRefPtr<HTMLAllCollection> Document::all()
index d29225b..f617f59 100644 (file)
@@ -1185,6 +1185,8 @@ private:
     PageVisibilityState visibilityState() const;
 #endif
 
+    const RefPtr<HTMLCollection>& cachedCollection(CollectionType);
+
     int m_guardRefCount;
 
     OwnPtr<CSSStyleSelector> m_styleSelector;
@@ -1365,6 +1367,8 @@ private:
     
     CheckedRadioButtons m_checkedRadioButtons;
 
+    RefPtr<HTMLCollection> m_collections[NumUnnamedDocumentCachedTypes];
+
     typedef HashMap<AtomicStringImpl*, CollectionCache*> NamedCollectionMap;
     FixedArray<CollectionCache, NumUnnamedDocumentCachedTypes> m_collectionInfo;
     FixedArray<NamedCollectionMap, NumNamedDocumentCachedTypes> m_nameCollectionInfo;
index 3f2d0e1..9e31863 100644 (file)
@@ -36,20 +36,28 @@ namespace WebCore {
 
 using namespace HTMLNames;
 
-HTMLCollection::HTMLCollection(PassRefPtr<Node> base, CollectionType type)
-    : m_ownsInfo(false)
+HTMLCollection::HTMLCollection(Document* document, CollectionType type)
+    : m_baseIsRetained(false)
+    , m_ownsInfo(false)
     , m_type(type)
-    , m_base(base)
-    , m_info(m_base->isDocumentNode() ? static_cast<Document*>(m_base.get())->collectionInfo(type) : 0)
+    , m_base(document)
+    , m_info(document->collectionInfo(type))
 {
 }
 
 HTMLCollection::HTMLCollection(PassRefPtr<Node> base, CollectionType type, CollectionCache* info)
-    : m_ownsInfo(false)
+    : m_baseIsRetained(true)
+    , m_ownsInfo(false)
     , m_type(type)
-    , m_base(base)
+    , m_base(base.get())
     , m_info(info)
 {
+    m_base->ref();
+}
+
+PassRefPtr<HTMLCollection> HTMLCollection::createForCachingOnDocument(Document* document, CollectionType type)
+{
+    return adoptRef(new HTMLCollection(document, type));
 }
 
 PassRefPtr<HTMLCollection> HTMLCollection::create(PassRefPtr<Node> base, CollectionType type)
@@ -61,6 +69,8 @@ HTMLCollection::~HTMLCollection()
 {
     if (m_ownsInfo)
         delete m_info;
+    if (m_baseIsRetained)
+        m_base->deref();
 }
 
 void HTMLCollection::resetCollectionInfo() const
@@ -121,9 +131,9 @@ Element* HTMLCollection::itemAfter(Element* previous) const
     if (!previous)
         current = m_base->firstChild();
     else
-        current = nextNodeOrSibling(m_base.get(), previous, deep);
+        current = nextNodeOrSibling(m_base, previous, deep);
 
-    for (; current; current = nextNodeOrSibling(m_base.get(), current, deep)) {
+    for (; current; current = nextNodeOrSibling(m_base, current, deep)) {
         if (!current->isElementNode())
             continue;
         Element* e = static_cast<Element*>(current);
index 8522f5b..f9db634 100644 (file)
@@ -31,6 +31,7 @@
 
 namespace WebCore {
 
+class Document;
 class Element;
 class Node;
 class NodeList;
@@ -40,6 +41,7 @@ struct CollectionCache;
 class HTMLCollection : public RefCounted<HTMLCollection> {
 public:
     static PassRefPtr<HTMLCollection> create(PassRefPtr<Node> base, CollectionType);
+    static PassRefPtr<HTMLCollection> createForCachingOnDocument(Document*, CollectionType);
     virtual ~HTMLCollection();
     
     unsigned length() const;
@@ -55,12 +57,12 @@ public:
 
     PassRefPtr<NodeList> tags(const String&);
 
-    Node* base() const { return m_base.get(); }
+    Node* base() const { return m_base; }
     CollectionType type() const { return static_cast<CollectionType>(m_type); }
 
 protected:
-    HTMLCollection(PassRefPtr<Node> base, CollectionType, CollectionCache*);
-    HTMLCollection(PassRefPtr<Node> base, CollectionType);
+    HTMLCollection(PassRefPtr<Node> base, CollectionType, CollectionCache* = 0);
+    HTMLCollection(Document*, CollectionType);
 
     CollectionCache* info() const { return m_info; }
     void resetCollectionInfo() const;
@@ -72,10 +74,11 @@ private:
     virtual unsigned calcLength() const;
     virtual void updateNameCache() const;
 
+    bool m_baseIsRetained : 1;
     mutable bool m_ownsInfo : 1;
     unsigned m_type : 5; // CollectionType
 
-    RefPtr<Node> m_base;
+    Node* m_base;
 
     mutable CollectionCache* m_info;
 };