Bug 1014252 - Optimize clearing of textruns via RemoveInFlows. r=matspal, a=lmandel
authorRobert O'Callahan <robert@ocallahan.org>
Wed, 28 May 2014 15:39:25 +1200
changeset 199356 29cf176e9fe77cfb833b6066cf802d546f50d369
parent 199355 7050a0f3db6506025687d9af40047ed7e128f792
child 199357 30ac291a7ed63a187caa8dce0aac8cfe685a0577
push id3624
push userasasaki@mozilla.com
push dateMon, 09 Jun 2014 21:49:01 +0000
treeherdermozilla-beta@b1a5da15899a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmatspal, lmandel
bugs1014252
milestone31.0a2
Bug 1014252 - Optimize clearing of textruns via RemoveInFlows. r=matspal, a=lmandel We're keeping the core idea that, before we remove the frames-to-be-destroyed from the continuation chain, their textruns need to be disconnected/destroyed. However, nsContinuingTextFrame::DestroyFrom tries to optimize when the destroying frames that aren't mentioned in the userdata for the textrun, and certain other conditions are met; we need a similar optimization here. It's simpler here because the other conditions are definitely met, since all the text for the frames being deleted has already been consumed and reflowed by previous frames. We don't need the TEXT_STYLE_MATCHES_PREV_CONTINUATION state bit anymore because nsContinuingTextFrame::DestroyFrom will never see any textruns when called via RemoveEmptyInFlows.
layout/generic/nsFrameStateBits.h
layout/generic/nsTextFrame.cpp
layout/generic/nsTextFrame.h
--- a/layout/generic/nsFrameStateBits.h
+++ b/layout/generic/nsFrameStateBits.h
@@ -400,21 +400,16 @@ FRAME_STATE_BIT(Text, 31, TEXT_HAS_NONCO
 FRAME_STATE_BIT(Text, 32, TEXT_IS_IN_TOKEN_MATHML)
 
 // Set when this text frame is mentioned in the userdata for the
 // uninflated textrun property
 FRAME_STATE_BIT(Text, 60, TEXT_IN_UNINFLATED_TEXTRUN_USER_DATA)
 
 FRAME_STATE_BIT(Text, 61, TEXT_HAS_FONT_INFLATION)
 
-// If true, then this frame is being removed due to a SetLength() on a
-// previous continuation and the style context of that previous
-// continuation is the same as this frame's
-FRAME_STATE_BIT(Text, 62, TEXT_STYLE_MATCHES_PREV_CONTINUATION)
-
 // Whether this frame is cached in the Offset Frame Cache
 // (OffsetToFrameProperty)
 FRAME_STATE_BIT(Text, 63, TEXT_IN_OFFSET_CACHE)
 
 
 // == Frame state bits that apply to block frames =============================
 
 FRAME_STATE_GROUP(Block, nsBlockFrame)
--- a/layout/generic/nsTextFrame.cpp
+++ b/layout/generic/nsTextFrame.cpp
@@ -4036,20 +4036,17 @@ nsContinuingTextFrame::DestroyFrom(nsIFr
   // prev-continuation. If that means the text has changed style, then
   // we need to wipe out the text run for the text.
   // Note that mPrevContinuation can be null if we're destroying the whole
   // frame chain from the start to the end.
   // If this frame is mentioned in the userData for a textrun (say
   // because there's a direction change at the start of this frame), then
   // we have to clear the textrun because we're going away and the
   // textrun had better not keep a dangling reference to us.
-  if ((GetStateBits() & TEXT_IN_TEXTRUN_USER_DATA) ||
-      (GetStateBits() & TEXT_IN_UNINFLATED_TEXTRUN_USER_DATA) ||
-      (!mPrevContinuation &&
-       !(GetStateBits() & TEXT_STYLE_MATCHES_PREV_CONTINUATION)) ||
+  if (IsInTextRunUserData() ||
       (mPrevContinuation &&
        mPrevContinuation->StyleContext() != StyleContext())) {
     ClearTextRuns();
     // Clear the previous continuation's text run also, so that it can rebuild
     // the text run to include our text.
     if (mPrevContinuation) {
       nsTextFrame *prevContinuationText =
         static_cast<nsTextFrame*>(mPrevContinuation);
@@ -4348,16 +4345,27 @@ nsTextFrame::ClearTextRun(nsTextFrame* a
 
   if (!textRun->GetUserData()) {
     // Remove it now because it's not doing anything useful
     gTextRuns->RemoveFromCache(textRun);
     delete textRun;
   }
 }
 
+void
+nsTextFrame::DisconnectTextRuns()
+{
+  MOZ_ASSERT(!IsInTextRunUserData(),
+             "Textrun mentions this frame in its user data so we can't just disconnect");
+  mTextRun = nullptr;
+  if ((GetStateBits() & TEXT_HAS_FONT_INFLATION)) {
+    Properties().Delete(UninflatedTextRunProperty());
+  }
+}
+
 nsresult
 nsTextFrame::CharacterDataChanged(CharacterDataChangeInfo* aInfo)
 {
   mContent->DeleteProperty(nsGkAtoms::newline);
   if (PresContext()->BidiEnabled()) {
     mContent->DeleteProperty(nsGkAtoms::flowlength);
   }
 
@@ -7357,18 +7365,22 @@ HasSoftHyphenBefore(const nsTextFragment
     if (!iter.IsOriginalCharSkipped())
       break;
     if (aFrag->CharAt(iter.GetOriginalOffset()) == CH_SHY)
       return true;
   }
   return false;
 }
 
+/**
+ * Removes all frames from aFrame up to (but not including) aFirstToNotRemove,
+ * because their text has all been taken and reflowed by earlier frames.
+ */
 static void
-RemoveInFlows(nsTextFrame* aFrame, nsTextFrame* aFirstToNotRemove)
+RemoveEmptyInFlows(nsTextFrame* aFrame, nsTextFrame* aFirstToNotRemove)
 {
   NS_PRECONDITION(aFrame != aFirstToNotRemove, "This will go very badly");
   // We have to be careful here, because some RemoveFrame implementations
   // remove and destroy not only the passed-in frame but also all its following
   // in-flows (and sometimes all its following continuations in general).  So
   // we remove |f| and everything up to but not including firstToNotRemove from
   // the flow first, to make sure that only the things we want destroyed are
   // destroyed.
@@ -7384,26 +7396,28 @@ RemoveInFlows(nsTextFrame* aFrame, nsTex
   NS_ASSERTION(aFrame->GetPrevContinuation() ==
                aFrame->GetPrevInFlow() &&
                aFrame->GetPrevInFlow() != nullptr,
                "aFrame should have a fluid prev continuation");
   
   nsIFrame* prevContinuation = aFrame->GetPrevContinuation();
   nsIFrame* lastRemoved = aFirstToNotRemove->GetPrevContinuation();
 
-  // Clear the text run on the first frame we'll remove to make sure none of
-  // the frames we keep shares its text run.  We need to do this now, before
-  // we unlink the frames to remove from the flow, because DestroyFrom calls
-  // ClearTextRuns() and that will start at the first frame with the text
-  // run and walk the continuations.  We only need to care about the first
-  // and last frames we remove since text runs are contiguous.
-  aFrame->ClearTextRuns();
-  if (aFrame != lastRemoved) {
-    // Clear the text run on the last frame we'll remove for the same reason.
-    static_cast<nsTextFrame*>(lastRemoved)->ClearTextRuns();
+  for (nsTextFrame* f = aFrame; f != aFirstToNotRemove;
+       f = static_cast<nsTextFrame*>(f->GetNextContinuation())) {
+    // f is going to be destroyed soon, after it is unlinked from the
+    // continuation chain. If its textrun is going to be destroyed we need to
+    // do it now, before we unlink the frames to remove from the flow,
+    // because DestroyFrom calls ClearTextRuns() and that will start at the
+    // first frame with the text run and walk the continuations.
+    if (f->IsInTextRunUserData()) {
+      f->ClearTextRuns();
+    } else {
+      f->DisconnectTextRuns();
+    }
   }
 
   prevContinuation->SetNextInFlow(aFirstToNotRemove);
   aFirstToNotRemove->SetPrevInFlow(prevContinuation);
 
   aFrame->SetPrevInFlow(nullptr);
   lastRemoved->SetNextInFlow(nullptr);
 
@@ -7502,36 +7516,29 @@ nsTextFrame::SetLength(int32_t aLength, 
       // rebuilding textruns for all following continuations.
       // We skip this optimization if we were called during bidi resolution,
       // since the bidi resolver may try to handle the destroyed frame later
       // and crash
       if (!framesToRemove) {
         // Remember that we have to remove this frame.
         framesToRemove = f;
       }
-
-      // Important: if |f| has the same style context as its prev continuation,
-      // mark it accordingly so we can skip clearing textruns as needed.  Note
-      // that at this point f always has a prev continuation.
-      if (f->StyleContext() == f->GetPrevContinuation()->StyleContext()) {
-        f->AddStateBits(TEXT_STYLE_MATCHES_PREV_CONTINUATION);
-      }
     } else if (framesToRemove) {
-      RemoveInFlows(framesToRemove, f);
+      RemoveEmptyInFlows(framesToRemove, f);
       framesToRemove = nullptr;
     }
     f = next;
   }
   NS_POSTCONDITION(!framesToRemove || (f && f->mContentOffset == end),
                    "How did we exit the loop if we null out framesToRemove if "
                    "!next || next->mContentOffset > end ?");
   if (framesToRemove) {
     // We are guaranteed that we exited the loop with f not null, per the
     // postcondition above
-    RemoveInFlows(framesToRemove, f);
+    RemoveEmptyInFlows(framesToRemove, f);
   }
 
 #ifdef DEBUG
   f = this;
   int32_t iterations = 0;
   while (f && iterations < 10) {
     f->GetContentLength(); // Assert if negative length
     f = static_cast<nsTextFrame*>(f->GetNextContinuation());
--- a/layout/generic/nsTextFrame.h
+++ b/layout/generic/nsTextFrame.h
@@ -463,16 +463,20 @@ public:
   gfxTextRun* GetTextRun(TextRunType aWhichTextRun) {
     if (aWhichTextRun == eInflated || !HasFontSizeInflation())
       return mTextRun;
     return GetUninflatedTextRun();
   }
   gfxTextRun* GetUninflatedTextRun();
   void SetTextRun(gfxTextRun* aTextRun, TextRunType aWhichTextRun,
                   float aInflation);
+  bool IsInTextRunUserData() const {
+    return GetStateBits() &
+      (TEXT_IN_TEXTRUN_USER_DATA | TEXT_IN_UNINFLATED_TEXTRUN_USER_DATA);
+  }
   /**
    * Notify the frame that it should drop its pointer to a text run.
    * Returns whether the text run was removed (i.e., whether it was
    * associated with this frame, either as its inflated or non-inflated
    * text run.
    */
   bool RemoveTextRun(gfxTextRun* aTextRun);
   /**
@@ -487,16 +491,21 @@ public:
 
   void ClearTextRuns() {
     ClearTextRun(nullptr, nsTextFrame::eInflated);
     if (HasFontSizeInflation()) {
       ClearTextRun(nullptr, nsTextFrame::eNotInflated);
     }
   }
 
+  /**
+   * Wipe out references to textrun(s) without deleting the textruns.
+   */
+  void DisconnectTextRuns();
+
   // Get the DOM content range mapped by this frame after excluding
   // whitespace subject to start-of-line and end-of-line trimming.
   // The textrun must have been created before calling this.
   struct TrimmedOffsets {
     int32_t mStart;
     int32_t mLength;
     int32_t GetEnd() const { return mStart + mLength; }
   };