Closed Bug 1505254 Opened 2 years ago Closed 2 years ago
Needs Reflow() shouldn't invalidate ancestors' ISizes up further than an abspos ancestor
47 bytes, text/x-phabricator-request
|Details | Review|
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.)
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/ade95bb476e5 When invalidating ancestors' intrinsic sizes, stop when we hit an abspos frame. r=mats
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.