[webkit-changes] [243122] trunk/Source/WebCore

Mon, 18 Mar 2019 18:33:16 -0700

Title: [243122] trunk/Source/WebCore
Revision
243122
Author
[email protected]
Date
2019-03-18 18:32:04 -0700 (Mon, 18 Mar 2019)

Log Message

Reduce the size of Node::deref by eliminating an explicit parentNode check
https://bugs.webkit.org/show_bug.cgi?id=195776

Reviewed by Geoffrey Garen.

This patch eliminates the nullity check of m_parentNode in Node::deref as well as the store to
m_refCount in the case of invoking Node::removedLastRef() as done for RefCounted in r30042.
Together, this patch shrinks WebCore's size by 46KB or ~0.7%.

To do this, we take we take a similar approach as WTF::String by using the lowest bit of m_refCount
to indicate whether a node has a parent or not. Regular ref-counting is done on the upper 31 bits.
Node::setParentNode updates this flag, and Node::deref() would only `delete this` if m_refCount
is identically equal to 0.

For a Document, we set m_refCounted to 0 before in the case of non-zero m_referencingNodeCount
since decrementReferencingNodeCount needs to be able to tell if there is an outstanding Ref/RefPtr
or not when m_referencingNodeCount becomes 0.

No new tests since there should be no behavioral change.

* dom/Document.cpp:
(WebCore::Document::removedLastRef):
* dom/Document.h:
(WebCore::Document::decrementReferencingNodeCount):
* dom/Node.cpp:
(WebCore::Node::Node): Moved the initialization of m_refCount to the member variable declaration.
(WebCore::Node::~Node):
(WebCore::Node::removedLastRef):
* dom/Node.h:
(WebCore::Node): Changed the type of m_refCount from signed int to uint32_t. It was changed from
unsigned int to signed int back in r11492 but I don't think the signedness is needed.
(WebCore::Node::ref): Increment the ref count by 2 (upper 31-bit).
(WebCore::Node::deref): Implemented the optimization. This is what shrinks the WebCore binary size.
(WebCore::Node::hasOneRef const):
(WebCore::Node::refCount const): Ignore the lowest bit. Without this fix, the optimization in
replaceChildrenWithFragment to avoid replacing the text node is disabled whenever there is a parent.
(WebCore::Node::setParentNode): Sets the lowest bit to 1 if the node has a parent and 0 otherwise.

Modified Paths

Diff

Modified: trunk/Source/WebCore/ChangeLog (243121 => 243122)


--- trunk/Source/WebCore/ChangeLog	2019-03-19 00:39:41 UTC (rev 243121)
+++ trunk/Source/WebCore/ChangeLog	2019-03-19 01:32:04 UTC (rev 243122)
@@ -1,3 +1,43 @@
+2019-03-18  Ryosuke Niwa  <[email protected]>
+
+        Reduce the size of Node::deref by eliminating an explicit parentNode check
+        https://bugs.webkit.org/show_bug.cgi?id=195776
+
+        Reviewed by Geoffrey Garen.
+
+        This patch eliminates the nullity check of m_parentNode in Node::deref as well as the store to
+        m_refCount in the case of invoking Node::removedLastRef() as done for RefCounted in r30042.
+        Together, this patch shrinks WebCore's size by 46KB or ~0.7%.
+
+        To do this, we take we take a similar approach as WTF::String by using the lowest bit of m_refCount
+        to indicate whether a node has a parent or not. Regular ref-counting is done on the upper 31 bits.
+        Node::setParentNode updates this flag, and Node::deref() would only `delete this` if m_refCount
+        is identically equal to 0.
+
+        For a Document, we set m_refCounted to 0 before in the case of non-zero m_referencingNodeCount
+        since decrementReferencingNodeCount needs to be able to tell if there is an outstanding Ref/RefPtr
+        or not when m_referencingNodeCount becomes 0.
+
+        No new tests since there should be no behavioral change.
+
+        * dom/Document.cpp:
+        (WebCore::Document::removedLastRef):
+        * dom/Document.h:
+        (WebCore::Document::decrementReferencingNodeCount):
+        * dom/Node.cpp:
+        (WebCore::Node::Node): Moved the initialization of m_refCount to the member variable declaration.
+        (WebCore::Node::~Node):
+        (WebCore::Node::removedLastRef):
+        * dom/Node.h:
+        (WebCore::Node): Changed the type of m_refCount from signed int to uint32_t. It was changed from
+        unsigned int to signed int back in r11492 but I don't think the signedness is needed.
+        (WebCore::Node::ref): Increment the ref count by 2 (upper 31-bit).
+        (WebCore::Node::deref): Implemented the optimization. This is what shrinks the WebCore binary size.
+        (WebCore::Node::hasOneRef const):
+        (WebCore::Node::refCount const): Ignore the lowest bit. Without this fix, the optimization in
+        replaceChildrenWithFragment to avoid replacing the text node is disabled whenever there is a parent.
+        (WebCore::Node::setParentNode): Sets the lowest bit to 1 if the node has a parent and 0 otherwise.
+
 2019-03-18  Said Abou-Hallawa  <[email protected]>
 
         Remove the SVG property tear off objects for SVGAnimatedBoolean

Modified: trunk/Source/WebCore/dom/Document.cpp (243121 => 243122)


--- trunk/Source/WebCore/dom/Document.cpp	2019-03-19 00:39:41 UTC (rev 243121)
+++ trunk/Source/WebCore/dom/Document.cpp	2019-03-19 01:32:04 UTC (rev 243122)
@@ -669,6 +669,10 @@
 {
     ASSERT(!m_deletionHasBegun);
     if (m_referencingNodeCount) {
+        // Node::removedLastRef doesn't set refCount() to zero because it's not observable.
+        // But we need to remember that our refCount reached zero in subsequent calls to decrementReferencingNodeCount()
+        m_refCountAndParentBit = 0;
+
         // If removing a child removes the last node reference, we don't want the scope to be destroyed
         // until after removeDetachedChildren returns, so we protect ourselves.
         incrementReferencingNodeCount();
@@ -718,7 +722,6 @@
         m_inRemovedLastRefFunction = false;
         m_deletionHasBegun = true;
 #endif
-        m_refCount = 1; // Avoid double destruction through use of RefPtr<T>. (This is a security mitigation in case of programmer error. It will ASSERT in debug builds.)
         delete this;
     }
 }

Modified: trunk/Source/WebCore/dom/Document.h (243121 => 243122)


--- trunk/Source/WebCore/dom/Document.h	2019-03-19 00:39:41 UTC (rev 243121)
+++ trunk/Source/WebCore/dom/Document.h	2019-03-19 01:32:04 UTC (rev 243122)
@@ -376,7 +376,7 @@
 #if !ASSERT_DISABLED
             m_deletionHasBegun = true;
 #endif
-            m_refCount = 1; // Avoid double destruction through use of RefPtr<T>. (This is a security mitigation in case of programmer error. It will ASSERT in debug builds.)
+            m_refCountAndParentBit = s_refCountIncrement; // Avoid double destruction through use of Ref<T>/RefPtr<T>. (This is a security mitigation in case of programmer error. It will ASSERT in debug builds.)
             delete this;
         }
     }

Modified: trunk/Source/WebCore/dom/Node.cpp (243121 => 243122)


--- trunk/Source/WebCore/dom/Node.cpp	2019-03-19 00:39:41 UTC (rev 243121)
+++ trunk/Source/WebCore/dom/Node.cpp	2019-03-19 01:32:04 UTC (rev 243122)
@@ -316,8 +316,7 @@
 }
 
 Node::Node(Document& document, ConstructionType type)
-    : m_refCount(1)
-    , m_nodeFlags(type)
+    : m_nodeFlags(type)
     , m_treeScope(&document)
 {
     ASSERT(isMainThread());
@@ -332,9 +331,9 @@
 Node::~Node()
 {
     ASSERT(isMainThread());
-    // We set m_refCount to 1 before calling delete to avoid double destruction through use of Ref<T>/RefPtr<T>.
+    // We set m_refCount to 2 before calling delete to avoid double destruction through use of Ref<T>/RefPtr<T>.
     // This is a security mitigation in case of programmer errorm (caught by a debug assertion).
-    ASSERT(m_refCount == 1);
+    ASSERT(m_refCountAndParentBit == s_refCountIncrement);
     ASSERT(m_deletionHasBegun);
     ASSERT(!m_adoptionIsRequired);
 
@@ -2523,6 +2522,10 @@
 // delete a Node at each deref call site.
 void Node::removedLastRef()
 {
+    // This avoids double destruction even when there is a programming error to use Ref<T> / RefPtr<T> on this node.
+    // There are debug assertions in Node::ref() / Node::deref() to catch such a programming error.
+    ASSERT(m_refCountAndParentBit == s_refCountIncrement);
+
     // An explicit check for Document here is better than a virtual function since it is
     // faster for non-Document nodes, and because the call to removedLastRef that is inlined
     // at all deref call sites is smaller if it's a non-virtual function.
@@ -2534,7 +2537,6 @@
 #ifndef NDEBUG
     m_deletionHasBegun = true;
 #endif
-    m_refCount = 1; // Avoid double destruction through use of RefPtr<T>. (This is a security mitigation in case of programmer error. It will ASSERT in debug builds.)
     delete this;
 }

Modified: trunk/Source/WebCore/dom/Node.h (243121 => 243122)


--- trunk/Source/WebCore/dom/Node.h	2019-03-19 00:39:41 UTC (rev 243121)
+++ trunk/Source/WebCore/dom/Node.h	2019-03-19 01:32:04 UTC (rev 243122)
@@ -500,7 +500,7 @@
     void ref();
     void deref();
     bool hasOneRef() const;
-    int refCount() const;
+    unsigned refCount() const;
 
 #ifndef NDEBUG
     bool m_deletionHasBegun { false };
@@ -618,6 +618,9 @@
     };
     Node(Document&, ConstructionType);
 
+    static constexpr uint32_t s_refCountIncrement = 2;
+    static constexpr uint32_t s_refCountMask = ~static_cast<uint32_t>(0x1);
+
     virtual void addSubresourceAttributeURLs(ListHashSet<URL>&) const { }
 
     bool hasRareData() const { return getFlag(HasRareDataFlag); }
@@ -664,7 +667,7 @@
     static void moveTreeToNewScope(Node&, TreeScope& oldScope, TreeScope& newScope);
     void moveNodeToNewDocument(Document& oldDocument, Document& newDocument);
 
-    int m_refCount;
+    uint32_t m_refCountAndParentBit { s_refCountIncrement };
     mutable uint32_t m_nodeFlags;
 
     ContainerNode* m_parentNode { nullptr };
@@ -695,22 +698,25 @@
     ASSERT(!m_deletionHasBegun);
     ASSERT(!m_inRemovedLastRefFunction);
     ASSERT(!m_adoptionIsRequired);
-    ++m_refCount;
+    m_refCountAndParentBit += s_refCountIncrement;
 }
 
 ALWAYS_INLINE void Node::deref()
 {
     ASSERT(isMainThread());
-    ASSERT(m_refCount >= 0);
+    ASSERT(refCount());
     ASSERT(!m_deletionHasBegun);
     ASSERT(!m_inRemovedLastRefFunction);
     ASSERT(!m_adoptionIsRequired);
-    if (--m_refCount <= 0 && !parentNode()) {
+    auto tempRefCount = m_refCountAndParentBit - s_refCountIncrement;
+    if (!tempRefCount) {
 #ifndef NDEBUG
         m_inRemovedLastRefFunction = true;
 #endif
         removedLastRef();
+        return;
     }
+    m_refCountAndParentBit = tempRefCount;
 }
 
 ALWAYS_INLINE bool Node::hasOneRef() const
@@ -717,12 +723,12 @@
 {
     ASSERT(!m_deletionHasBegun);
     ASSERT(!m_inRemovedLastRefFunction);
-    return m_refCount == 1;
+    return refCount() == 1;
 }
 
-ALWAYS_INLINE int Node::refCount() const
+ALWAYS_INLINE unsigned Node::refCount() const
 {
-    return m_refCount;
+    return m_refCountAndParentBit / s_refCountIncrement;
 }
 
 // Used in Node::addSubresourceAttributeURLs() and in addSubresourceStyleURLs()
@@ -736,6 +742,8 @@
 {
     ASSERT(isMainThread());
     m_parentNode = parent;
+    auto refCountWithoutParentBit = m_refCountAndParentBit & s_refCountMask;
+    m_refCountAndParentBit = refCountWithoutParentBit | !!parent;
 }
 
 inline ContainerNode* Node::parentNode() const

_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to