Bug 1393098 part 1: Adjust nsTextFrame::CharacterDataChanged to skip redundant requests for reflow, via a new boolean member-var. r?jfkthame draft
authorDaniel Holbert <dholbert@cs.stanford.edu>
Thu, 31 Aug 2017 10:45:38 -0700
changeset 656787 3a59e1e8d9bd814b13c4be363a0ace95e3a07cdb
parent 656786 7ff12753990e4ffd88c4d30e8e43d45fd5030520
child 729237 15e9c19f744d49faaa578b3a1c2193bfe5b89d4d
push id77318
push userdholbert@mozilla.com
push dateThu, 31 Aug 2017 17:45:48 +0000
reviewersjfkthame
bugs1393098
milestone57.0a1
Bug 1393098 part 1: Adjust nsTextFrame::CharacterDataChanged to skip redundant requests for reflow, via a new boolean member-var. r?jfkthame Some benchmarks & use-cases cause nsTextFrame::CharacterDataChanged to be called multiple times for the same text between reflows. Each call triggers a slightly-expensive call to shell->FrameNeedsReflow(), for each affected nsTextFrame in the continuation chain. (OK, it's not quite that bad -- we skip the FrameNeedsReflow calls for siblings, since the ancestor notifications/tweaks would all be the same.) This patch makes us set a flag on the nsTextFrame to indicate that a reflow has *already* been requested by this chunk of code, and we'll now use that to skip the FrameNeedsReflow() call (and the dirty-bit-setting for siblings) on the next invocation. And we clear this new flag when the pending reflow actually happens. This shouldn't change behavior in a web-observable way, but it should speed things up by removing redundant work. MozReview-Commit-ID: 5nmbZHEFFDi
layout/generic/nsIFrame.h
layout/generic/nsTextFrame.cpp
--- a/layout/generic/nsIFrame.h
+++ b/layout/generic/nsIFrame.h
@@ -617,16 +617,17 @@ public:
     , mPrevSibling(nullptr)
     , mState(NS_FRAME_FIRST_REFLOW | NS_FRAME_IS_DIRTY)
     , mClass(aID)
     , mMayHaveRoundedCorners(false)
     , mHasImageRequest(false)
     , mHasFirstLetterChild(false)
     , mParentIsWrapperAnonBox(false)
     , mIsWrapperBoxNeedingRestyle(false)
+    , mReflowRequestedForCharDataChange(false)
   {
     mozilla::PodZero(&mOverflow);
   }
 
   nsPresContext* PresContext() const {
     return StyleContext()->PresContext();
   }
 
@@ -4166,17 +4167,34 @@ protected:
 
   /**
    * True if this is a wrapper anonymous box needing a restyle.  This is used to
    * track, during stylo post-traversal, whether we've already recomputed the
    * style of this anonymous box, if we end up seeing it twice.
    */
   bool mIsWrapperBoxNeedingRestyle : 1;
 
-  // There is an 11-bit gap left here.
+  /**
+   * This bit is used in nsTextFrame::CharacterDataChanged() as an optimization
+   * to skip redundant reflow-requests when the character data changes multiple
+   * times between reflows. If this flag is set, then it implies that the
+   * NS_FRAME_IS_DIRTY state bit is also set (and that intrinsic sizes have
+   * been marked as dirty on our ancestor chain).
+   *
+   * XXXdholbert This bit is *only* used on nsTextFrame, but it lives here on
+   * nsIFrame simply because this is where we've got unused state bits
+   * available in a gap. If bits become more scarce, we should perhaps consider
+   * expanding the range of frame-specific state bits in nsFrameStateBits.h and
+   * moving this to be one of those (e.g. by swapping one of the adjacent
+   * general-purpose bits to take the place of this bool:1 here, so we can grow
+   * that range of frame-specific bits by 1).
+   */
+  bool mReflowRequestedForCharDataChange : 1;
+
+  // There is a 10-bit gap left here.
 
   // Helpers
   /**
    * Can we stop inside this frame when we're skipping non-rendered whitespace?
    * @param  aForward [in] Are we moving forward (or backward) in content order.
    * @param  aOffset [in/out] At what offset into the frame to start looking.
    *         on output - what offset was reached (whether or not we found a place to stop).
    * @return STOP: An appropriate offset was found within this frame,
--- a/layout/generic/nsTextFrame.cpp
+++ b/layout/generic/nsTextFrame.cpp
@@ -4857,17 +4857,18 @@ nsTextFrame::CharacterDataChanged(Charac
     next = textFrame->GetNextContinuation();
     if (!next || next->GetContentOffset() > int32_t(aInfo->mChangeStart))
       break;
     textFrame = next;
   }
 
   int32_t endOfChangedText = aInfo->mChangeStart + aInfo->mReplaceLength;
 
-  // Parent of the last frame that we passed to FrameNeedsReflow.
+  // Parent of the last frame that we passed to FrameNeedsReflow (or noticed
+  // had already received an earlier FrameNeedsReflow call).
   // (For subsequent frames with this same parent, we can just set their
   // dirty bit without bothering to call FrameNeedsReflow again.)
   nsIFrame* lastDirtiedFrameParent = nullptr;
 
   nsIPresShell* shell = PresContext()->GetPresShell();
   do {
     // textFrame contained deleted text (or the insertion point,
     // if this was a pure insertion).
@@ -4879,28 +4880,38 @@ nsTextFrame::CharacterDataChanged(Charac
     if (lastDirtiedFrameParent == parentOfTextFrame) {
       // An earlier iteration of this loop already called
       // FrameNeedsReflow for a sibling of |textFrame|.
       areAncestorsAwareOfReflowRequest = true;
     } else {
       lastDirtiedFrameParent = parentOfTextFrame;
     }
 
-    if (!areAncestorsAwareOfReflowRequest) {
-      // Ask the parent frame to reflow me.
-      shell->FrameNeedsReflow(textFrame, nsIPresShell::eStyleChange,
-                              NS_FRAME_IS_DIRTY);
+    if (textFrame->mReflowRequestedForCharDataChange) {
+      // We already requested a reflow for this frame; nothing to do.
+      MOZ_ASSERT(textFrame->HasAnyStateBits(NS_FRAME_IS_DIRTY),
+                 "mReflowRequestedForCharDataChange should only be set "
+                 "on dirty frames");
     } else {
-      // We already called FrameNeedsReflow on behalf of an earlier sibling,
-      // so we can just mark this frame as dirty and don't need to bother
-      // telling its ancestors.
-      // Note: if the parent is a block, we're cheating here because we should
-      // be marking our line dirty, but we're not. nsTextFrame::SetLength will
-      // do that when it gets called during reflow.
-      textFrame->AddStateBits(NS_FRAME_IS_DIRTY);
+      // Make sure textFrame is queued up for a reflow.  Also set a flag so we
+      // don't waste time doing this again in repeated calls to this method.
+      textFrame->mReflowRequestedForCharDataChange = true;
+      if (!areAncestorsAwareOfReflowRequest) {
+        // Ask the parent frame to reflow me.
+        shell->FrameNeedsReflow(textFrame, nsIPresShell::eStyleChange,
+                                NS_FRAME_IS_DIRTY);
+      } else {
+        // We already called FrameNeedsReflow on behalf of an earlier sibling,
+        // so we can just mark this frame as dirty and don't need to bother
+        // telling its ancestors.
+        // Note: if the parent is a block, we're cheating here because we should
+        // be marking our line dirty, but we're not. nsTextFrame::SetLength will
+        // do that when it gets called during reflow.
+        textFrame->AddStateBits(NS_FRAME_IS_DIRTY);
+      }
     }
     textFrame->InvalidateFrame();
 
     // Below, frames that start after the deleted text will be adjusted so that
     // their offsets move with the trailing unchanged text. If this change
     // deletes more text than it inserts, those frame offsets will decrease.
     // We need to maintain the invariant that mContentOffset is non-decreasing
     // along the continuation chain. So we need to ensure that frames that
@@ -9398,18 +9409,20 @@ nsTextFrame::ReflowText(nsLineLayout& aL
 #endif
 
   /////////////////////////////////////////////////////////////////////
   // Set up flags and clear out state
   /////////////////////////////////////////////////////////////////////
 
   // Clear out the reflow state flags in mState. We also clear the whitespace
   // flags because this can change whether the frame maps whitespace-only text
-  // or not.
+  // or not. We also clear the flag that tracks whether we had a pending
+  // reflow request from CharacterDataChanged (since we're reflowing now).
   RemoveStateBits(TEXT_REFLOW_FLAGS | TEXT_WHITESPACE_FLAGS);
+  mReflowRequestedForCharDataChange = false;
 
   // Temporarily map all possible content while we construct our new textrun.
   // so that when doing reflow our styles prevail over any part of the
   // textrun we look at. Note that next-in-flows may be mapping the same
   // content; gfxTextRun construction logic will ensure that we take priority.
   int32_t maxContentLength = GetInFlowContentLength();
 
   // We don't need to reflow if there is no content.