Bug 1427625: Optimize appends to avoid restyling unnecessarily. r=xidorn draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Tue, 27 Feb 2018 17:02:52 +0100
changeset 761457 3b760c274829a112120d41d1a49fd213ad7f2589
parent 761456 d4b47e577288cfadacd94a9706af31db83feef2d
push id100952
push userbmo:emilio@crisal.io
push dateThu, 01 Mar 2018 00:36:07 +0000
reviewersxidorn
bugs1427625
milestone60.0a1
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<bool> 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
dom/base/nsGenericDOMDataNode.cpp
layout/base/PresShell.cpp
layout/base/RestyleManager.cpp
layout/base/RestyleManager.h
--- a/dom/base/nsGenericDOMDataNode.cpp
+++ b/dom/base/nsGenericDOMDataNode.cpp
@@ -906,29 +906,35 @@ nsGenericDOMDataNode::TextIsOnlyWhitespa
 
 bool
 nsGenericDOMDataNode::ThreadSafeTextIsOnlyWhitespace() const
 {
   // FIXME: should this method take content language into account?
   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;
   }
 
   if (HasFlag(NS_CACHED_TEXT_IS_ONLY_WHITESPACE)) {
     return HasFlag(NS_TEXT_IS_ONLY_WHITESPACE);
   }
 
   const char* cp = mText.Get1b();
   const char* end = cp + mText.GetLength();
 
   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;
     }
 
     ++cp;
   }
 
   return true;
--- a/layout/base/PresShell.cpp
+++ b/layout/base/PresShell.cpp
@@ -4293,30 +4293,17 @@ PresShell::CharacterDataChanged(nsIDocum
                                 nsIContent* aContent,
                                 const CharacterDataChangeInfo& aInfo)
 {
   NS_PRECONDITION(!mIsDocumentGone, "Unexpected CharacterDataChanged");
   NS_PRECONDITION(aDocument == mDocument, "Unexpected 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;
 }
 
 void
 PresShell::ContentStateChanged(nsIDocument* aDocument,
                                nsIContent* aContent,
                                EventStates aStateMask)
--- a/layout/base/RestyleManager.cpp
+++ b/layout/base/RestyleManager.cpp
@@ -16,16 +16,17 @@
 #include "nsIFrame.h"
 #include "nsIFrameInlines.h"
 #include "nsIPresShellInlines.h"
 #include "nsPlaceholderFrame.h"
 #include "nsStyleChangeList.h"
 #include "nsStyleUtil.h"
 #include "StickyScrollContainer.h"
 #include "mozilla/EffectSet.h"
+#include "mozilla/IntegerRange.h"
 #include "mozilla/ViewportFrame.h"
 #include "SVGObserverUtils.h"
 #include "SVGTextFrame.h"
 #include "ActiveLayerTracker.h"
 #include "nsSVGIntegrationUtils.h"
 
 using namespace mozilla::dom;
 
@@ -48,37 +49,16 @@ void
 RestyleManager::ContentInserted(nsINode* aContainer, nsIContent* aChild)
 {
   RestyleForInsertOrChange(aContainer, 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()) {
     return;
   }
   Element* container = aContainer->AsElement();
 
 #ifdef DEBUG
   {
@@ -129,16 +109,72 @@ RestyleManager::RestyleForAppend(nsICont
       if (cur->IsElement()) {
         PostRestyleEvent(cur->AsElement(), eRestyle_Subtree, nsChangeHint(0));
         break;
       }
     }
   }
 }
 
+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).
 static void
 RestyleSiblingsStartingWith(RestyleManager* aRestyleManager,
                             nsIContent* aStartingSibling /* may be null */)
 {
   for (nsIContent* sibling = aStartingSibling; sibling;
@@ -148,16 +184,142 @@ RestyleSiblingsStartingWith(RestyleManag
         PostRestyleEvent(sibling->AsElement(),
                          nsRestyleHint(eRestyle_Subtree | eRestyle_LaterSiblings),
                          nsChangeHint(0));
       break;
     }
   }
 }
 
+template<typename CharT>
+bool
+WhitespaceOnly(const CharT* aBuffer, size_t aUpTo)
+{
+  for (auto index : IntegerRange(aUpTo)) {
+    if (!dom::IsSpaceCharacter(aBuffer[index])) {
+      return false;
+    }
+  }
+  return true;
+}
+
+template<typename CharT>
+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
 // CharacterDataChanged is more like a removal than an addition).
 // The comments are written and variables are named in terms of it being
 // a ContentInserted notification.
 void
 RestyleManager::RestyleForInsertOrChange(nsINode* aContainer,
@@ -171,33 +333,23 @@ RestyleManager::RestyleForInsertOrChange
 
   NS_ASSERTION(!aChild->IsRootOfAnonymousSubtree(),
                "anonymous nodes should not be in child lists");
   uint32_t selectorFlags = container->GetFlags() & NODE_ALL_SELECTOR_FLAGS;
   if (selectorFlags == 0)
     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;
     }
   }
 
   if (selectorFlags & NODE_HAS_SLOW_SELECTOR) {
     PostRestyleEvent(container, eRestyle_Subtree, nsChangeHint(0));
     // Restyling the container is the most we can do here, so we're done.
@@ -205,50 +357,17 @@ RestyleManager::RestyleForInsertOrChange
   }
 
   if (selectorFlags & NODE_HAS_SLOW_SELECTOR_LATER_SIBLINGS) {
     // Restyle all later siblings.
     RestyleSiblingsStartingWith(this, aChild->GetNextSibling());
   }
 
   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);
   }
 }
 
 void
 RestyleManager::ContentRemoved(nsINode* aContainer,
                                nsIContent* aOldChild,
                                nsIContent* aFollowingSibling)
 {
--- a/layout/base/RestyleManager.h
+++ b/layout/base/RestyleManager.h
@@ -154,24 +154,23 @@ public:
   // following sibling in addition to the old child.  |aContainer| must be
   // non-null; when the container is null, no work is needed.  aFollowingSibling
   // is the sibling that used to come after aOldChild before the removal.
   void ContentRemoved(nsINode* aContainer,
                       nsIContent* aOldChild,
                       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)
 
   inline void PostRestyleEvent(dom::Element* aElement,
                                nsRestyleHint aRestyleHint,
                                nsChangeHint aMinChangeHint);
   inline void RebuildAllStyleData(nsChangeHint aExtraHint,
                                   nsRestyleHint aRestyleHint);
@@ -218,16 +217,17 @@ protected:
 
   virtual ~RestyleManager()
   {
     MOZ_ASSERT(!mAnimationsWithDestroyedFrame,
                "leaving dangling pointers from AnimationsWithDestroyedFrame");
   }
 
   void RestyleForEmptyChange(Element* aContainer);
+  void MaybeRestyleForEdgeChildChange(Element* aContainer, nsIContent* aChangedChild);
 
   void ContentStateChangedInternal(Element* aElement,
                                    EventStates aStateMask,
                                    nsChangeHint* aOutChangeHint);
 
   bool IsDisconnected() { return mPresContext == nullptr; }
 
   void IncrementHoverGeneration() {