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
--- 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.