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

RESOLVED FIXED in Firefox 65

Status

()

enhancement
P1
normal
RESOLVED FIXED
6 months ago
2 months ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks 1 bug, {perf})

Trunk
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

(Whiteboard: [qf:p3:f65:responsiveness] )

Attachments

(1 attachment)

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:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d7ed5eefc03d6bff53c50910f660d358a9af371
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):
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d23f7bcf493588f0072c81a6d178b18c46b8ebf
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]

Comment 6

6 months ago
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ade95bb476e5
When invalidating ancestors' intrinsic sizes, stop when we hit an abspos frame. r=mats

Comment 7

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ade95bb476e5
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months 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) ==

Improvements:

 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 custom.jsdebugger.open.DAMP windows10-64 pgo e10s stylo                 1,004.06 -> 893.68
 10%  damp custom.jsdebugger.open.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 simple.jsdebugger.open.DAMP windows10-64 pgo e10s stylo                 720.25 -> 673.69
  6%  damp complicated.jsdebugger.open.DAMP windows10-64 pgo e10s stylo            1,333.97 -> 1,251.70
  6%  damp complicated.jsdebugger.open.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: https://treeherder.mozilla.org/perf.html#/alerts?id=17408
Nice! Thanks for letting me know.
You need to log in before you can comment on or make changes to this bug.