Closed Bug 1505254 Opened 6 years ago Closed 6 years ago

FrameNeedsReflow() shouldn't invalidate ancestors' ISizes up further than an abspos ancestor


(Core :: Layout: Positioned, enhancement, P1)




Performance Impact low
Tracking Status
firefox65 --- fixed


(Reporter: dholbert, Assigned: dholbert)


(Blocks 1 open bug)


(Keywords: perf, perf:responsiveness)


(1 file)

PresShell::FrameNeedsReflow has a loop where it marks the intrinsic widths as dirty on all of the frame's ancestors.

If this loop encounters an abspos ancestor, I'm pretty sure it can stop there.  The ancestors of that abspos frame shouldn't have any intrinsic sizes that could possibly depend on the abspos frame (& its contents).

If we cut off this invalidation at abspos ancestors, that'd help with bug 1376536.  In particular, it prevents typing-related "FrameNeedsReflow" calls from invalidating a cached bsize-measurement on a distant flex-item ancestor, which happens to be separated from the changing text by an intermediate abspos frame.

I gave this an experimental Try run and it seems not to break anything:
BTW, we'll get some of this "for free" from bug 1159042, I think -- because this "walk-the-ancestor-chain" loop already checks for whether the ancestor is a reflow root, and bug 1159042 will make *some* abspos frames may become dynamic reflow roots after that bug lands.

Nonetheless, I don't think bug 1159042 will make *all* abspos frames into reflow roots -- only the ones that are fixed-size, IIRC.  And for the purposes of this bug here, I'm pretty sure we can include *all* abspos frames in the optimization.
Yeah, I think this makes sense.  The one risk is that we're depending on ClearAncestorIntrinsics for something that's *not* intrinsic sizes.  I can't think of anything, though.

There are also a bunch of cases where this would be useful but we can't do the reflow-root-ish thing.
Try run with the patch from phabricator (including a new mochitest, unlike the try push from comment 0):
Blocks: 1376536
As noted in bug 1505254 comment 25, this patch is sufficient to fix WhatsApp -- it reduces reflow-time from ~74ms to ~0.5ms on each keypress, in a particular WhatsApp conversation that I'm testing with with lots of backscroll.

(Also: I'm cloning [qf:p3] status from bug 1376536, and I'm adding the QF tag to indicate that this'll hopefully land in Firefox 65.)
Whiteboard: [qf:p3:f65:responsiveness]
Pushed by
When invalidating ancestors' intrinsic sizes, stop when we hit an abspos frame. r=mats
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Thanks Daniel, it looks like it has a significant impact on the debugger as many of its actions are faster with this patch.

== Change summary for alert #17408 (as of Wed, 07 Nov 2018 18:07:19 GMT) ==


 16%  damp custom.jsdebugger.pause.DAMP windows10-64 pgo e10s stylo                411.97 -> 344.15
 14%  damp simple.jsdebugger.reload.DAMP osx-10-10 opt e10s stylo                  503.63 -> 433.69
 14%  damp simple.jsdebugger.reload.DAMP windows10-64 pgo e10s stylo               279.23 -> 240.56
 13%  damp custom.jsdebugger.stepIn.DAMP osx-10-10 opt e10s stylo                  1,581.66 -> 1,378.22
 12%  damp custom.jsdebugger.reload.DAMP windows10-64 pgo e10s stylo               979.70 -> 861.91
 11%  damp windows10-64 pgo e10s stylo                 1,004.06 -> 893.68
 10%  damp osx-10-10 opt e10s stylo                    2,015.96 -> 1,806.09
  9%  damp custom.jsdebugger.reload.DAMP osx-10-10 opt e10s stylo                  2,212.34 -> 2,021.80
  6%  damp windows10-64 pgo e10s stylo                 720.25 -> 673.69
  6%  damp windows10-64 pgo e10s stylo            1,333.97 -> 1,251.70
  6%  damp osx-10-10 opt e10s stylo               2,882.63 -> 2,717.66
  4%  damp custom.inspector.manyrules.deselectnode windows10-64 pgo e10s stylo     185.05 -> 177.93
  3%  damp complicated.jsdebugger.reload.DAMP windows10-64 pgo e10s stylo          1,808.45 -> 1,745.61

For up to date results, see:
Nice! Thanks for letting me know.
Performance Impact: --- → P3
Whiteboard: [qf:p3:f65:responsiveness]
You need to log in before you can comment on or make changes to this bug.