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 1250d6b..48c1e26 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 fa6a85f..830275c 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 8419e98..b38b9cb 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 a0d65a3..9dab211 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 58a9eec..f339c25 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 e3e44be..7e23799 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 5458730..313ce9a 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