Bug 1393098 part 1: Adjust nsTextFrame::CharacterDataChanged to skip redundant requests for reflow, via a new boolean member-var. r=jfkthame
authorDaniel Holbert <dholbert@cs.stanford.edu>
Thu, 31 Aug 2017 10:45:38 -0700
changeset 378041 9c13f2c5b9515e456a2a9abd1107affeb3709b93
parent 378040 007b471778c310660823bf7680e116a828bcb1e5
child 378042 a3689ab6e5be6b53c2c018c6e2ecac4646ad979e
push id50166
push userdholbert@mozilla.com
push dateThu, 31 Aug 2017 20:36:35 +0000
treeherderautoland@9c13f2c5b951 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjfkthame
bugs1393098
milestone57.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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.