Changeset 45620 in webkit

Timestamp:

Jul 7, 2009 10:38:20 PM (15 years ago)

Author:

oliver@apple.com

Message:

Reduce complexity of lifetime management in DynamicNodeList caches
<​https://bugs.webkit.org/show_bug.cgi?id=27068>

Reviewed by Maciej Stachowiak

Switch the Cache object used by DynamicNodeList into a normal
refcounted object rather than having a weird flag controlled
refcounting system, where positive refcount did not automatically
imply the cache object would actually still be live.

Location:

trunk/WebCore

Files:

• ChangeLog (modified) (1 diff)
• dom/DynamicNodeList.cpp (modified) (4 diffs)
• dom/DynamicNodeList.h (modified) (3 diffs)
• dom/Node.cpp (modified) (8 diffs)
• dom/NodeRareData.h (modified) (2 diffs)

trunk/WebCore/ChangeLog

@@ +1 @@
+2009-07-07  Oliver Hunt  <oliver@apple.com>
+
+        Reviewed by Maciej Stachowiak.
+
+        Reduce complexity of lifetime management in DynamicNodeList caches
+        <https://bugs.webkit.org/show_bug.cgi?id=27068>
+
+        Switch the Cache object used by DynamicNodeList into a normal
+        refcounted object rather than having a weird flag controlled
+        refcounting system, where positive refcount did not automatically
+        imply the cache object would actually still be live.
+
+        * dom/DynamicNodeList.cpp:
+        (WebCore::DynamicNodeList::DynamicNodeList):
+        (WebCore::DynamicNodeList::~DynamicNodeList):
+        (WebCore::DynamicNodeList::Caches::Caches):
+        (WebCore::DynamicNodeList::Caches::create):
+        * dom/DynamicNodeList.h:
+        * dom/Node.cpp:
+        (WebCore::Node::childNodes):
+        (WebCore::Node::getElementsByTagNameNS):
+        (WebCore::Node::getElementsByName):
+        (WebCore::Node::getElementsByClassName):
+        (WebCore::NodeListsNodeData::invalidateCaches):
+        (WebCore::NodeListsNodeData::isEmpty):
+        * dom/NodeRareData.h:
+        (WebCore::NodeListsNodeData::NodeListsNodeData):
+
 2009-07-07  Simon Fraser  <simon.fraser@apple.com>
 

trunk/WebCore/dom/DynamicNodeList.cpp

@@ -31 +31 @@
 DynamicNodeList::DynamicNodeList(PassRefPtr<Node> rootNode)
     : m_rootNode(rootNode)
-    , m_caches(new Caches)
+    , m_caches(Caches::create())
     , m_ownsCaches(true)
 {
@@ -43 +43 @@
 {
     m_rootNode->registerDynamicNodeList(this);
-    ++caches->refCount;
 }    
 
@@ -49 +48 @@
 {
     m_rootNode->unregisterDynamicNodeList(this);
-    if (m_ownsCaches)
-        delete m_caches;
-    else
-        --m_caches->refCount;
 }
 
@@ -159 +154 @@
     , isLengthCacheValid(false)
     , isItemCacheValid(false)
-    , refCount(0)
 {
+}
+
+PassRefPtr<DynamicNodeList::Caches> DynamicNodeList::Caches::create()
+{
+    return adoptRef(new Caches());
 }
 

trunk/WebCore/dom/DynamicNodeList.h

@@ -38 +38 @@
     class DynamicNodeList : public NodeList {
     public:
-        struct Caches {
-            Caches();
+        struct Caches : RefCounted<Caches>{
+            static PassRefPtr<Caches> create();
             void reset();
 
@@ -47 +47 @@
             bool isLengthCacheValid : 1;
             bool isItemCacheValid : 1;
-            unsigned refCount;
+        protected:
+            Caches();
         };
 
@@ -69 +70 @@
 
         RefPtr<Node> m_rootNode;
-        mutable Caches* m_caches;
+        mutable RefPtr<Caches> m_caches;
         bool m_ownsCaches;
 

trunk/WebCore/dom/Node.cpp

@@ -518 +518 @@
     }
 
-    return ChildNodeList::create(this, &data->nodeLists()->m_childNodeListCaches);
+    return ChildNodeList::create(this, data->nodeLists()->m_childNodeListCaches.get());
 }
 
@@ -1501 +1501 @@
     pair<NodeListsNodeData::TagCacheMap::iterator, bool> result = data->nodeLists()->m_tagNodeListCaches.add(QualifiedName(nullAtom, localNameAtom, namespaceURI), 0);
     if (result.second)
-        result.first->second = new DynamicNodeList::Caches;
-    
-    return TagNodeList::create(this, namespaceURI.isEmpty() ? nullAtom : namespaceURI, localNameAtom, result.first->second);
+        result.first->second = DynamicNodeList::Caches::create();
+    
+    return TagNodeList::create(this, namespaceURI.isEmpty() ? nullAtom : namespaceURI, localNameAtom, result.first->second.get());
 }
 
@@ -1516 +1516 @@
     pair<NodeListsNodeData::CacheMap::iterator, bool> result = data->nodeLists()->m_nameNodeListCaches.add(elementName, 0);
     if (result.second)
-        result.first->second = new DynamicNodeList::Caches;
-    
-    return NameNodeList::create(this, elementName, result.first->second);
+        result.first->second = DynamicNodeList::Caches::create();
+    
+    return NameNodeList::create(this, elementName, result.first->second.get());
 }
 
@@ -1531 +1531 @@
     pair<NodeListsNodeData::CacheMap::iterator, bool> result = data->nodeLists()->m_classNodeListCaches.add(classNames, 0);
     if (result.second)
-        result.first->second = new DynamicNodeList::Caches;
-    
-    return ClassNodeList::create(this, classNames, result.first->second);
+        result.first->second = DynamicNodeList::Caches::create();
+    
+    return ClassNodeList::create(this, classNames, result.first->second.get());
 }
 
@@ -2186 +2186 @@
 void NodeListsNodeData::invalidateCaches()
 {
-    m_childNodeListCaches.reset();
+    m_childNodeListCaches->reset();
     TagCacheMap::const_iterator tagCachesEnd = m_tagNodeListCaches.end();
     for (TagCacheMap::const_iterator it = m_tagNodeListCaches.begin(); it != tagCachesEnd; ++it)
@@ -2209 +2209 @@
         return false;
 
-    if (m_childNodeListCaches.refCount)
+    if (m_childNodeListCaches->refCount())
         return false;
     
     TagCacheMap::const_iterator tagCachesEnd = m_tagNodeListCaches.end();
     for (TagCacheMap::const_iterator it = m_tagNodeListCaches.begin(); it != tagCachesEnd; ++it) {
-        if (it->second->refCount)
+        if (it->second->refCount())
             return false;
     }
@@ -2220 +2220 @@
     CacheMap::const_iterator classCachesEnd = m_classNodeListCaches.end();
     for (CacheMap::const_iterator it = m_classNodeListCaches.begin(); it != classCachesEnd; ++it) {
-        if (it->second->refCount)
+        if (it->second->refCount())
             return false;
     }
@@ -2226 +2226 @@
     CacheMap::const_iterator nameCachesEnd = m_nameNodeListCaches.end();
     for (CacheMap::const_iterator it = m_nameNodeListCaches.begin(); it != nameCachesEnd; ++it) {
-        if (it->second->refCount)
+        if (it->second->refCount())
             return false;
     }

trunk/WebCore/dom/NodeRareData.h

@@ -38 +38 @@
     NodeListSet m_listsWithCaches;
     
-    DynamicNodeList::Caches m_childNodeListCaches;
+    RefPtr<DynamicNodeList::Caches> m_childNodeListCaches;
     
-    typedef HashMap<String, DynamicNodeList::Caches*> CacheMap;
+    typedef HashMap<String, RefPtr<DynamicNodeList::Caches> > CacheMap;
     CacheMap m_classNodeListCaches;
     CacheMap m_nameNodeListCaches;
     
-    typedef HashMap<QualifiedName, DynamicNodeList::Caches*> TagCacheMap;
+    typedef HashMap<QualifiedName, RefPtr<DynamicNodeList::Caches> > TagCacheMap;
     TagCacheMap m_tagNodeListCaches;
 
     static PassOwnPtr<NodeListsNodeData> create() {
         return new NodeListsNodeData;
-    }
-
-    ~NodeListsNodeData()
-    {
-        deleteAllValues(m_classNodeListCaches);
-        deleteAllValues(m_nameNodeListCaches);
-        deleteAllValues(m_tagNodeListCaches);
     }
     
@@ -63 +56 @@
 
 private:
-    NodeListsNodeData() { }
+    NodeListsNodeData()
+        : m_childNodeListCaches(DynamicNodeList::Caches::create())
+    {
+    }
 };