Oilpan: move LiveNodeList collections to the heap.

Source/core/dom/NodeRareData.h

Index: Source/core/dom/NodeRareData.h
diff --git a/Source/core/dom/NodeRareData.h b/Source/core/dom/NodeRareData.h
index 91405c86952227d8a9399081702467750a313929..0ebd858b0dc98c769f1b940469a545481a0c5acf 100644
--- a/Source/core/dom/NodeRareData.h
+++ b/Source/core/dom/NodeRareData.h
@@ -42,8 +42,9 @@ class LabelsNodeList;
 class RadioNodeList;
 class TreeScope;
 
-class NodeListsNodeData {
-    WTF_MAKE_NONCOPYABLE(NodeListsNodeData); WTF_MAKE_FAST_ALLOCATED;
+class NodeListsNodeData FINAL : public NoBaseWillBeGarbageCollectedFinalized<NodeListsNodeData> {
+    WTF_MAKE_NONCOPYABLE(NodeListsNodeData);
+    WTF_MAKE_FAST_ALLOCATED_WILL_BE_REMOVED;
 public:
     void clearChildNodeListCache()
     {
@@ -51,30 +52,31 @@ public:
             toChildNodeList(m_childNodeList)->invalidateCache();
     }
 
-    PassRefPtr<ChildNodeList> ensureChildNodeList(ContainerNode& node)
+    PassRefPtrWillBeRawPtr<ChildNodeList> ensureChildNodeList(ContainerNode& node)
     {
         if (m_childNodeList)
             return toChildNodeList(m_childNodeList);
-        RefPtr<ChildNodeList> list = ChildNodeList::create(node);
+        RefPtrWillBeRawPtr<ChildNodeList> list = ChildNodeList::create(node);
         m_childNodeList = list.get();
         return list.release();
     }
 
-    PassRefPtr<EmptyNodeList> ensureEmptyChildNodeList(Node& node)
+    PassRefPtrWillBeRawPtr<EmptyNodeList> ensureEmptyChildNodeList(Node& node)
     {
         if (m_childNodeList)
             return toEmptyNodeList(m_childNodeList);
-        RefPtr<EmptyNodeList> list = EmptyNodeList::create(node);
+        RefPtrWillBeRawPtr<EmptyNodeList> list = EmptyNodeList::create(node);
         m_childNodeList = list.get();
         return list.release();
     }
 
+#if !ENABLE(OILPAN)
     void removeChildNodeList(ChildNodeList* list)
     {
         ASSERT(m_childNodeList == list);
         if (deleteThisAndUpdateNodeRareDataIfAboutToRemoveLastList(list->ownerNode()))
             return;
-        m_childNodeList = 0;
+        m_childNodeList = nullptr;
     }
 
     void removeEmptyChildNodeList(EmptyNodeList* list)
@@ -82,8 +84,9 @@ public:
         ASSERT(m_childNodeList == list);
         if (deleteThisAndUpdateNodeRareDataIfAboutToRemoveLastList(list->ownerNode()))
             return;
-        m_childNodeList = 0;
+        m_childNodeList = nullptr;
     }
+#endif
 
     struct NodeListAtomicCacheMapEntryHash {
         static unsigned hash(const std::pair<unsigned char, StringImpl*>& entry)
@@ -94,29 +97,33 @@ public:
         static const bool safeToCompareToEmptyOrDeleted = DefaultHash<StringImpl*>::Hash::safeToCompareToEmptyOrDeleted;
     };
 
-    typedef HashMap<std::pair<unsigned char, StringImpl*>, LiveNodeListBase*, NodeListAtomicCacheMapEntryHash> NodeListAtomicNameCacheMap;
-    typedef HashMap<QualifiedName, TagCollection*> TagCollectionCacheNS;
+    // Oilpan: keep a weak reference to the collection objects.
+    // Explicit object unregistration in a non-Oilpan setting
+    // on object destruction is replaced by the garbage collector
+    // clearing out their weak reference.
+    typedef WillBeHeapHashMap<std::pair<unsigned char, StringImpl*>, RawPtrWillBeWeakMember<LiveNodeListBase>, NodeListAtomicCacheMapEntryHash> NodeListAtomicNameCacheMap;
+    typedef WillBeHeapHashMap<QualifiedName, RawPtrWillBeWeakMember<TagCollection> > TagCollectionCacheNS;
 
     template<typename T>
-    PassRefPtr<T> addCache(ContainerNode& node, CollectionType collectionType, const AtomicString& name)
+    PassRefPtrWillBeRawPtr<T> addCache(ContainerNode& node, CollectionType collectionType, const AtomicString& name)
     {
-        NodeListAtomicNameCacheMap::AddResult result = m_atomicNameCaches.add(namedNodeListKey(collectionType, name), 0);
+        NodeListAtomicNameCacheMap::AddResult result = m_atomicNameCaches.add(namedNodeListKey(collectionType, name), nullptr);
         if (!result.isNewEntry)
-            return static_cast<T*>(result.storedValue->value);
+            return static_cast<T*>(result.storedValue->value.get());
 
-        RefPtr<T> list = T::create(node, collectionType, name);
+        RefPtrWillBeRawPtr<T> list = T::create(node, collectionType, name);
         result.storedValue->value = list.get();
         return list.release();
     }
 
     template<typename T>
-    PassRefPtr<T> addCache(ContainerNode& node, CollectionType collectionType)
+    PassRefPtrWillBeRawPtr<T> addCache(ContainerNode& node, CollectionType collectionType)
     {
-        NodeListAtomicNameCacheMap::AddResult result = m_atomicNameCaches.add(namedNodeListKey(collectionType, starAtom), 0);
+        NodeListAtomicNameCacheMap::AddResult result = m_atomicNameCaches.add(namedNodeListKey(collectionType, starAtom), nullptr);
         if (!result.isNewEntry)
-            return static_cast<T*>(result.storedValue->value);
+            return static_cast<T*>(result.storedValue->value.get());
 
-        RefPtr<T> list = T::create(node, collectionType);
+        RefPtrWillBeRawPtr<T> list = T::create(node, collectionType);
         result.storedValue->value = list.get();
         return list.release();
     }
@@ -124,21 +131,28 @@ public:
     template<typename T>
     T* cached(CollectionType collectionType)
     {
+#if ENABLE(OILPAN)
+        // FIXME: Oilpan: unify, if possible. The lookup resolves to a T& with Oilpan,
+        // whereas non-Oilpan resolves to RawPtr<T>.
         return static_cast<T*>(m_atomicNameCaches.get(namedNodeListKey(collectionType, starAtom)));
+#else
+        return static_cast<T*>(m_atomicNameCaches.get(namedNodeListKey(collectionType, starAtom)).get());
+#endif
     }
 
-    PassRefPtr<TagCollection> addCache(ContainerNode& node, const AtomicString& namespaceURI, const AtomicString& localName)
+    PassRefPtrWillBeRawPtr<TagCollection> addCache(ContainerNode& node, const AtomicString& namespaceURI, const AtomicString& localName)
     {
         QualifiedName name(nullAtom, localName, namespaceURI);
-        TagCollectionCacheNS::AddResult result = m_tagCollectionCacheNS.add(name, 0);
+        TagCollectionCacheNS::AddResult result = m_tagCollectionCacheNS.add(name, nullptr);
         if (!result.isNewEntry)
             return result.storedValue->value;
 
-        RefPtr<TagCollection> list = TagCollection::create(node, namespaceURI, localName);
+        RefPtrWillBeRawPtr<TagCollection> list = TagCollection::create(node, namespaceURI, localName);
         result.storedValue->value = list.get();
         return list.release();
     }
 
+#if !ENABLE(OILPAN)
     void removeCache(LiveNodeListBase* list, CollectionType collectionType, const AtomicString& name = starAtom)
     {
         ASSERT(list == m_atomicNameCaches.get(namedNodeListKey(collectionType, name)));
@@ -155,11 +169,19 @@ public:
             return;
         m_tagCollectionCacheNS.remove(name);
     }
+#endif
 
-    static PassOwnPtr<NodeListsNodeData> create()
+#if ENABLE(OILPAN)
+    static PassOwnPtrWillBeRawPtr<NodeListsNodeData> create(NodeRareData* owner)

[Mads Ager (chromium), 2014/05/20 06:58:57]

Since we have the #if around the entire definition we should just use the real
oilpan/non-oilpan types in the two versions.

[sof, 2014/05/20 08:27:49]

Done.

+    {
+        return adoptPtrWillBeNoop(new NodeListsNodeData(owner));
+    }
+#else
+    static PassOwnPtrWillBeRawPtr<NodeListsNodeData> create()
     {
-        return adoptPtr(new NodeListsNodeData);
+        return adoptPtrWillBeNoop(new NodeListsNodeData);
     }
+#endif
 
     void invalidateCaches(const QualifiedName* attrName = 0);
     bool isEmpty() const
@@ -190,9 +212,31 @@ public:
         }
     }
 
+#if ENABLE(OILPAN)

[Mads Ager (chromium), 2014/05/20 06:58:57]

Nit: For Document and friends I have not used #if for clearWeakMembers. We could
move the #if to the body of clearWeakMembers to reduce the #if spaghetti. I
don't feel strongly about it though. :)

[sof, 2014/05/20 08:27:49]

I will leave it out for non-Oilpan for now, unpretty as it is. We've already
#if-ed out the non-Oilpan version of this optimization, so by symmetry.. :)

+    void clearWeakMembers(Visitor*);
+#endif
+
+    void trace(Visitor* visitor)
+    {
+        visitor->trace(m_childNodeList);
+        visitor->trace(m_atomicNameCaches);
+        visitor->trace(m_tagCollectionCacheNS);
+#if ENABLE(OILPAN)

[Mads Ager (chromium), 2014/05/20 06:58:57]

Nit: Maybe we should just #if out the entire body instead of just parts of it?

[sof, 2014/05/20 08:27:49]

Done.

+        visitor->trace(m_owner);

[sof, 2014/05/19 21:40:49]

Not sure about this trace() call (and the one for the other weak member,
m_childNodeList) -- is there a way to signal to the GC plugin that the weak
members will be handled manually within the weak callback? 

[Erik Corry, 2014/05/19 21:45:01]

I think you just have to mention the fields in the trace method to shut it up. 
Just having them as an expression statement should be enough:

m_childNodeList;

[zerny-chromium, 2014/05/20 06:07:43]

This is fine as is. The plugin will/should identify the registered callback
function and since both m_owner and m_childNodeList appear there it will
consider them traced.

One small nit: we might wan't to move this trace to the cpp file. I'm not sure
if all compilation units will have access to the definition of clearWeakMembers
since it occurs in the cpp file but trace is in the header.

[sof, 2014/05/20 06:23:59]

Oh, that sounds encouraging. I used isAlive() over those two members in
clearWeakMembers() initially but the GC checker insisted on there being trace()s
for them.
Perhaps that is the reason why the GC plugin wasn't able to spot it? :) Will
move it there regardless.

[zerny-chromium, 2014/05/20 07:58:00]

I think it is. The plugin checks trace methods every time it encounters its
definition, so that will cause this trace to be checked in compilation units
that do not know about the clearWeakMembers callback. Better to keep them
together. Maybe I should find a way to warn about such undefined callbacks when
checking trace.

[sof, 2014/05/20 08:27:49]

You're quite right, not reported as trace() problem with the method moved into
the .cpp file.

+        visitor->registerWeakMembers<NodeListsNodeData, &NodeListsNodeData::clearWeakMembers>(this);
+#endif
+    }
+
 private:
+#if ENABLE(OILPAN)
+    NodeListsNodeData(NodeRareData* owner)

[Mads Ager (chromium), 2014/05/20 06:58:57]

explicit

[sof, 2014/05/20 08:27:49]

Done.

+#else
     NodeListsNodeData()
-        : m_childNodeList(0)
+#endif
+        : m_childNodeList(nullptr)
+#if ENABLE(OILPAN)
+        , m_owner(owner)
+#endif
     { }
 
     std::pair<unsigned char, StringImpl*> namedNodeListKey(CollectionType type, const AtomicString& name)
@@ -202,12 +246,17 @@ private:
         return std::pair<unsigned char, StringImpl*>(type, name.impl());
     }
 
+#if !ENABLE(OILPAN)
     bool deleteThisAndUpdateNodeRareDataIfAboutToRemoveLastList(Node&);
+#endif
 
     // Can be a ChildNodeList or an EmptyNodeList.
-    NodeList* m_childNodeList;
+    RawPtrWillBeWeakMember<NodeList> m_childNodeList;
     NodeListAtomicNameCacheMap m_atomicNameCaches;
     TagCollectionCacheNS m_tagCollectionCacheNS;
+#if ENABLE(OILPAN)
+    WeakMember<NodeRareData> m_owner;

[Mads Ager (chromium), 2014/05/20 06:58:57]

The NodeRareData owns the NodeListNodeData and therefore I don't see any reason
for this to be a WeakMember. The NodeListNodeData will be unreachable when
removed from the NodeRareData and at that point the Member will not be traced
and it will not keep the NodeRareData alive. When they are connected they should
live and die together. So there is no need for weakness here. Let's change this
back to a strong pointer?

I'm sorry for the back and forth here. In general, if we think that something
might need to be weak we should make sure that it cannot just be strong. We
should always prefer strong pointers.

[sof, 2014/05/20 08:27:49]

thanks for bringing clarity; that makes good sense.

The back reference to NodeRareData will not prolong its lifetime, which is
really controlled by its Node. Hence, a strong reference will do just fine.
Switched and simplified.

+#endif
 };
 
 class NodeMutationObserverData FINAL : public NoBaseWillBeGarbageCollected<NodeMutationObserverData> {
@@ -245,8 +294,13 @@ public:
     NodeListsNodeData* nodeLists() const { return m_nodeLists.get(); }
     NodeListsNodeData& ensureNodeLists()
     {
-        if (!m_nodeLists)
+        if (!m_nodeLists) {
+#if ENABLE(OILPAN)
+            m_nodeLists = NodeListsNodeData::create(this);
+#else
             m_nodeLists = NodeListsNodeData::create();
+#endif
+        }
         return *m_nodeLists;
     }
 
@@ -289,7 +343,7 @@ public:
     void finalizeGarbageCollectedObject();
 
 protected:
-    NodeRareData(RenderObject* renderer)
+    explicit NodeRareData(RenderObject* renderer)
         : NodeRareDataBase(renderer)
         , m_connectedFrameCount(0)
         , m_elementFlags(0)
@@ -298,7 +352,7 @@ protected:
     { }
 
 private:
-    OwnPtr<NodeListsNodeData> m_nodeLists;
+    OwnPtrWillBeMember<NodeListsNodeData> m_nodeLists;
     OwnPtrWillBeMember<NodeMutationObserverData> m_mutationObserverData;
 
     unsigned m_connectedFrameCount : ConnectedFrameCountBits;
@@ -308,6 +362,7 @@ protected:
     unsigned m_isElementRareData : 1;
 };
 
+#if !ENABLE(OILPAN)
 inline bool NodeListsNodeData::deleteThisAndUpdateNodeRareDataIfAboutToRemoveLastList(Node& ownerNode)
 {
     ASSERT(ownerNode.nodeLists() == this);
@@ -316,6 +371,7 @@ inline bool NodeListsNodeData::deleteThisAndUpdateNodeRareDataIfAboutToRemoveLas
     ownerNode.clearNodeLists();
     return true;
 }
+#endif
 
 } // namespace WebCore