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

Status: RESOLVED FIXED

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

Description

[Daniel Holbert [:dholbert]]

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

Comment 1

[Daniel Holbert [:dholbert]]

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.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: Bug 1505254: When invalidating ancestors' intrinsic sizes, stop when we hit an abspos frame. r?mats

Comment 4

[Daniel Holbert [:dholbert]]

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

Comment 5

[Daniel Holbert [:dholbert]]

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

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ade95bb476e5
When invalidating ancestors' intrinsic sizes, stop when we hit an abspos frame. r=mats

Comment 7

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/ade95bb476e5

Comment 8

[Alexandre Poirot [:ochameau]]

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

Comment 9

[Daniel Holbert [:dholbert]]

Nice! Thanks for letting me know.