Changeset 117353 in webkit

Timestamp:

May 16, 2012 3:25:10 PM (12 years ago)

Author:

ojan@chromium.org

Message:

Fix perf regression from r116487
​https://bugs.webkit.org/show_bug.cgi?id=86680

Reviewed by Ryosuke Niwa.

​http://trac.webkit.org/changeset/116487 caused a 6% regression on
Dromaeo's dom-attr test. The issue is that we invalidated NodeList
caches whenever an id/checked/type attribute changed.

First, we don't need to invalidate on checked/type since that only
affects the values return by NodeList items, not the list of items.
Second, we only need to invalidate NodeList caches when an id attribute
changes on a FormControlElement.

Incidentally, we also don't need to invalidate caches for changes
to attributes that don't have an ownerElement.

No new tests. This is strictly a performance improvement.

• dom/Attr.cpp:

(WebCore::Attr::setValue):
(WebCore::Attr::childrenChanged):

• dom/Element.cpp:

(WebCore::Element::attributeChanged):

• dom/Node.cpp:

(WebCore::Node::invalidateNodeListsCacheAfterAttributeChanged):

• dom/Node.h:

(Node):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• dom/Attr.cpp (modified) (2 diffs)
• dom/Element.cpp (modified) (1 diff)
• dom/Node.cpp (modified) (3 diffs)
• dom/Node.h (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2012-05-16  Ojan Vafai  <ojan@chromium.org>
+
+        Fix perf regression from r116487
+        https://bugs.webkit.org/show_bug.cgi?id=86680
+
+        Reviewed by Ryosuke Niwa.
+
+        http://trac.webkit.org/changeset/116487 caused a 6% regression on
+        Dromaeo's dom-attr test. The issue is that we invalidated NodeList
+        caches whenever an id/checked/type attribute changed.
+
+        First, we don't need to invalidate on checked/type since that only
+        affects the values return by NodeList items, not the list of items.
+        Second, we only need to invalidate NodeList caches when an id attribute
+        changes on a FormControlElement.
+
+        Incidentally, we also don't need to invalidate caches for changes
+        to attributes that don't have an ownerElement.
+
+        No new tests. This is strictly a performance improvement.
+
+        * dom/Attr.cpp:
+        (WebCore::Attr::setValue):
+        (WebCore::Attr::childrenChanged):
+        * dom/Element.cpp:
+        (WebCore::Element::attributeChanged):
+        * dom/Node.cpp:
+        (WebCore::Node::invalidateNodeListsCacheAfterAttributeChanged):
+        * dom/Node.h:
+        (Node):
+
 2012-04-22  Robert Hogan  <robert@webkit.org>
 

trunk/Source/WebCore/dom/Attr.cpp

@@ -120 +120 @@
     m_ignoreChildrenChanged--;
 
-    invalidateNodeListsCacheAfterAttributeChanged(m_name);
+    invalidateNodeListsCacheAfterAttributeChanged(m_name, m_element);
 }
 
@@ -163 +163 @@
         return;
 
-    invalidateNodeListsCacheAfterAttributeChanged(qualifiedName());
+    invalidateNodeListsCacheAfterAttributeChanged(qualifiedName(), m_element);
 
     // FIXME: We should include entity references in the value

trunk/Source/WebCore/dom/Element.cpp

@@ -717 +717 @@
     }
 
-    invalidateNodeListsCacheAfterAttributeChanged(attribute.name());
+    invalidateNodeListsCacheAfterAttributeChanged(attribute.name(), this);
 
     if (!AXObjectCache::accessibilityEnabled())

trunk/Source/WebCore/dom/Node.cpp

@@ -977 +977 @@
 }
 
-void Node::invalidateNodeListsCacheAfterAttributeChanged(const QualifiedName& attrName)
+void Node::invalidateNodeListsCacheAfterAttributeChanged(const QualifiedName& attrName, Element* attributeOwnerElement)
 {
     if (hasRareData() && isAttributeNode()) {
@@ -985 +985 @@
     }
 
-    // This list should be sync'ed with NodeListsNodeData.
+    // Modifications to attributes that are not associated with an Element can't invalidate NodeList caches.
+    if (!attributeOwnerElement)
+        return;
+
+    // FIXME: Move the list of attributes each NodeList type cares about to be a static on the
+    // appropriate NodeList class. Then use those lists here and in invalidateCachesThatDependOnAttributes
+    // to only invalidate the cache types that depend on the attribute that changed.
+    // FIXME: Keep track of when we have no caches of a given type so that we can avoid the for-loop
+    // below even if a related attribute changed (e.g. if we have no RadioNodeLists, we don't need
+    // to invalidate any caches when id attributes change.)
     if (attrName != classAttr
 #if ENABLE(MICRODATA)
@@ -992 +1001 @@
         && attrName != itemtypeAttr
 #endif
-        && attrName != idAttr
-        && attrName != typeAttr
-        && attrName != checkedAttr
         && attrName != nameAttr
-        && attrName != forAttr)
+        && attrName != forAttr
+        && (attrName != idAttr || !attributeOwnerElement->isFormControlElement()))
         return;
 

trunk/Source/WebCore/dom/Node.h

@@ -558 +558 @@
     void registerDynamicSubtreeNodeList(DynamicSubtreeNodeList*);
     void unregisterDynamicSubtreeNodeList(DynamicSubtreeNodeList*);
-    void invalidateNodeListsCacheAfterAttributeChanged(const QualifiedName&);
+    void invalidateNodeListsCacheAfterAttributeChanged(const QualifiedName&, Element* attributeOwnerElement);
     void invalidateNodeListsCacheAfterChildrenChanged();
     void removeCachedClassNodeList(ClassNodeList*, const String&);