Changeset 207374 in webkit


Ignore:
Timestamp:
Oct 15, 2016 7:24:33 AM (8 years ago)
Author:
Alan Bujtas
Message:

CounterNode::resetRenderers is so inefficient.
​https://bugs.webkit.org/show_bug.cgi?id=163480

Reviewed by Simon Fraser.

CounterNode::resetRenderers() removes all the associated renderers from this CounterNode
and sets the dirty bit on them.
This patch does all that in a loop, instead of traversing the linked tree on each removal.

No change in functionality.

  • rendering/CounterNode.cpp:

(WebCore::CounterNode::CounterNode):
(WebCore::CounterNode::~CounterNode):
(WebCore::CounterNode::nextInPreOrderAfterChildren):
(WebCore::CounterNode::lastDescendant):
(WebCore::CounterNode::addRenderer): These assertions do not seem super useful.
(WebCore::CounterNode::removeRenderer):
(WebCore::CounterNode::resetRenderers):
(WebCore::CounterNode::insertAfter):
(WebCore::CounterNode::removeChild):

  • rendering/CounterNode.h:
  • rendering/RenderCounter.cpp:

(WebCore::makeCounterNode):
(WebCore::RenderCounter::RenderCounter):
(WebCore::RenderCounter::~RenderCounter):
(WebCore::RenderCounter::originalText):
(WebCore::updateCounters):
(WebCore::RenderCounter::invalidate): Deleted.

  • rendering/RenderCounter.h:
Location:
trunk/Source/WebCore
Files:
5 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r207373 r207374  
     12016-10-15  Zalan Bujtas  <zalan@apple.com>
     2
     3        CounterNode::resetRenderers is so inefficient.
     4        https://bugs.webkit.org/show_bug.cgi?id=163480
     5
     6        Reviewed by Simon Fraser.
     7
     8        CounterNode::resetRenderers() removes all the associated renderers from this CounterNode
     9        and sets the dirty bit on them.
     10        This patch does all that in a loop, instead of traversing the linked tree on each removal.
     11
     12        No change in functionality.
     13
     14        * rendering/CounterNode.cpp:
     15        (WebCore::CounterNode::CounterNode):
     16        (WebCore::CounterNode::~CounterNode):
     17        (WebCore::CounterNode::nextInPreOrderAfterChildren):
     18        (WebCore::CounterNode::lastDescendant):
     19        (WebCore::CounterNode::addRenderer): These assertions do not seem super useful.
     20        (WebCore::CounterNode::removeRenderer):
     21        (WebCore::CounterNode::resetRenderers):
     22        (WebCore::CounterNode::insertAfter):
     23        (WebCore::CounterNode::removeChild):
     24        * rendering/CounterNode.h:
     25        * rendering/RenderCounter.cpp:
     26        (WebCore::makeCounterNode):
     27        (WebCore::RenderCounter::RenderCounter):
     28        (WebCore::RenderCounter::~RenderCounter):
     29        (WebCore::RenderCounter::originalText):
     30        (WebCore::updateCounters):
     31        (WebCore::RenderCounter::invalidate): Deleted.
     32        * rendering/RenderCounter.h:
     33
    1342016-10-15  Antoine Quint  <graouts@apple.com>
    235

  • trunk/Source/WebCore/rendering/CounterNode.cpp

    r181166 r207374  
    3434    , m_countInParent(0)
    3535    , m_owner(owner)
    36     , m_rootRenderer(0)
    37     , m_parent(0)
    38     , m_previousSibling(0)
    39     , m_nextSibling(0)
    40     , m_firstChild(0)
    41     , m_lastChild(0)
    4236{
    4337}
    … …  
    4842    // so we need to handle these cases. The node is still connected to the tree so we need to detach it.
    4943    if (m_parent || m_previousSibling || m_nextSibling || m_firstChild || m_lastChild) {
    50         CounterNode* oldParent = 0;
    51         CounterNode* oldPreviousSibling = 0;
     44        CounterNode* oldParent = nullptr;
     45        CounterNode* oldPreviousSibling = nullptr;
    5246        // Instead of calling removeChild() we do this safely as the tree is likely broken if we get here.
    5347        if (m_parent) {
    … …  
    5751                m_parent->m_lastChild = m_previousSibling;
    5852            oldParent = m_parent;
    59             m_parent = 0;
     53            m_parent = nullptr;
    6054        }
    6155        if (m_previousSibling) {
    … …  
    6357                m_previousSibling->m_nextSibling = m_nextSibling;
    6458            oldPreviousSibling = m_previousSibling;
    65             m_previousSibling = 0;
     59            m_previousSibling = nullptr;
    6660        }
    6761        if (m_nextSibling) {
    6862            if (m_nextSibling->m_previousSibling == this)
    6963                m_nextSibling->m_previousSibling = oldPreviousSibling;
    70             m_nextSibling = 0;
     64            m_nextSibling = nullptr;
    7165        }
    7266        if (m_firstChild) {
    … …  
    7468            for (CounterNode* child = m_firstChild; child; ) {
    7569                CounterNode* nextChild = child->m_nextSibling;
    76                 CounterNode* nextSibling = 0;
     70                CounterNode* nextSibling = nullptr;
    7771                child->m_parent = oldParent;
    7872                if (oldPreviousSibling) {
    … …  
    9993{
    10094    if (this == stayWithin)
    101         return 0;
     95        return nullptr;
    10296
    10397    const CounterNode* current = this;
    … …  
    106100        current = current->m_parent;
    107101        if (!current || current == stayWithin)
    108             return 0;
     102            return nullptr;
    109103    }
    110104    return next;
    … …  
    123117    CounterNode* last = m_lastChild;
    124118    if (!last)
    125         return 0;
     119        return nullptr;
    126120
    127121    while (CounterNode* lastChild = last->m_lastChild)
    … …  
    152146}
    153147
    154 void CounterNode::addRenderer(RenderCounter* value)
    155 {
    156     if (!value) {
    157         ASSERT_NOT_REACHED();
     148void CounterNode::addRenderer(RenderCounter& renderer)
     149{
     150    ASSERT(!renderer.m_counterNode);
     151    ASSERT(!renderer.m_nextForSameCounter);
     152    renderer.m_nextForSameCounter = m_rootRenderer;
     153    m_rootRenderer = &renderer;
     154    renderer.m_counterNode = this;
     155}
     156
     157void CounterNode::removeRenderer(RenderCounter& renderer)
     158{
     159    ASSERT(renderer.m_counterNode && renderer.m_counterNode == this);
     160    RenderCounter* previous = nullptr;
     161    for (auto* current = m_rootRenderer; current; previous = current, current = current->m_nextForSameCounter) {
     162        if (current != &renderer)
     163            continue;
     164
     165        if (previous)
     166            previous->m_nextForSameCounter = renderer.m_nextForSameCounter;
     167        else
     168            m_rootRenderer = renderer.m_nextForSameCounter;
     169        renderer.m_nextForSameCounter = nullptr;
     170        renderer.m_counterNode = nullptr;
    158171        return;
    159172    }
    160     if (value->m_counterNode) {
    161         ASSERT_NOT_REACHED();
    162         value->m_counterNode->removeRenderer(value);
    163     }
    164     ASSERT(!value->m_nextForSameCounter);
    165     for (RenderCounter* iterator = m_rootRenderer;iterator; iterator = iterator->m_nextForSameCounter) {
    166         if (iterator == value) {
    167             ASSERT_NOT_REACHED();
    168             return;
    169         }
    170     }
    171     value->m_nextForSameCounter = m_rootRenderer;
    172     m_rootRenderer = value;
    173     if (value->m_counterNode != this) {
    174         if (value->m_counterNode) {
    175             ASSERT_NOT_REACHED();
    176             value->m_counterNode->removeRenderer(value);
    177         }
    178         value->m_counterNode = this;
    179     }
    180 }
    181 
    182 void CounterNode::removeRenderer(RenderCounter* value)
    183 {
    184     if (!value) {
    185         ASSERT_NOT_REACHED();
     173    ASSERT_NOT_REACHED();
     174}
     175
     176void CounterNode::resetRenderers()
     177{
     178    if (!m_rootRenderer)
    186179        return;
    187     }
    188     if (value->m_counterNode && value->m_counterNode != this) {
    189         ASSERT_NOT_REACHED();
    190         value->m_counterNode->removeRenderer(value);
    191     }
    192     RenderCounter* previous = 0;
    193     for (RenderCounter* iterator = m_rootRenderer;iterator; iterator = iterator->m_nextForSameCounter) {
    194         if (iterator == value) {
    195             if (previous)
    196                 previous->m_nextForSameCounter = value->m_nextForSameCounter;
    197             else
    198                 m_rootRenderer = value->m_nextForSameCounter;
    199             value->m_nextForSameCounter = 0;
    200             value->m_counterNode = 0;
    201             return;
    202         }
    203         previous = iterator;
    204     }
    205     ASSERT_NOT_REACHED();
    206 }
    207 
    208 void CounterNode::resetRenderers()
    209 {
    210     while (m_rootRenderer)
    211         m_rootRenderer->invalidate(); // This makes m_rootRenderer point to the next renderer if any since it disconnects the m_rootRenderer from this.
     180    bool skipLayoutAndPerfWidthsRecalc = m_rootRenderer->documentBeingDestroyed();
     181    auto* current = m_rootRenderer;
     182    while (current) {
     183        if (!skipLayoutAndPerfWidthsRecalc)
     184            current->setNeedsLayoutAndPrefWidthsRecalc();
     185        auto* next = current->m_nextForSameCounter;
     186        current->m_nextForSameCounter = nullptr;
     187        current->m_counterNode = nullptr;
     188        current = next;
     189    }
     190    m_rootRenderer = nullptr;
    212191}
    213192
    … …  
    313292        }
    314293    }
    315     newChild->m_firstChild = 0;
    316     newChild->m_lastChild = 0;
     294    newChild->m_firstChild = nullptr;
     295    newChild->m_lastChild = nullptr;
    317296    newChild->m_countInParent = newChild->computeCountInParent();
    318297    newChild->resetRenderers();
    … …  
    329308    CounterNode* previous = oldChild->m_previousSibling;
    330309
    331     oldChild->m_nextSibling = 0;
    332     oldChild->m_previousSibling = 0;
    333     oldChild->m_parent = 0;
     310    oldChild->m_nextSibling = nullptr;
     311    oldChild->m_previousSibling = nullptr;
     312    oldChild->m_parent = nullptr;
    334313
    335314    if (previous)

  • trunk/Source/WebCore/rendering/CounterNode.h

    r181166 r207374  
    5050    int countInParent() const { return m_countInParent; }
    5151    RenderElement& owner() const { return m_owner; }
    52     void addRenderer(RenderCounter*);
    53     void removeRenderer(RenderCounter*);
     52    void addRenderer(RenderCounter&);
     53    void removeRenderer(RenderCounter&);
    5454
    5555    // Invalidates the text in the renderers of this counter, if any.
    … …  
    6363    CounterNode* lastDescendant() const;
    6464    CounterNode* previousInPreOrder() const;
    65     CounterNode* nextInPreOrder(const CounterNode* stayWithin = 0) const;
    66     CounterNode* nextInPreOrderAfterChildren(const CounterNode* stayWithin = 0) const;
     65    CounterNode* nextInPreOrder(const CounterNode* stayWithin = nullptr) const;
     66    CounterNode* nextInPreOrderAfterChildren(const CounterNode* stayWithin = nullptr) const;
    6767
    6868    void insertAfter(CounterNode* newChild, CounterNode* beforeChild, const AtomicString& identifier);
    … …  
    8383    int m_countInParent;
    8484    RenderElement& m_owner;
    85     RenderCounter* m_rootRenderer;
     85    RenderCounter* m_rootRenderer { nullptr };
    8686
    87     CounterNode* m_parent;
    88     CounterNode* m_previousSibling;
    89     CounterNode* m_nextSibling;
    90     CounterNode* m_firstChild;
    91     CounterNode* m_lastChild;
     87    CounterNode* m_parent { nullptr };
     88    CounterNode* m_previousSibling { nullptr };
     89    CounterNode* m_nextSibling { nullptr };
     90    CounterNode* m_firstChild { nullptr };
     91    CounterNode* m_lastChild { nullptr };
    9292};
    9393

  • trunk/Source/WebCore/rendering/RenderCounter.cpp

    r194819 r207374  
    305305        return nullptr;
    306306
    307     RefPtr<CounterNode> newParent = 0;
    308     RefPtr<CounterNode> newPreviousSibling = 0;
     307    RefPtr<CounterNode> newParent;
     308    RefPtr<CounterNode> newPreviousSibling;
    309309    RefPtr<CounterNode> newNode = CounterNode::create(renderer, isReset, value);
    310310    if (findPlaceForCounter(renderer, identifier, isReset, newParent, newPreviousSibling))
    … …  
    346346    : RenderText(document, emptyString())
    347347    , m_counter(counter)
    348     , m_counterNode(nullptr)
    349     , m_nextForSameCounter(0)
    350348{
    351349    view().addRenderCounter();
    … …  
    357355
    358356    if (m_counterNode) {
    359         m_counterNode->removeRenderer(this);
     357        m_counterNode->removeRenderer(*this);
    360358        ASSERT(!m_counterNode);
    361359    }
    … …  
    386384            beforeAfterContainer = beforeAfterContainer->parent();
    387385        }
    388         makeCounterNode(*beforeAfterContainer, m_counter.identifier(), true)->addRenderer(const_cast<RenderCounter*>(this));
     386        makeCounterNode(*beforeAfterContainer, m_counter.identifier(), true)->addRenderer(const_cast<RenderCounter&>(*this));
    389387        ASSERT(m_counterNode);
    390388    }
    … …  
    426424
    427425    RenderText::computePreferredLogicalWidths(lead);
    428 }
    429 
    430 void RenderCounter::invalidate()
    431 {
    432     m_counterNode->removeRenderer(this);
    433     ASSERT(!m_counterNode);
    434     if (documentBeingDestroyed())
    435         return;
    436     setNeedsLayoutAndPrefWidthsRecalc();
    437426}
    438427
    … …  
    523512            continue;
    524513        }
    525         RefPtr<CounterNode> newParent = 0;
    526         RefPtr<CounterNode> newPreviousSibling = 0;
     514        RefPtr<CounterNode> newParent;
     515        RefPtr<CounterNode> newPreviousSibling;
    527516       
    528517        findPlaceForCounter(renderer, it->key, node->hasResetType(), newParent, newPreviousSibling);

  • trunk/Source/WebCore/rendering/RenderCounter.h

    r197563 r207374  
    5050    void computePreferredLogicalWidths(float leadWidth) override;
    5151
    52     // Removes the reference to the CounterNode associated with this renderer.
    53     // This is used to cause a counter display update when the CounterNode tree changes.
    54     void invalidate();
    55 
    5652    CounterContent m_counter;
    57     CounterNode* m_counterNode;
    58     RenderCounter* m_nextForSameCounter;
     53    CounterNode* m_counterNode { nullptr };
     54    RenderCounter* m_nextForSameCounter { nullptr };
    5955    friend class CounterNode;
    6056};
    … …  
    6662#if ENABLE(TREE_DEBUGGING)
    6763// Outside the WebCore namespace for ease of invocation from gdb.
    68 void showCounterRendererTree(const WebCore::RenderObject*, const char* counterName = 0);
     64void showCounterRendererTree(const WebCore::RenderObject*, const char* counterName = nullptr);
    6965#endif
    7066
Note: See TracChangeset for help on using the changeset viewer.