Changeset 243037 in webkit


Ignore:
Timestamp:
Mar 16, 2019 12:50:24 AM (5 years ago)
Author:
rniwa@webkit.org
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.

Location:
trunk/Source/WebCore
Files:
5 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/WebCore/ChangeLog

    r243036 r243037  
     12019-03-16  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        Reduce the size of Node::deref by eliminating an explicit parentNode check
     4        https://bugs.webkit.org/show_bug.cgi?id=195776
     5
     6        Reviewed by Geoffrey Garen.
     7
     8        This patch eliminates the nullity check of m_parentNode in Node::deref as well as the store to
     9        m_refCount in the case of invoking Node::removedLastRef() as done for RefCounted in r30042.
     10        Together, this patch shrinks WebCore's size by 46KB or ~0.7%.
     11
     12        To do this, we take we take a similar approach as WTF::String by using the lowest bit of m_refCount
     13        to indicate whether a node has a parent or not. Regular ref-counting is done on the upper 31 bits.
     14        Node::setParentNode updates this flag, and Node::deref() would only `delete this` if m_refCount
     15        is identically equal to 0.
     16
     17        For a Document, we set m_refCounted to 0 before in the case of non-zero m_referencingNodeCount
     18        since decrementReferencingNodeCount needs to be able to tell if there is an outstanding Ref/RefPtr
     19        or not when m_referencingNodeCount becomes 0.
     20
     21        No new tests since there should be no behavioral change.
     22
     23        * dom/Document.cpp:
     24        (WebCore::Document::removedLastRef):
     25        * dom/Document.h:
     26        (WebCore::Document::decrementReferencingNodeCount):
     27        * dom/Node.cpp:
     28        (WebCore::Node::Node): Moved the initialization of m_refCount to the member variable declaration.
     29        (WebCore::Node::~Node):
     30        (WebCore::Node::removedLastRef):
     31        * dom/Node.h:
     32        (WebCore::Node): Changed the type of m_refCount from signed int to uint32_t. It was changed from
     33        unsigned int to signed int back in r11492 but I don't think the signedness is needed.
     34        (WebCore::Node::ref): Increment the ref count by 2 (upper 31-bit).
     35        (WebCore::Node::deref): Implemented the optimization. This is what shrinks the WebCore binary size.
     36        (WebCore::Node::hasOneRef const):
     37        (WebCore::Node::refCount const): Ignore the lowest bit. Without this fix, the optimization in
     38        replaceChildrenWithFragment to avoid replacing the text node is disabled whenever there is a parent.
     39        (WebCore::Node::setParentNode): Sets the lowest bit to 1 if the node has a parent and 0 otherwise.
     40
    1412019-03-16  Said Abou-Hallawa  <sabouhallawa@apple.com>
    242
  • trunk/Source/WebCore/dom/Document.cpp

    r243035 r243037  
    670670    ASSERT(!m_deletionHasBegun);
    671671    if (m_referencingNodeCount) {
     672        // Node::removedLastRef doesn't set refCount() to zero because it's not observable.
     673        // But we need to remember that our refCount reached zero in subsequent calls to decrementReferencingNodeCount()
     674        m_refCountAndParentBit = 0;
     675
    672676        // If removing a child removes the last node reference, we don't want the scope to be destroyed
    673677        // until after removeDetachedChildren returns, so we protect ourselves.
     
    719723        m_deletionHasBegun = true;
    720724#endif
    721         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.)
    722725        delete this;
    723726    }
  • trunk/Source/WebCore/dom/Document.h

    r242964 r243037  
    377377            m_deletionHasBegun = true;
    378378#endif
    379             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.)
     379            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.)
    380380            delete this;
    381381        }
  • trunk/Source/WebCore/dom/Node.cpp

    r242964 r243037  
    317317
    318318Node::Node(Document& document, ConstructionType type)
    319     : m_refCount(1)
    320     , m_nodeFlags(type)
     319    : m_nodeFlags(type)
    321320    , m_treeScope(&document)
    322321{
     
    333332{
    334333    ASSERT(isMainThread());
    335     // We set m_refCount to 1 before calling delete to avoid double destruction through use of Ref<T>/RefPtr<T>.
     334    // We set m_refCount to 2 before calling delete to avoid double destruction through use of Ref<T>/RefPtr<T>.
    336335    // This is a security mitigation in case of programmer errorm (caught by a debug assertion).
    337     ASSERT(m_refCount == 1);
     336    ASSERT(m_refCountAndParentBit == s_refCountIncrement);
    338337    ASSERT(m_deletionHasBegun);
    339338    ASSERT(!m_adoptionIsRequired);
     
    25242523void Node::removedLastRef()
    25252524{
     2525    // This avoids double destruction even when there is a programming error to use Ref<T> / RefPtr<T> on this node.
     2526    // There are debug assertions in Node::ref() / Node::deref() to catch such a programming error.
     2527    ASSERT(m_refCountAndParentBit == s_refCountIncrement);
     2528
    25262529    // An explicit check for Document here is better than a virtual function since it is
    25272530    // faster for non-Document nodes, and because the call to removedLastRef that is inlined
     
    25352538    m_deletionHasBegun = true;
    25362539#endif
    2537     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.)
    25382540    delete this;
    25392541}
  • trunk/Source/WebCore/dom/Node.h

    r242696 r243037  
    501501    void deref();
    502502    bool hasOneRef() const;
    503     int refCount() const;
     503    unsigned refCount() const;
    504504
    505505#ifndef NDEBUG
     
    619619    Node(Document&, ConstructionType);
    620620
     621    static constexpr uint32_t s_refCountIncrement = 2;
     622    static constexpr uint32_t s_refCountMask = ~0x1;
     623
    621624    virtual void addSubresourceAttributeURLs(ListHashSet<URL>&) const { }
    622625
     
    665668    void moveNodeToNewDocument(Document& oldDocument, Document& newDocument);
    666669
    667     int m_refCount;
     670    uint32_t m_refCountAndParentBit { s_refCountIncrement };
    668671    mutable uint32_t m_nodeFlags;
    669672
     
    696699    ASSERT(!m_inRemovedLastRefFunction);
    697700    ASSERT(!m_adoptionIsRequired);
    698     ++m_refCount;
     701    m_refCountAndParentBit += s_refCountIncrement;
    699702}
    700703
     
    702705{
    703706    ASSERT(isMainThread());
    704     ASSERT(m_refCount >= 0);
     707    ASSERT(refCount());
    705708    ASSERT(!m_deletionHasBegun);
    706709    ASSERT(!m_inRemovedLastRefFunction);
    707710    ASSERT(!m_adoptionIsRequired);
    708     if (--m_refCount <= 0 && !parentNode()) {
     711    auto tempRefCount = m_refCountAndParentBit - s_refCountIncrement;
     712    if (!tempRefCount) {
    709713#ifndef NDEBUG
    710714        m_inRemovedLastRefFunction = true;
    711715#endif
    712716        removedLastRef();
     717        return;
    713718    }
     719    m_refCountAndParentBit = tempRefCount;
    714720}
    715721
     
    718724    ASSERT(!m_deletionHasBegun);
    719725    ASSERT(!m_inRemovedLastRefFunction);
    720     return m_refCount == 1;
    721 }
    722 
    723 ALWAYS_INLINE int Node::refCount() const
    724 {
    725     return m_refCount;
     726    return refCount() == 1;
     727}
     728
     729ALWAYS_INLINE unsigned Node::refCount() const
     730{
     731    return m_refCountAndParentBit / s_refCountIncrement;
    726732}
    727733
     
    737743    ASSERT(isMainThread());
    738744    m_parentNode = parent;
     745    auto refCountWithoutParentBit = m_refCountAndParentBit & s_refCountMask;
     746    m_refCountAndParentBit = refCountWithoutParentBit | !!parent;
    739747}
    740748
Note: See TracChangeset for help on using the changeset viewer.