Oilpan: move LiveNodeList collections to the heap.

Source/core/dom/NodeRareData.h

 /*
  * Copyright (C) 2008, 2010 Apple Inc. All rights reserved.
  * Copyright (C) 2008 David Smith <catfish.man@gmail.com>
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
  *
  * This library is distributed in the hope that it will be useful,
@@ skipping 24 matching lines @@
 #include "wtf/PassOwnPtr.h"
 #include "wtf/text/AtomicString.h"
 #include "wtf/text/StringHash.h"
 
 namespace WebCore {
 
 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()
     {
         if (m_childNodeList && m_childNodeList->isChildNodeList())
             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)
     {
         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)
         {
             return DefaultHash<StringImpl*>::Hash::hash(entry.second) + entry.first;
         }
         static bool equal(const std::pair<unsigned char, StringImpl*>& a, const std::pair<unsigned char, StringImpl*>& b) { return a == b; }
         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();
     }
 
     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)));
         if (deleteThisAndUpdateNodeRareDataIfAboutToRemoveLastList(list->ownerNode()))
             return;
         m_atomicNameCaches.remove(namedNodeListKey(collectionType, name));
     }
 
     void removeCache(LiveNodeListBase* list, const AtomicString& namespaceURI, const AtomicString& localName)
     {
         QualifiedName name(nullAtom, localName, namespaceURI);
         ASSERT(list == m_tagCollectionCacheNS.get(name));
         if (deleteThisAndUpdateNodeRareDataIfAboutToRemoveLastList(list->ownerNode()))
             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 adoptPtr(new NodeListsNodeData);
+        return adoptPtrWillBeNoop(new NodeListsNodeData(owner));
     }
+#else
+    static PassOwnPtrWillBeRawPtr<NodeListsNodeData> create()
+    {
+        return adoptPtrWillBeNoop(new NodeListsNodeData);
+    }
+#endif
 
     void invalidateCaches(const QualifiedName* attrName = 0);
     bool isEmpty() const
     {
         return m_atomicNameCaches.isEmpty() && m_tagCollectionCacheNS.isEmpty();
     }
 
     void adoptTreeScope()
     {
         invalidateCaches();
@@ skipping 10 matching lines @@
         }
 
         TagCollectionCacheNS::const_iterator tagEnd = m_tagCollectionCacheNS.end();
         for (TagCollectionCacheNS::const_iterator it = m_tagCollectionCacheNS.begin(); it != tagEnd; ++it) {
             LiveNodeListBase* list = it->value;
             ASSERT(!list->isRootedAtDocument());
             list->didMoveToDocument(oldDocument, newDocument);
         }
     }
 
+#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)
     {
         // Holding the raw StringImpl is safe because |name| is retained by the NodeList and the NodeList
         // is reponsible for removing itself from the cache on deletion.
         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> {
     WTF_MAKE_NONCOPYABLE(NodeMutationObserverData);
     WTF_MAKE_FAST_ALLOCATED_WILL_BE_REMOVED;
 public:
     WillBeHeapVector<OwnPtrWillBeMember<MutationObserverRegistration> > registry;
     WillBeHeapHashSet<RawPtrWillBeMember<MutationObserverRegistration> > transientRegistry;
 
     static PassOwnPtrWillBeRawPtr<NodeMutationObserverData> create()
@@ skipping 17 matching lines @@
 public:
     static NodeRareData* create(RenderObject* renderer)
     {
         return new NodeRareData(renderer);
     }
 
     void clearNodeLists() { m_nodeLists.clear(); }
     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;
     }
 
     NodeMutationObserverData* mutationObserverData() { return m_mutationObserverData.get(); }
     NodeMutationObserverData& ensureMutationObserverData()
     {
         if (!m_mutationObserverData)
             m_mutationObserverData = NodeMutationObserverData::create();
         return *m_mutationObserverData;
     }
@@ skipping 22 matching lines @@
     enum {
         ConnectedFrameCountBits = 10, // Must fit Page::maxNumberOfFrames.
     };
 
     void trace(Visitor*);
 
     void traceAfterDispatch(Visitor*);
     void finalizeGarbageCollectedObject();
 
 protected:
-    NodeRareData(RenderObject* renderer)
+    explicit NodeRareData(RenderObject* renderer)
         : NodeRareDataBase(renderer)
         , m_connectedFrameCount(0)
         , m_elementFlags(0)
         , m_restyleFlags(0)
         , m_isElementRareData(false)
     { }
 
 private:
-    OwnPtr<NodeListsNodeData> m_nodeLists;
+    OwnPtrWillBeMember<NodeListsNodeData> m_nodeLists;
     OwnPtrWillBeMember<NodeMutationObserverData> m_mutationObserverData;
 
     unsigned m_connectedFrameCount : ConnectedFrameCountBits;
     unsigned m_elementFlags : NumberOfElementFlags;
     unsigned m_restyleFlags : NumberOfDynamicRestyleFlags;
 protected:
     unsigned m_isElementRareData : 1;
 };
 
+#if !ENABLE(OILPAN)
 inline bool NodeListsNodeData::deleteThisAndUpdateNodeRareDataIfAboutToRemoveLastList(Node& ownerNode)
 {
     ASSERT(ownerNode.nodeLists() == this);
     if ((m_childNodeList ? 1 : 0) + m_atomicNameCaches.size() + m_tagCollectionCacheNS.size() != 1)
         return false;
     ownerNode.clearNodeLists();
     return true;
 }
+#endif
 
 } // namespace WebCore
 
 #endif // NodeRareData_h