Avoid unnecessary HTML Collection invalidations for id and name attribute changes
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 26 Feb 2014 08:26:05 +0000 (08:26 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 26 Feb 2014 08:26:05 +0000 (08:26 +0000)
https://bugs.webkit.org/show_bug.cgi?id=129361

Reviewed by Benjamin Poulain.

Before this patch, setting id and name attributes resulted in traversing all the ancestors to invalidate
HTML collections on those nodes whenever we had more than one HTMLCollection alive.

Avoid the traversal when HTMLCollections don't have any valid id and name map caches by making each
HTMLCollection explicitly call collectionCachedIdNameMap and collectionWillClearIdNameMap when it caches
or clears the id and name map.

Inspired by https://chromium.googlesource.com/chromium/blink/+/5b06b91b79098f7d42e480f85be32198315d2440

* dom/Document.cpp:
(WebCore::Document::registerCollection): Takes a boolean to indicate whether collection has a valid cache
for the id and name map.
(WebCore::Document::unregisterCollection): Ditto.
(WebCore::Document::collectionCachedIdNameMap): Added.
(WebCore::Document::collectionWillClearIdNameMap): Added.
* dom/Document.h:

* dom/NodeRareData.h:
(WebCore::NodeListsNodeData::adoptDocument): Call invalidateCache on HTML collections after, not before,
calling unregisterCollection and registerCollection since collections' owner nodes have already been
moved to the new document here and invalidateCache uses owner node's document to call
collectionWillClearIdNameMap. So calling invalidateCache before calling unregister/registerCollection
would result in collectionWillClearIdNameMap getting called on a wrong document.

* html/HTMLCollection.cpp:
(WebCore::HTMLCollection::HTMLCollection):
(WebCore::HTMLCollection::~HTMLCollection):
(WebCore::HTMLCollection::invalidateCache):
(WebCore::HTMLCollection::invalidateIdNameCacheMaps): Added the code to uncount itself from the number
of live node lists and HTML collections that need to be invalidated upon id and name attribute changes.
(WebCore::HTMLCollection::updateNameCache):

* html/HTMLCollection.h:
(WebCore::HTMLCollection::invalidateCache):
(WebCore::HTMLCollection::hasIdNameCache): Renamed from hasNameCache.
(WebCore::HTMLCollection::setHasIdNameCache): Renamed from setHasIdNameCache.

* html/HTMLFormControlsCollection.cpp:
(WebCore::HTMLFormControlsCollection::updateNameCache):

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

Source/WebCore/ChangeLog
Source/WebCore/dom/Document.cpp
Source/WebCore/dom/Document.h
Source/WebCore/dom/NodeRareData.h
Source/WebCore/html/HTMLCollection.cpp
Source/WebCore/html/HTMLCollection.h
Source/WebCore/html/HTMLFormControlsCollection.cpp

index 1250d6bb495b950afc44d0ac92485b8ec92c027c..48c1e2600494769b44ede7ec838c158c8c87d67d 100644 (file)
@@ -1,3 +1,50 @@
+2014-02-26  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Avoid unnecessary HTML Collection invalidations for id and name attribute changes
+        https://bugs.webkit.org/show_bug.cgi?id=129361
+
+        Reviewed by Benjamin Poulain.
+
+        Before this patch, setting id and name attributes resulted in traversing all the ancestors to invalidate
+        HTML collections on those nodes whenever we had more than one HTMLCollection alive.
+
+        Avoid the traversal when HTMLCollections don't have any valid id and name map caches by making each
+        HTMLCollection explicitly call collectionCachedIdNameMap and collectionWillClearIdNameMap when it caches
+        or clears the id and name map.
+
+        Inspired by https://chromium.googlesource.com/chromium/blink/+/5b06b91b79098f7d42e480f85be32198315d2440
+
+        * dom/Document.cpp:
+        (WebCore::Document::registerCollection): Takes a boolean to indicate whether collection has a valid cache
+        for the id and name map.
+        (WebCore::Document::unregisterCollection): Ditto.
+        (WebCore::Document::collectionCachedIdNameMap): Added.
+        (WebCore::Document::collectionWillClearIdNameMap): Added.
+        * dom/Document.h:
+
+        * dom/NodeRareData.h:
+        (WebCore::NodeListsNodeData::adoptDocument): Call invalidateCache on HTML collections after, not before,
+        calling unregisterCollection and registerCollection since collections' owner nodes have already been
+        moved to the new document here and invalidateCache uses owner node's document to call
+        collectionWillClearIdNameMap. So calling invalidateCache before calling unregister/registerCollection
+        would result in collectionWillClearIdNameMap getting called on a wrong document.
+
+        * html/HTMLCollection.cpp:
+        (WebCore::HTMLCollection::HTMLCollection):
+        (WebCore::HTMLCollection::~HTMLCollection):
+        (WebCore::HTMLCollection::invalidateCache):
+        (WebCore::HTMLCollection::invalidateIdNameCacheMaps): Added the code to uncount itself from the number
+        of live node lists and HTML collections that need to be invalidated upon id and name attribute changes.
+        (WebCore::HTMLCollection::updateNameCache):
+
+        * html/HTMLCollection.h:
+        (WebCore::HTMLCollection::invalidateCache):
+        (WebCore::HTMLCollection::hasIdNameCache): Renamed from hasNameCache.
+        (WebCore::HTMLCollection::setHasIdNameCache): Renamed from setHasIdNameCache.
+
+        * html/HTMLFormControlsCollection.cpp:
+        (WebCore::HTMLFormControlsCollection::updateNameCache):
+
 2014-02-25  Frédéric Wang  <fred.wang@free.fr>
 
         Add support for minsize/maxsize attributes.
index fa6a85fc84fe71e6d3af9e70190c9be4bf63ba5d..830275c0567724ed4247f1809a95b14e53462c35 100644 (file)
@@ -3450,17 +3450,20 @@ void Document::unregisterNodeList(LiveNodeList& list)
     }
 }
 
-void Document::registerCollection(HTMLCollection& collection)
+void Document::registerCollection(HTMLCollection& collection, bool hasIdNameMap)
 {
-    m_nodeListAndCollectionCounts[InvalidateOnIdNameAttrChange]++;
+    if (hasIdNameMap)
+        collectionCachedIdNameMap(collection);
     m_nodeListAndCollectionCounts[collection.invalidationType()]++;
     if (collection.isRootedAtDocument())
         m_collectionsInvalidatedAtDocument.add(&collection);
 }
 
-void Document::unregisterCollection(HTMLCollection& collection)
+void Document::unregisterCollection(HTMLCollection& collection, bool hasIdNameMap)
 {
-    m_nodeListAndCollectionCounts[InvalidateOnIdNameAttrChange]--;
+    if (hasIdNameMap)
+        collectionWillClearIdNameMap(collection);
+    ASSERT(m_nodeListAndCollectionCounts[collection.invalidationType()]);
     m_nodeListAndCollectionCounts[collection.invalidationType()]--;
     if (collection.isRootedAtDocument()) {
         ASSERT(m_collectionsInvalidatedAtDocument.contains(&collection));
@@ -3468,6 +3471,19 @@ void Document::unregisterCollection(HTMLCollection& collection)
     }
 }
 
+void Document::collectionCachedIdNameMap(const HTMLCollection& collection)
+{
+    ASSERT_UNUSED(collection, collection.hasIdNameCache());
+    m_nodeListAndCollectionCounts[InvalidateOnIdNameAttrChange]++;
+}
+
+void Document::collectionWillClearIdNameMap(const HTMLCollection& collection)
+{
+    ASSERT_UNUSED(collection, collection.hasIdNameCache());
+    ASSERT(m_nodeListAndCollectionCounts[InvalidateOnIdNameAttrChange]);
+    m_nodeListAndCollectionCounts[InvalidateOnIdNameAttrChange]--;
+}
+
 void Document::attachNodeIterator(NodeIterator* ni)
 {
     m_nodeIterators.add(ni);
index 8419e98ba9950cd1d95934d07a8262b7c390a011..b38b9cb3b79710daaa5ea68cd99cb2b1efb71706 100644 (file)
@@ -758,8 +758,10 @@ public:
 
     void registerNodeList(LiveNodeList&);
     void unregisterNodeList(LiveNodeList&);
-    void registerCollection(HTMLCollection&);
-    void unregisterCollection(HTMLCollection&);
+    void registerCollection(HTMLCollection&, bool hasIdNameCache);
+    void unregisterCollection(HTMLCollection&, bool hasIdNameCache);
+    void collectionCachedIdNameMap(const HTMLCollection&);
+    void collectionWillClearIdNameMap(const HTMLCollection&);
     bool shouldInvalidateNodeListAndCollectionCaches(const QualifiedName* attrName = nullptr) const;
     void invalidateNodeListAndCollectionCaches(const QualifiedName* attrName);
 
index a0d65a3baa6a604ad4049b313e9b87affc8cfa79..9dab21115634482cdb054df343a273a807047298 100644 (file)
@@ -225,33 +225,38 @@ public:
 
     void adoptDocument(Document* oldDocument, Document* newDocument)
     {
-        invalidateCaches();
+        if (oldDocument == newDocument) {
+            invalidateCaches();
+            return;
+        }
+
+        for (auto it : m_atomicNameCaches) {
+            LiveNodeList& list = *it.value;
+            oldDocument->unregisterNodeList(list);
+            newDocument->registerNodeList(list);
+            list.invalidateCache();
+        }
+
+        for (auto it : m_nameCaches) {
+            LiveNodeList& list = *it.value;
+            oldDocument->unregisterNodeList(list);
+            newDocument->registerNodeList(list);
+            list.invalidateCache();
+        }
+
+        for (auto it : m_tagNodeListCacheNS) {
+            LiveNodeList& list = *it.value;
+            ASSERT(!list.isRootedAtDocument());
+            oldDocument->unregisterNodeList(list);
+            newDocument->registerNodeList(list);
+            list.invalidateCache();
+        }
 
-        if (oldDocument != newDocument) {
-            for (auto it = m_atomicNameCaches.begin(), end = m_atomicNameCaches.end(); it != end; ++it) {
-                LiveNodeList& list = *it->value;
-                oldDocument->unregisterNodeList(list);
-                newDocument->registerNodeList(list);
-            }
-
-            for (auto it = m_nameCaches.begin(), end = m_nameCaches.end(); it != end; ++it) {
-                LiveNodeList& list = *it->value;
-                oldDocument->unregisterNodeList(list);
-                newDocument->registerNodeList(list);
-            }
-
-            for (auto it = m_tagNodeListCacheNS.begin(), end = m_tagNodeListCacheNS.end(); it != end; ++it) {
-                LiveNodeList& list = *it->value;
-                ASSERT(!list.isRootedAtDocument());
-                oldDocument->unregisterNodeList(list);
-                newDocument->registerNodeList(list);
-            }
-
-            for (auto it = m_cachedCollections.begin(), end = m_cachedCollections.end(); it != end; ++it) {
-                HTMLCollection& collection = *it->value;
-                oldDocument->unregisterCollection(collection);
-                newDocument->registerCollection(collection);
-            }
+        for (auto it : m_cachedCollections) {
+            HTMLCollection& collection = *it.value;
+            oldDocument->unregisterCollection(collection, collection.hasIdNameCache());
+            newDocument->registerCollection(collection, collection.hasIdNameCache());
+            collection.invalidateCache();
         }
     }
 
index 58a9eeca27f8459ca6836c60ffbb325d906c9078..f339c25ee82aa63ab2483c5196e8ed4375d20908 100644 (file)
@@ -145,7 +145,7 @@ HTMLCollection::HTMLCollection(ContainerNode& ownerNode, CollectionType type, El
     ASSERT(m_invalidationType == static_cast<unsigned>(invalidationTypeExcludingIdAndNameAttributes(type)));
     ASSERT(m_collectionType == static_cast<unsigned>(type));
 
-    document().registerCollection(*this);
+    document().registerCollection(*this, hasIdNameCache());
 }
 
 PassRefPtr<HTMLCollection> HTMLCollection::create(ContainerNode& base, CollectionType type)
@@ -155,7 +155,7 @@ PassRefPtr<HTMLCollection> HTMLCollection::create(ContainerNode& base, Collectio
 
 HTMLCollection::~HTMLCollection()
 {
-    document().unregisterCollection(*this);
+    document().unregisterCollection(*this, hasIdNameCache());
     // HTMLNameCollection removes cache by itself.
     if (type() != WindowNamedItems && type() != DocumentNamedItems)
         ownerNode().nodeLists()->removeCachedCollection(this);
@@ -367,14 +367,16 @@ Element* HTMLCollection::collectionTraverseBackward(Element& current, unsigned c
 void HTMLCollection::invalidateCache() const
 {
     m_indexCache.invalidate();
-    m_isNameCacheValid = false;
     m_isItemRefElementsCacheValid = false;
-    m_idCache.clear();
-    m_nameCache.clear();
+    if (hasIdNameCache())
+        invalidateIdNameCacheMaps();
 }
 
 void HTMLCollection::invalidateIdNameCacheMaps() const
 {
+    ASSERT(hasIdNameCache());
+    document().collectionWillClearIdNameMap(*this);
+    m_isNameCacheValid = false;
     m_idCache.clear();
     m_nameCache.clear();
 }
@@ -429,7 +431,7 @@ Node* HTMLCollection::namedItem(const AtomicString& name) const
 
 void HTMLCollection::updateNameCache() const
 {
-    if (hasNameCache())
+    if (hasIdNameCache())
         return;
 
     ContainerNode& root = rootNode();
@@ -446,7 +448,7 @@ void HTMLCollection::updateNameCache() const
             appendNameCache(nameAttrVal, element);
     }
 
-    setHasNameCache();
+    setHasIdNameCache();
 }
 
 bool HTMLCollection::hasNamedItem(const AtomicString& name) const
index e3e44beb6cc5feea8f4027056e3314641dcb72b4..7e2379998a6e9c5c794e829761941ad8a10a86e2 100644 (file)
@@ -61,11 +61,10 @@ public:
     {
         if (!attrName || shouldInvalidateTypeOnAttributeChange(invalidationType(), *attrName))
             invalidateCache();
-        else if (*attrName == HTMLNames::idAttr || *attrName == HTMLNames::nameAttr)
+        else if (hasIdNameCache() && (*attrName == HTMLNames::idAttr || *attrName == HTMLNames::nameAttr))
             invalidateIdNameCacheMaps();
     }
     virtual void invalidateCache() const;
-    void invalidateIdNameCacheMaps() const;
 
     // For CollectionIndexCache
     Element* collectionFirst() const;
@@ -74,10 +73,13 @@ public:
     Element* collectionTraverseBackward(Element&, unsigned count) const;
     bool collectionCanTraverseBackward() const { return !m_usesCustomForwardOnlyTraversal; }
 
+    bool hasIdNameCache() const { return m_isNameCacheValid; }
+
 protected:
     enum ElementTraversalType { NormalTraversal, CustomForwardOnlyTraversal };
     HTMLCollection(ContainerNode& base, CollectionType, ElementTraversalType = NormalTraversal);
 
+    void invalidateIdNameCacheMaps() const;
     virtual void updateNameCache() const;
 
     typedef HashMap<AtomicStringImpl*, OwnPtr<Vector<Element*>>> NodeCacheMap;
@@ -95,8 +97,11 @@ protected:
 
     NodeListRootType rootType() const { return static_cast<NodeListRootType>(m_rootType); }
 
-    bool hasNameCache() const { return m_isNameCacheValid; }
-    void setHasNameCache() const { m_isNameCacheValid = true; }
+    void setHasIdNameCache() const
+    {
+        m_isNameCacheValid = true;
+        document().collectionCachedIdNameMap(*this);
+    }
 
 private:
     static void append(NodeCacheMap&, const AtomicString&, Element*);
index 54587306c85308ac5d2f7892d1ff8fd706c45462..313ce9aebc6642a9fba1709c612cbd3336af513f 100644 (file)
@@ -137,7 +137,7 @@ Node* HTMLFormControlsCollection::namedItem(const AtomicString& name) const
 
 void HTMLFormControlsCollection::updateNameCache() const
 {
-    if (hasNameCache())
+    if (hasIdNameCache())
         return;
 
     HashSet<AtomicStringImpl*> foundInputElements;
@@ -174,7 +174,7 @@ void HTMLFormControlsCollection::updateNameCache() const
         }
     }
 
-    setHasNameCache();
+    setHasIdNameCache();
 }
 
 void HTMLFormControlsCollection::invalidateCache() const