From a133a50adae4562da0521399d9404458331d8c46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Tue, 27 Feb 2018 17:02:52 +0100 Subject: [PATCH] Bug 1427625: Optimize appends to avoid restyling unnecessarily. r=xidorn This unfortunately doesn't fix my test-case (because we're replacing the text content all the time and all that), but it's still worth it, since it fixes the case we care about (the parser appending). We could also optimize pure insertions (since in that case we can still figure out what the old text was), but it's probably annoying and not worth the churn. In any case, we cannot optimize anything that resembles any kind of removal, because from there we don't know the old text in any way (and the text nodes like to reuse string buffers and such). We could do two other optimizations to replace / extend this one, in that order: * Pass the buffer and length to CharacterDataWillChange, and use that to get the exact old text and the new one in RestyleManager. That would make the optimization exact. * Pass some sort of Maybe mWasWhitespace down the CharacterDataChangeInfo which is computed like: HasFlag(NS_CACHED_TEXT_IS_ONLY_WHITESPACE) ? Some(NS_TEXT_IS_ONLY_WHITESPACE) : Nothing() It's not clear to me it's going to be completely worth the churn, so I haven't done those yet, if we see code in the wild which resembles my testcase, we can think of doing it. MozReview-Commit-ID: 2rTWaZti8rv --HG-- extra : rebase_source : 7390b8740801eb7b91700bb2533c43c173ac5db9 --- dom/base/nsGenericDOMDataNode.cpp | 6 + layout/base/PresShell.cpp | 15 +- layout/base/RestyleManager.cpp | 261 ++++++++++++++++++++++-------- layout/base/RestyleManager.h | 10 +- 4 files changed, 202 insertions(+), 90 deletions(-) diff --git a/dom/base/nsGenericDOMDataNode.cpp b/dom/base/nsGenericDOMDataNode.cpp index f27dcb39b01f..73bd2439e80d 100644 --- a/dom/base/nsGenericDOMDataNode.cpp +++ b/dom/base/nsGenericDOMDataNode.cpp @@ -911,6 +911,10 @@ nsGenericDOMDataNode::ThreadSafeTextIsOnlyWhitespace() const if (mText.Is2b()) { // The fragment contains non-8bit characters and such characters // are never considered whitespace. + // + // FIXME(emilio): This is not quite true in presence of the + // NS_MAYBE_MODIFIED_FREQUENTLY flag... But looks like we only set that on + // anonymous nodes, so should be fine... return false; } @@ -924,6 +928,8 @@ nsGenericDOMDataNode::ThreadSafeTextIsOnlyWhitespace() const while (cp < end) { char ch = *cp; + // NOTE(emilio): If you ever change the definition of "whitespace" here, you + // need to change it too in RestyleManager::CharacterDataChanged. if (!dom::IsSpaceCharacter(ch)) { return false; } diff --git a/layout/base/PresShell.cpp b/layout/base/PresShell.cpp index f325379c41c9..e632387c021f 100644 --- a/layout/base/PresShell.cpp +++ b/layout/base/PresShell.cpp @@ -4298,20 +4298,7 @@ PresShell::CharacterDataChanged(nsIDocument* aDocument, nsAutoCauseReflowNotifier crNotifier(this); - // Call this here so it only happens for real content mutations and - // not cases when the frame constructor calls its own methods to force - // frame reconstruction. - nsIContent *container = aContent->GetParent(); - uint32_t selectorFlags = - container ? (container->GetFlags() & NODE_ALL_SELECTOR_FLAGS) : 0; - if (selectorFlags != 0 && !aContent->IsRootOfAnonymousSubtree()) { - Element* element = container->AsElement(); - if (aInfo.mAppend && !aContent->GetNextSibling()) - mPresContext->RestyleManager()->RestyleForAppend(element, aContent); - else - mPresContext->RestyleManager()->RestyleForInsertOrChange(element, aContent); - } - + mPresContext->RestyleManager()->CharacterDataChanged(aContent, aInfo); mFrameConstructor->CharacterDataChanged(aContent, aInfo); VERIFY_STYLE_TREE; } diff --git a/layout/base/RestyleManager.cpp b/layout/base/RestyleManager.cpp index 0acb44db4370..d66a1334ce95 100644 --- a/layout/base/RestyleManager.cpp +++ b/layout/base/RestyleManager.cpp @@ -21,6 +21,7 @@ #include "nsStyleUtil.h" #include "StickyScrollContainer.h" #include "mozilla/EffectSet.h" +#include "mozilla/IntegerRange.h" #include "mozilla/ViewportFrame.h" #include "SVGObserverUtils.h" #include "SVGTextFrame.h" @@ -52,27 +53,6 @@ RestyleManager::ContentInserted(nsINode* aContainer, nsIContent* aChild) void RestyleManager::ContentAppended(nsIContent* aContainer, nsIContent* aFirstNewContent) -{ - RestyleForAppend(aContainer, aFirstNewContent); -} - -void -RestyleManager::RestyleForEmptyChange(Element* aContainer) -{ - // In some cases (:empty + E, :empty ~ E), a change in the content of - // an element requires restyling its parent's siblings. - nsRestyleHint hint = eRestyle_Subtree; - nsIContent* grandparent = aContainer->GetParent(); - if (grandparent && - (grandparent->GetFlags() & NODE_HAS_SLOW_SELECTOR_LATER_SIBLINGS)) { - hint = nsRestyleHint(hint | eRestyle_LaterSiblings); - } - PostRestyleEvent(aContainer, hint, nsChangeHint(0)); -} - -void -RestyleManager::RestyleForAppend(nsIContent* aContainer, - nsIContent* aFirstNewContent) { // The container cannot be a document, but might be a ShadowRoot. if (!aContainer->IsElement()) { @@ -134,6 +114,62 @@ RestyleManager::RestyleForAppend(nsIContent* aContainer, } } +void +RestyleManager::RestyleForEmptyChange(Element* aContainer) +{ + // In some cases (:empty + E, :empty ~ E), a change in the content of + // an element requires restyling its parent's siblings. + nsRestyleHint hint = eRestyle_Subtree; + nsIContent* grandparent = aContainer->GetParent(); + if (grandparent && + (grandparent->GetFlags() & NODE_HAS_SLOW_SELECTOR_LATER_SIBLINGS)) { + hint = nsRestyleHint(hint | eRestyle_LaterSiblings); + } + PostRestyleEvent(aContainer, hint, nsChangeHint(0)); +} + +void +RestyleManager::MaybeRestyleForEdgeChildChange(Element* aContainer, + nsIContent* aChangedChild) +{ + MOZ_ASSERT(aContainer->GetFlags() & NODE_HAS_EDGE_CHILD_SELECTOR); + MOZ_ASSERT(aChangedChild->GetParent() == aContainer); + // restyle the previously-first element child if it is after this node + bool passedChild = false; + for (nsIContent* content = aContainer->GetFirstChild(); + content; + content = content->GetNextSibling()) { + if (content == aChangedChild) { + passedChild = true; + continue; + } + if (content->IsElement()) { + if (passedChild) { + PostRestyleEvent(content->AsElement(), eRestyle_Subtree, + nsChangeHint(0)); + } + break; + } + } + // restyle the previously-last element child if it is before this node + passedChild = false; + for (nsIContent* content = aContainer->GetLastChild(); + content; + content = content->GetPreviousSibling()) { + if (content == aChangedChild) { + passedChild = true; + continue; + } + if (content->IsElement()) { + if (passedChild) { + PostRestyleEvent(content->AsElement(), eRestyle_Subtree, + nsChangeHint(0)); + } + break; + } + } +} + // Needed since we can't use PostRestyleEvent on non-elements (with // eRestyle_LaterSiblings or nsRestyleHint(eRestyle_Subtree | // eRestyle_LaterSiblings) as appropriate). @@ -153,6 +189,132 @@ RestyleSiblingsStartingWith(RestyleManager* aRestyleManager, } } +template +bool +WhitespaceOnly(const CharT* aBuffer, size_t aUpTo) +{ + for (auto index : IntegerRange(aUpTo)) { + if (!dom::IsSpaceCharacter(aBuffer[index])) { + return false; + } + } + return true; +} + +template +bool +WhitespaceOnlyChangedOnAppend(const CharT* aBuffer, + size_t aOldLength, + size_t aNewLength) +{ + MOZ_ASSERT(aOldLength < aNewLength); + if (!WhitespaceOnly(aBuffer, aOldLength)) { + // The old text was already not whitespace-only. + return false; + } + + return !WhitespaceOnly(aBuffer + aOldLength, aNewLength - aOldLength); +} + +static bool +HasAnySignificantSibling(Element* aContainer, nsIContent* aChild) +{ + MOZ_ASSERT(aChild->GetParent() == aContainer); + for (nsIContent* child = aContainer->GetFirstChild(); + child; + child = child->GetNextSibling()) { + if (child == aChild) { + continue; + } + // We don't know whether we're testing :empty or :-moz-only-whitespace, + // so be conservative and assume :-moz-only-whitespace (i.e., make + // IsSignificantChild less likely to be true, and thus make us more + // likely to restyle). + if (nsStyleUtil::IsSignificantChild(child, true, false)) { + return true; + } + } + + return false; +} + +void +RestyleManager::CharacterDataChanged(nsIContent* aContent, + const CharacterDataChangeInfo& aInfo) +{ + nsINode* parent = aContent->GetParentNode(); + MOZ_ASSERT(parent, "How were we notified of a stray node?"); + + uint32_t slowSelectorFlags = parent->GetFlags() & NODE_ALL_SELECTOR_FLAGS; + if (!(slowSelectorFlags & (NODE_HAS_EMPTY_SELECTOR | + NODE_HAS_EDGE_CHILD_SELECTOR))) { + // Nothing to do, no other slow selector can change as a result of this. + return; + } + + if (!aContent->IsNodeOfType(nsINode::eTEXT)) { + // Doesn't matter to styling (could be a processing instruction or a + // comment), it can't change whether any selectors match or don't. + return; + } + + + if (MOZ_UNLIKELY(!parent->IsElement())) { + MOZ_ASSERT(parent->IsShadowRoot()); + return; + } + + if (MOZ_UNLIKELY(aContent->IsRootOfAnonymousSubtree())) { + // This is an anonymous node and thus isn't in child lists, so isn't taken + // into account for selector matching the relevant selectors here. + return; + } + + // Handle appends specially since they're common and we can know both the old + // and the new text exactly. + // + // TODO(emilio): This could be made much more general if :-moz-only-whitespace + // / :-moz-first-node and :-moz-last-node didn't exist. In that case we only + // need to know whether we went from empty to non-empty, and that's trivial to + // know, with CharacterDataChangeInfo... + if (!aInfo.mAppend) { + // FIXME(emilio): This restyles unnecessarily if the text node is the only + // child of the parent element. Fortunately, it's uncommon to have such + // nodes and this not being an append. + // + // See the testcase in bug 1427625 for a test-case that triggers this. + RestyleForInsertOrChange(parent->AsElement(), aContent); + return; + } + + const nsTextFragment* text = aContent->GetText(); + + const size_t oldLength = aInfo.mChangeStart; + const size_t newLength = text->GetLength(); + + const bool emptyChanged = !oldLength && newLength; + + const bool whitespaceOnlyChanged = text->Is2b() + ? WhitespaceOnlyChangedOnAppend(text->Get2b(), oldLength, newLength) + : WhitespaceOnlyChangedOnAppend(text->Get1b(), oldLength, newLength); + + if (!emptyChanged && !whitespaceOnlyChanged) { + return; + } + + if (slowSelectorFlags & NODE_HAS_EMPTY_SELECTOR) { + if (!HasAnySignificantSibling(parent->AsElement(), aContent)) { + // We used to be empty, restyle the parent. + RestyleForEmptyChange(parent->AsElement()); + return; + } + } + + if (slowSelectorFlags & NODE_HAS_EDGE_CHILD_SELECTOR) { + MaybeRestyleForEdgeChildChange(parent->AsElement(), aContent); + } +} + // Restyling for a ContentInserted or CharacterDataChanged notification. // This could be used for ContentRemoved as well if we got the // notification before the removal happened (and sometimes @@ -176,23 +338,13 @@ RestyleManager::RestyleForInsertOrChange(nsINode* aContainer, return; if (selectorFlags & NODE_HAS_EMPTY_SELECTOR) { - // see whether we need to restyle the container - bool wasEmpty = true; // :empty or :-moz-only-whitespace - for (nsIContent* child = container->GetFirstChild(); - child; - child = child->GetNextSibling()) { - if (child == aChild) - continue; - // We don't know whether we're testing :empty or :-moz-only-whitespace, - // so be conservative and assume :-moz-only-whitespace (i.e., make - // IsSignificantChild less likely to be true, and thus make us more - // likely to restyle). - if (nsStyleUtil::IsSignificantChild(child, true, false)) { - wasEmpty = false; - break; - } - } + // See whether we need to restyle the container due to :empty / + // :-moz-only-whitespace. + const bool wasEmpty = !HasAnySignificantSibling(container, aChild); if (wasEmpty) { + // FIXME(emilio): When coming from CharacterDataChanged this can restyle + // unnecessarily. Also can restyle unnecessarily if aChild is not + // significant anyway, though that's more unlikely. RestyleForEmptyChange(container); return; } @@ -210,40 +362,7 @@ RestyleManager::RestyleForInsertOrChange(nsINode* aContainer, } if (selectorFlags & NODE_HAS_EDGE_CHILD_SELECTOR) { - // restyle the previously-first element child if it is after this node - bool passedChild = false; - for (nsIContent* content = container->GetFirstChild(); - content; - content = content->GetNextSibling()) { - if (content == aChild) { - passedChild = true; - continue; - } - if (content->IsElement()) { - if (passedChild) { - PostRestyleEvent(content->AsElement(), eRestyle_Subtree, - nsChangeHint(0)); - } - break; - } - } - // restyle the previously-last element child if it is before this node - passedChild = false; - for (nsIContent* content = container->GetLastChild(); - content; - content = content->GetPreviousSibling()) { - if (content == aChild) { - passedChild = true; - continue; - } - if (content->IsElement()) { - if (passedChild) { - PostRestyleEvent(content->AsElement(), eRestyle_Subtree, - nsChangeHint(0)); - } - break; - } - } + MaybeRestyleForEdgeChildChange(container, aChild); } } diff --git a/layout/base/RestyleManager.h b/layout/base/RestyleManager.h index f90ca3ff7a43..d6d9075f9a65 100644 --- a/layout/base/RestyleManager.h +++ b/layout/base/RestyleManager.h @@ -159,14 +159,13 @@ public: nsIContent* aFollowingSibling); // Restyling for a ContentInserted (notification after insertion) or - // for a CharacterDataChanged. |aContainer| must be non-null; when + // for some CharacterDataChanged. |aContainer| must be non-null; when // the container is null, no work is needed. void RestyleForInsertOrChange(nsINode* aContainer, nsIContent* aChild); - // Restyling for a ContentAppended (notification after insertion) or - // for a CharacterDataChanged. |aContainer| must be non-null; when - // the container is null, no work is needed. - void RestyleForAppend(nsIContent* aContainer, nsIContent* aFirstNewContent); + // Restyle for a CharacterDataChanged notification. In practice this can only + // affect :empty / :-moz-only-whitespace / :-moz-first-node / :-moz-last-node. + void CharacterDataChanged(nsIContent*, const CharacterDataChangeInfo&); MOZ_DECL_STYLO_METHODS(GeckoRestyleManager, ServoRestyleManager) @@ -223,6 +222,7 @@ protected: } void RestyleForEmptyChange(Element* aContainer); + void MaybeRestyleForEdgeChildChange(Element* aContainer, nsIContent* aChangedChild); void ContentStateChangedInternal(Element* aElement, EventStates aStateMask,