Title: [87147] trunk/Source/WebCore
- Revision
- 87147
- Author
- an...@apple.com
- Date
- 2011-05-24 07:48:24 -0700 (Tue, 24 May 2011)
Log Message
REGRESSION (r45620): Node list caches never deleted
https://bugs.webkit.org/show_bug.cgi?id=61268
<rdar://problem/9467379>
Reviewed by Oliver Hunt.
NodeListsNodeData::isEmpty() tests if RefCounted objects have refcount of zero which is impossible.
As a results NodeList caches are never deleted, causing bad performance in DOM mutating operations as
they repeatedly invalidate caches.
* dom/Node.cpp:
(WebCore::Node::childNodes):
Construct m_childNodeListCaches lazily.
(WebCore::Node::unregisterDynamicNodeList):
(WebCore::Node::notifyLocalNodeListsAttributeChanged):
(WebCore::Node::notifyLocalNodeListsChildrenChanged):
(WebCore::Node::removeNodeListCacheIfPossible):
Add a helper.
(WebCore::NodeListsNodeData::invalidateCaches):
Invalidate m_childNodeListCaches by clearing it if there are no additional clients
(WebCore::NodeListsNodeData::isEmpty):
Test emptiness of various NodeListCaches simply by testing hash emptiness instead of testing for non-zero ref count of items.
m_childNodeListCaches is empty if it is null.
* dom/Node.h:
* dom/NodeRareData.h:
(WebCore::NodeListsNodeData::NodeListsNodeData):
Construct m_childNodeListCaches lazily.
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (87146 => 87147)
--- trunk/Source/WebCore/ChangeLog 2011-05-24 14:17:10 UTC (rev 87146)
+++ trunk/Source/WebCore/ChangeLog 2011-05-24 14:48:24 UTC (rev 87147)
@@ -1,3 +1,37 @@
+2011-05-23 Antti Koivisto <an...@apple.com>
+
+ Reviewed by Oliver Hunt.
+
+ REGRESSION (r45620): Node list caches never deleted
+ https://bugs.webkit.org/show_bug.cgi?id=61268
+ <rdar://problem/9467379>
+
+ NodeListsNodeData::isEmpty() tests if RefCounted objects have refcount of zero which is impossible.
+ As a results NodeList caches are never deleted, causing bad performance in DOM mutating operations as
+ they repeatedly invalidate caches.
+
+ * dom/Node.cpp:
+ (WebCore::Node::childNodes):
+ Construct m_childNodeListCaches lazily.
+
+ (WebCore::Node::unregisterDynamicNodeList):
+ (WebCore::Node::notifyLocalNodeListsAttributeChanged):
+ (WebCore::Node::notifyLocalNodeListsChildrenChanged):
+ (WebCore::Node::removeNodeListCacheIfPossible):
+ Add a helper.
+
+ (WebCore::NodeListsNodeData::invalidateCaches):
+ Invalidate m_childNodeListCaches by clearing it if there are no additional clients
+
+ (WebCore::NodeListsNodeData::isEmpty):
+ Test emptiness of various NodeListCaches simply by testing hash emptiness instead of testing for non-zero ref count of items.
+ m_childNodeListCaches is empty if it is null.
+
+ * dom/Node.h:
+ * dom/NodeRareData.h:
+ (WebCore::NodeListsNodeData::NodeListsNodeData):
+ Construct m_childNodeListCaches lazily.
+
2011-05-24 Mikhail Naganov <mnaga...@chromium.org>
Reviewed by Yury Semikhatsky.
Modified: trunk/Source/WebCore/dom/Node.cpp (87146 => 87147)
--- trunk/Source/WebCore/dom/Node.cpp 2011-05-24 14:17:10 UTC (rev 87146)
+++ trunk/Source/WebCore/dom/Node.cpp 2011-05-24 14:48:24 UTC (rev 87147)
@@ -610,6 +610,8 @@
treeScope()->addNodeListCache();
}
+ if (!data->nodeLists()->m_childNodeListCaches)
+ data->nodeLists()->m_childNodeListCaches = DynamicNodeList::Caches::create();
return ChildNodeList::create(this, data->nodeLists()->m_childNodeListCaches.get());
}
@@ -991,15 +993,11 @@
if (list->hasOwnCaches()) {
NodeRareData* data = ""
data->nodeLists()->m_listsWithCaches.remove(list);
- if (data->nodeLists()->isEmpty()) {
- data->clearNodeLists();
- if (treeScope())
- treeScope()->removeNodeListCache();
- }
+ removeNodeListCacheIfPossible();
}
}
-void Node::notifyLocalNodeListsAttributeChanged()
+inline void Node::notifyLocalNodeListsAttributeChanged()
{
if (!hasRareData())
return;
@@ -1012,10 +1010,7 @@
else
data->nodeLists()->invalidateCaches();
- if (data->nodeLists()->isEmpty()) {
- data->clearNodeLists();
- treeScope()->removeNodeListCache();
- }
+ removeNodeListCacheIfPossible();
}
void Node::notifyNodeListsAttributeChanged()
@@ -1024,7 +1019,7 @@
n->notifyLocalNodeListsAttributeChanged();
}
-void Node::notifyLocalNodeListsChildrenChanged()
+inline void Node::notifyLocalNodeListsChildrenChanged()
{
if (!hasRareData())
return;
@@ -1038,12 +1033,20 @@
for (NodeListsNodeData::NodeListSet::iterator i = data->nodeLists()->m_listsWithCaches.begin(); i != end; ++i)
(*i)->invalidateCache();
- if (data->nodeLists()->isEmpty()) {
- data->clearNodeLists();
- treeScope()->removeNodeListCache();
- }
+ removeNodeListCacheIfPossible();
}
+
+void Node::removeNodeListCacheIfPossible()
+{
+ ASSERT(rareData()->nodeLists());
+ NodeRareData* data = ""
+ if (!data->nodeLists()->isEmpty())
+ return;
+ data->clearNodeLists();
+ treeScope()->removeNodeListCache();
+}
+
void Node::notifyNodeListsChildrenChanged()
{
for (Node* n = this; n; n = n->parentNode())
@@ -2378,7 +2381,12 @@
void NodeListsNodeData::invalidateCaches()
{
- m_childNodeListCaches->reset();
+ if (m_childNodeListCaches) {
+ if (m_childNodeListCaches->hasOneRef())
+ m_childNodeListCaches.clear();
+ else
+ m_childNodeListCaches->reset();
+ }
if (m_labelsNodeListCache)
m_labelsNodeListCache->invalidateCache();
@@ -2409,33 +2417,18 @@
if (!m_listsWithCaches.isEmpty())
return false;
- if (m_childNodeListCaches->refCount())
+ if (m_childNodeListCaches)
return false;
-
- TagNodeListCache::const_iterator tagCacheEnd = m_tagNodeListCache.end();
- for (TagNodeListCache::const_iterator it = m_tagNodeListCache.begin(); it != tagCacheEnd; ++it) {
- if (it->second->refCount())
- return false;
- }
- TagNodeListCacheNS::const_iterator tagCacheNSEnd = m_tagNodeListCacheNS.end();
- for (TagNodeListCacheNS::const_iterator it = m_tagNodeListCacheNS.begin(); it != tagCacheNSEnd; ++it) {
- if (it->second->refCount())
- return false;
- }
+ if (!m_tagNodeListCache.isEmpty())
+ return false;
+ if (!m_tagNodeListCacheNS.isEmpty())
+ return false;
+ if (!m_classNodeListCache.isEmpty())
+ return false;
+ if (!m_nameNodeListCache.isEmpty())
+ return false;
- ClassNodeListCache::const_iterator classCacheEnd = m_classNodeListCache.end();
- for (ClassNodeListCache::const_iterator it = m_classNodeListCache.begin(); it != classCacheEnd; ++it) {
- if (it->second->refCount())
- return false;
- }
-
- NameNodeListCache::const_iterator nameCacheEnd = m_nameNodeListCache.end();
- for (NameNodeListCache::const_iterator it = m_nameNodeListCache.begin(); it != nameCacheEnd; ++it) {
- if (it->second->refCount())
- return false;
- }
-
if (m_labelsNodeListCache)
return false;
Modified: trunk/Source/WebCore/dom/Node.h (87146 => 87147)
--- trunk/Source/WebCore/dom/Node.h 2011-05-24 14:17:10 UTC (rev 87146)
+++ trunk/Source/WebCore/dom/Node.h 2011-05-24 14:48:24 UTC (rev 87147)
@@ -503,6 +503,7 @@
void showTreeAndMark(const Node* markedNode1, const char* markedLabel1, const Node* markedNode2 = 0, const char* markedLabel2 = 0) const;
#endif
+ void removeNodeListCacheIfPossible();
void registerDynamicNodeList(DynamicNodeList*);
void unregisterDynamicNodeList(DynamicNodeList*);
void notifyNodeListsChildrenChanged();
Modified: trunk/Source/WebCore/dom/NodeRareData.h (87146 => 87147)
--- trunk/Source/WebCore/dom/NodeRareData.h 2011-05-24 14:17:10 UTC (rev 87146)
+++ trunk/Source/WebCore/dom/NodeRareData.h 2011-05-24 14:48:24 UTC (rev 87147)
@@ -69,10 +69,7 @@
bool isEmpty() const;
private:
- NodeListsNodeData()
- : m_childNodeListCaches(DynamicNodeList::Caches::create()), m_labelsNodeListCache(0)
- {
- }
+ NodeListsNodeData() : m_labelsNodeListCache(0) {}
};
class NodeRareData {
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-changes
