Bug 1579929: When a reflow is interrupted, don't purge flex item measurements until the next time they're needed in a later non-interrupted reflow. r=emilio
authorDaniel Holbert <dholbert@cs.stanford.edu>
Mon, 18 Nov 2019 06:49:06 +0000
changeset 502376 34f0f8db631a206ab5115baa73c13bcd7c2f36ea
parent 502375 eb5f05b67d5684795a3043d90ef99cc9f17c504a
child 502377 41919e4ed34d693e9bc16755957f27f95936c8a9
push id114172
push userdluca@mozilla.com
push dateTue, 19 Nov 2019 11:31:10 +0000
treeherdermozilla-inbound@b5c5ba07d3db [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersemilio
bugs1579929
milestone72.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 1579929: When a reflow is interrupted, don't purge flex item measurements until the next time they're needed in a later non-interrupted reflow. r=emilio Differential Revision: https://phabricator.services.mozilla.com/D53313
layout/generic/nsFlexContainerFrame.cpp
layout/generic/nsFrameStateBits.h
layout/generic/test/mochitest.ini
layout/generic/test/test_flex_interrupt.html
--- a/layout/generic/nsFlexContainerFrame.cpp
+++ b/layout/generic/nsFlexContainerFrame.cpp
@@ -1789,18 +1789,31 @@ void nsFlexContainerFrame::MarkCachedFle
     nsIFrame* aItemFrame) {
   aItemFrame->DeleteProperty(CachedFlexMeasuringReflow());
 }
 
 const CachedMeasuringReflowResult&
 nsFlexContainerFrame::MeasureAscentAndBSizeForFlexItem(
     FlexItem& aItem, nsPresContext* aPresContext,
     ReflowInput& aChildReflowInput) {
-  if (const auto* cachedResult =
-          aItem.Frame()->GetProperty(CachedFlexMeasuringReflow())) {
+  if (HasAnyStateBits(NS_STATE_FLEX_MEASUREMENTS_INTERRUPTED) &&
+      !aPresContext->HasPendingInterrupt()) {
+    // Our measurements are from an earlier reflow which was interrupted.
+    // (and the current reflow is not [yet] interrupted, so we have a chance
+    // to maybe get a more accurate measurement now).
+    // Purge our potentially-invalid item measurements.
+    for (nsIFrame* frame : mFrames) {
+      frame->DeleteProperty(CachedFlexMeasuringReflow());
+    }
+    RemoveStateBits(NS_STATE_FLEX_MEASUREMENTS_INTERRUPTED);
+    MOZ_LOG(gFlexContainerLog, LogLevel::Debug,
+            ("[perf] MeasureAscentAndBSizeForFlexItem purged all "
+             "cached values\n"));
+  } else if (const auto* cachedResult =
+                 aItem.Frame()->GetProperty(CachedFlexMeasuringReflow())) {
     if (cachedResult->IsValidFor(aChildReflowInput)) {
       return *cachedResult;
     }
     FLEX_LOG("[perf] MeasureAscentAndBSizeForFlexItem rejected cached value");
   } else {
     FLEX_LOG(
         "[perf] MeasureAscentAndBSizeForFlexItem didn't have a cached value");
   }
@@ -4288,26 +4301,27 @@ void FlexLine::PositionItemsInCrossAxis(
 
     // Back out to cross-axis edge of the line.
     lineCrossAxisPosnTracker.ResetPosition();
   }
 }
 
 void nsFlexContainerFrame::DidReflow(nsPresContext* aPresContext,
                                      const ReflowInput* aReflowInput) {
-  // Remove the cached values if we got an interrupt because the values will be
-  // the wrong ones for following reflows.
+  // If we got an interrupt, we make a note here that our cached measurements
+  // are potentially invalid, because our descendant block frames' reflows may
+  // have bailed out early due to the interrupt.  We'll keep these invalid
+  // measurements for the rest of this reflow (to avoid repeating the same
+  // bogus measurement), and purge them on the next (non-interrupted) reflow.
   //
   // TODO(emilio): Can we do this only for the kids that are interrupted? We
   // probably want to figure out what the right thing to do here is regarding
   // interrupts, see bug 1495532.
   if (aPresContext->HasPendingInterrupt()) {
-    for (nsIFrame* frame : mFrames) {
-      frame->DeleteProperty(CachedFlexMeasuringReflow());
-    }
+    AddStateBits(NS_STATE_FLEX_MEASUREMENTS_INTERRUPTED);
   }
   nsContainerFrame::DidReflow(aPresContext, aReflowInput);
 }
 
 void nsFlexContainerFrame::Reflow(nsPresContext* aPresContext,
                                   ReflowOutput& aDesiredSize,
                                   const ReflowInput& aReflowInput,
                                   nsReflowStatus& aStatus) {
--- a/layout/generic/nsFrameStateBits.h
+++ b/layout/generic/nsFrameStateBits.h
@@ -349,16 +349,24 @@ FRAME_STATE_BIT(FlexContainer, 22, NS_ST
 
 // True if the container has no flex items; may lie if there is a pending reflow
 FRAME_STATE_BIT(FlexContainer, 23, NS_STATE_FLEX_SYNTHESIZE_BASELINE)
 
 // True if any flex item in the container has a line with a
 // -webkit-line-ellipsis marker.
 FRAME_STATE_BIT(FlexContainer, 24, NS_STATE_FLEX_HAS_LINE_CLAMP_ELLIPSIS)
 
+// True if the flex container's items' cached measurements are potentially
+// invalid, due to them having been computed during a reflow that was
+// interrupted.  (We may still use those invalid measurements for the rest of
+// the interrupted reflow; but as soon as we need a cached measurement in a
+// non-interrupted reflow, this bit should make us purge our flex items'
+// measurements and remeasure.)
+FRAME_STATE_BIT(FlexContainer, 25, NS_STATE_FLEX_MEASUREMENTS_INTERRUPTED)
+
 // == Frame state bits that apply to grid container frames ====================
 
 FRAME_STATE_GROUP(GridContainer, nsGridContainerFrame)
 
 // True iff the normal flow children are already in CSS 'order' in the
 // order they occur in the child frame list.
 FRAME_STATE_BIT(GridContainer, 20,
                 NS_STATE_GRID_NORMAL_FLOW_CHILDREN_IN_CSS_ORDER)
--- a/layout/generic/test/mochitest.ini
+++ b/layout/generic/test/mochitest.ini
@@ -100,16 +100,17 @@ support-files = bug633762_iframe.html
 support-files = file_bug1307853.html
 [test_bug1408607.html]
 [test_bug1499961.html]
 [test_bug1566783.html]
 support-files = file_bug1566783.html
 [test_contained_plugin_transplant.html]
 skip-if = os=='win'
 [test_dynamic_reflow_root_disallowal.html]
+[test_flex_interrupt.html]
 [test_image_selection.html]
 [test_image_selection_2.html]
 [test_image_selection_in_contenteditable.html]
 [test_invalidate_during_plugin_paint.html]
 skip-if = headless # Headless:Bug 1405871
 [test_intrinsic_size_on_loading.html]
 [test_movement_by_characters.html]
 [test_movement_by_words.html]
new file mode 100644
--- /dev/null
+++ b/layout/generic/test/test_flex_interrupt.html
@@ -0,0 +1,107 @@
+<!doctype html>
+<title>Test for bug 1579929</title>
+<link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css">
+<script src="/tests/SimpleTest/SimpleTest.js"></script>
+<script src="/tests/SimpleTest/EventUtils.js"></script>
+<style>
+  .container {
+    display: flex;
+    flex-direction: column;
+    overflow-y: scroll;
+  }
+  #outer {
+    width: 500px;
+    height: 300px;
+  }
+</style>
+<div class="container" id="outer">
+  <div class="item" id="firstItem" style="width: 50px">
+    <!--popuplated by script-->
+  </div>
+  <div class="container">
+    <div class="container">
+      <div class="container">
+        <div class="container">
+          <div class="container">
+            <div class="item">Item</div>
+          </div>
+        </div>
+      </div>
+    </div>
+  </div>
+</div>
+<script>
+SimpleTest.waitForExplicitFinish();
+
+// Globals/constants:
+
+const gUtils = SpecialPowers.DOMWindowUtils;
+
+// This needs to match/exceed nsPresContext::sInterruptChecksToSkip in order
+// for us to actually be able to trigger the mHasPendingInterrupt=true codepath
+// in this test.  Each of these "dummy" blocks will trigger a call to
+// nsPresContext::CheckForInterrupt, and after 200, we'll actually trigger an
+// interrupt.
+const gInterruptCheckThreshold = 200;
+
+// Expected to match the inline style="width:..." for #firstItem (we slowly
+// increment it to trigger reflows):
+let gFirstItemWidthPX = 50;
+
+function main() {
+  const outer = document.getElementById("outer");
+  const firstItem = document.getElementById("firstItem");
+
+  // Insert a bunch of elements to be sure we actually get interrupted.
+  // (See description of gInterruptCheckThreshold above)
+  for (let i = 0; i < gInterruptCheckThreshold; i++) {
+    let dummyBlock = document.createElement("div");
+    dummyBlock.innerText = "dummy";
+    firstItem.appendChild(dummyBlock);
+  }
+
+  // Flush any pending relayout:
+  outer.offsetHeight;
+
+  // Take control of the refresh driver
+  gUtils.advanceTimeAndRefresh(0);
+
+  // Force reflow and store the "cost" (in num-frames-reflowed)
+  const costOfNormalReflow = forceReflow();
+
+  // Sanity-check: do that again and remeasure cost, to be sure it's stable:
+  const costOfNormalReflow2 = forceReflow();
+  is(costOfNormalReflow, costOfNormalReflow2,
+     "Our forceReflow function is expected to reliably trigger " +
+     "the same set of frames to be reflowed. If this fails, there's a " +
+     "bug in the test, or non-determinism in layout code.");
+
+  // Now, we force the next reflow to get interrupted, and then measure the
+  // cost under those conditions:
+  gUtils.forceReflowInterrupt();
+  const costOfInterruptedReflow = forceReflow();
+
+  // Hopefully the interrupted one was less expensive...
+  ok(costOfInterruptedReflow <= costOfNormalReflow,
+     "num frames reflowed in interrupted reflow (" +
+     costOfInterruptedReflow +
+     ") shouldn't exceed the num frames reflowed in normal reflow (" +
+     costOfNormalReflow + ")");
+
+  gUtils.restoreNormalRefresh();
+  SimpleTest.finish();
+}
+
+// This function dirties firstItem's width, forces a reflow, and
+// returns the number of frames that were reflowed as a result.
+function forceReflow() {
+  gFirstItemWidthPX++;
+  firstItem.style.width = gFirstItemWidthPX + "px";
+
+  const origFramesReflowed = gUtils.framesReflowed;
+  gUtils.advanceTimeAndRefresh(0);
+  return gUtils.framesReflowed - origFramesReflowed;
+}
+
+main();
+</script>