Try and reduce number of reflows triggered by setting the visual viewport size
Categories
(Core :: Panning and Zooming, task)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox80 | --- | fixed |
People
(Reporter: kats, Assigned: kats)
References
Details
Attachments
(1 file)
Spinoff from https://phabricator.services.mozilla.com/D80089#2445572
The quick summary is that when enabling the visual viewport size on desktop, a number of crashtests had their assertions counts go up. This was a result of doing more reflow work. In particular, the MobileViewportManager would set the visual viewport size (for the first time) after getting the load event, which would trigger a new reflow, even though the document had already been laid out just fine before the load event.
If we can set the visual viewport size after laying out the root scrollframe for the first time, that should avoid this problem. If we just always do it after laying out the root scrollframe, we can probably dump the call I added to the ViewportFrame fixed-pos code in https://phabricator.services.mozilla.com/D80040 as well, since it will be redundant.
Try push with this attempt: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=4b4e8bfc06799d19d5c88ba11e66afd5a8c9dad7
|
||
Updated•5 years ago
|
|
Assignee | |
Comment 1•5 years ago
|
||
Even with the obvious build error fixed, the above try push didn't have the desired effect, because the call to update the VV size from nsGfxScrollFrame was happening too early, before MVM::mDisplaySize was populated. This meant that the VV size still didn't get populated until the load event and so still had the extra reflow.
This try push should be better, as it detects the "early reflow" and does a smaller set of changes. I think it should also remove the extra reflow on Android while not breaking anything.
|
|
Assignee | |
Comment 2•5 years ago
|
||
I investigated the layout/reftests/meta-viewport/invalid-content.html failure in the above try push yesterday. The problem is actually the reference page not properly doing a "shrink to fit", because the reference page now does only a single reflow before the load event, and at that point the MVM's mDisplaySize is zero at the point that it gets queried for "minimum scale size" purposes. This ends up bypassing the shrink-to-fit entirely. So it seems like we should populate the MVM's display size unconditionally at the start of a reflow. Or stop pulling the display size from the MVM and just use the content viewer size directly in UpdateMinimumScaleSize. The former seems a bit cleaner.
|
|
Assignee | |
Comment 3•5 years ago
|
||
This shrink-to-fit stuff is a bit annoying because it triggers spurious reflows too. With my modifications so far, for the reference page we do:
- Initial reflow which uses display size of 800 x 1000. The scrollframe content width is 980 so it expands the height to 1225 so it can shrink to fit. After laying out the scrollbars we determine the VV size to be 800 x 1000. This happens during the reflow so no additional things are marked dirty.
- In the post-reflow phase we do the shrink-to-fit, which sets the resolution to 0.816327 which sets the visual viewport to 980 x 1225 which marks things for reflow again. This seems kinda reasonable because we theoretically need to reposition fixed-pos items and scrollbars after a shrink-to-fit operation.
- Second reflow happens. No relevant changes here
- Load event fires on the MVM, which sets the VV size back to 800 x 1000 as part of UpdateResolution and then does the shrink-to-fit again, to set it back to 980 x 1225. This flip-flop is all part of the load event handler in the MVM, but it has the effect of marking things dirty again and so triggers a third reflow
- Third reflow happens, no relevant changes.
So that third reflow seems avoidable if we can turn off the flip-flopping.
|
|
Assignee | |
Comment 4•5 years ago
|
||
After the changes in bug 1648193, it's much easier to understand how to turn off the flip-flopping, and I did so in a local patch. Try push at https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=d76081b28dc6b97f90f1d7b153617ed4011e6dcb shows that test passing. Next I looked at the wpt9 failure (css/css-scroll-anchoring/position-change-heuristic.html).
The problem there was in this case because changing the element from position:fixed to position:absolute introduces a horizontal scrollbar. That horizontal scrollbar triggers a visual viewport size change, which then resets the scroll anchor.
I don't know why the scroll anchor should need to be reset if the visual viewport size changes, and it certainly seems undesirable in the case where it's just a matter of scrollbars appearing or disappearing. Removing that line of code makes the test pass, as well as a couple of other tests in that directory currently marked failing. So that's nice.
|
|
Assignee | |
Comment 5•5 years ago
|
||
With dependencies fixed the last remaining problem seems to be that some reftest on Android are showing up with scrollbars in the test image vs no scrollbars in the reference image. Since reftests run with apz.allow_zooming=false on Android the pages don't undergo a shrink-to-fit, so if they are larger than the visual viewport (800x1000) then they should render with scrollbars.
The very first reflow that happens doesn't have the VV size set, and so when we do the TryLayout code we use the scrollport size instead of the visual viewport size. This results in no scrollbars being added. Subsequent reflows do correctly have the scrollbars. The problem is that the reference pages just do one reflow and then stop, whereas the test pages in question have some sort of MozReftestInvalidate action happening and so do multiple reflows.
|
|
Assignee | |
Comment 6•5 years ago
|
||
I'm not sure why that block is conditional on the visual viewport size being set at all. We want to use the equivalent of the visual viewport size even if it's not set, I think. And it's not like we're reading the VV size directly from the presShell, we're computing it from the composition size and resolution, which should work regardless of whether the VV size is set on the presShell. I tried this change and according to my logging it has the desired effect of adding scrollbars to the reference image, but the screenshot still doesn't show them. Not sure what's going on there.
|
|
Assignee | |
Comment 7•5 years ago
|
||
The scrollbars were getting positioned incorrectly, because reading the VV size here wasn't working, because the very first reflow (and in this case, the only reflow) didn't have the VV size set yet. Updating that to use the composition size from nsLayoutUtils seems to do the job.
|
|
Assignee | |
Comment 8•5 years ago
|
||
|
|
Assignee | |
Comment 9•5 years ago
|
||
I fixed the android reftest failures via bug 1648500, but there's additional scroll-anchoring WPT failures on android that i either didn't see before or got introduced somewhere along the way. Trying to investigate those now.
|
|
Assignee | |
Comment 10•5 years ago
|
||
The start-edge-in-block-layout-direction.html WPT failure is because on Android, we do a shrink-to-fit on the page on very first reflow now, which is before the test runs. So there's zero scroll range for the test to do any scrolling and obviously that doesn't work. However there's a bit of a caveat here, because just setting an initial-scale=1 meta tag doesn't fix the problem, even though it prevents the shrink-to-fit. It has to be a user-scalable=no meta tag, to prevent the scrollframe from setting the minimum scale size. If it does set the minimum scale size, that gets used as the scrollport size, which again means there's a zero scroll range. I'm not sure if this behaviour is correct - it was certainly surprising. I guess it sort of makes sense though, because just setting initial-scale=1 still means you can zoom out manually, and that means the layout viewport and layout scrollport needs to correspond to whatever the minimum scale is.
Anyway, the fix for this test seems to be to add a user-scalable=no meta tag. If this test had waited for the load event before doing its testing work, it would probably be failing even with the current code.
|
|
Assignee | |
Comment 11•5 years ago
|
||
Hopefully that's it. The user-scalable=no thing fixed all the failing WPT tests I had on the previous try push. Just need to document the last patch a bit better and then it'll be up for review.
|
|
Assignee | |
Comment 12•5 years ago
|
||
There's two code changes in this patch:
-
The update to the visual viewport that was happening just before positioning
the fixed items gets moved to happen after determining the scrollbars for
the root scrollframe. This moves it a little bit earlier, to basically the
earliest point at which the visual viewport can actually be computed, since
it depends on the presence of the root scrollframe's scrollbars.More importantly, this change sets the visual viewport without checking to
see if one was already set, as the old code did. This means every reflow
of the root scrollframe on a presShell with an MVM will now have a visual
viewport set. Previously the visual viewport would only get set for the first
time when the MVM got a load or first-paint event, and then would get updated
for subsequent reflows. The net effect here is that the visual viewport is
set earlier, and this can sometimes eliminate extra reflows from after the
load event, because everything is already in a consistent state. -
The NotifyResizeReflow call to MVM is replaced by a NotifyReflow call that
runs before every reflow, instead of just on resizes. Note that the
NotifyReflow also doesn't update the visual viewport like NotifyResizeReflow
used to do, because that is taken care of by the above-mentioned code change
to set the visual viewport.This is desirable because there are things that run during reflow that attempt
to read the display size from the MVM, and they were getting a zero size
for reflows that happened before the first resize or load/first-paint events.
Now they get a valid display size on every reflow, and so again this allows
fewer overall reflows as the code converges to a stable state faster.
Together these changes ensure that every reflow has access to up-to-date
properties (display size, mobile viewport size, visual viewport size) from the
MVM. This eliminates unnecessary reflows because of out-of-order computations
based on stale values and such. Therefore the number of reflows goes down,
which is reflected by the changes to the crashtest assertion counts.
|
||
Updated•5 years ago
|
|
||
Comment 13•5 years ago
|
||
|
||
Comment 14•5 years ago
|
||
| bugherder | ||
|
||
Comment 15•5 years ago
|
||
== Change summary for alert #26422 (as of Fri, 03 Jul 2020 03:30:11 GMT) ==
Improvements:
96% perf_reftest recompute-position-vertical-rl.html linux64-shippable opt e10s stylo 10.96 -> 0.41
96% perf_reftest recompute-position-vertical-rl.html windows7-32-shippable opt e10s stylo 11.98 -> 0.49
95% perf_reftest recompute-position-vertical-rl.html macosx1014-64-shippable opt e10s stylo 12.38 -> 0.60
95% perf_reftest recompute-position-vertical-rl.html windows10-64-shippable-qr opt e10s stylo 11.07 -> 0.61
94% perf_reftest recompute-position-vertical-rl.html windows10-64-shippable opt e10s stylo 10.63 -> 0.67
92% perf_reftest recompute-position-vertical-rl.html linux64-shippable-qr opt e10s stylo 11.72 -> 0.92
20% perf_reftest windows7-32-shippable opt e10s stylo 1.62 -> 1.29
16% perf_reftest windows10-64-shippable opt e10s stylo 1.75 -> 1.47
15% perf_reftest windows10-64-shippable-qr opt e10s stylo 1.79 -> 1.52
10% perf_reftest windows10-64-shippable-qr opt e10s stylo 1.65 -> 1.49
For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=26422
Description
•