1492538 - Don't reflow flex items if they're not dirty and their size from the flex algorithm hasn't changed

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P1)

Description

[Daniel Holbert [:dholbert]]

Right now, we don't honor the nsIFrame "dirty" flag on flex items -- we're pretty aggressive about reflowing flex items, when the flex container itself receives a reflow (due to having some dirty descendant somewhere).

We could probably honor the dirty flag (skipping reflows on non-dirty flex items) in cases where we know the flex algorithm isn't giving these items a different size.

This is part of bug 1377253 -- copying qf status from that bug.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: wip fix v1

Comment 2

[Daniel Holbert [:dholbert]]

I'm going to bump this from qf:p1 down to to :p3, because:

• qf:p1 is now reserved for pageload performance, whereas this optimization would typically help with incremental reflows.

• This is a speculative fix -- I haven't yet run across a concrete example where this optimization helps much. (Initially I was thinking this might help in e.g. bug 1376536 but it didn't end up helping with the actual problem there.)

Comment 3

[Daniel Holbert [:dholbert]]

Picking this up again.

Concrete examples where this may help:

• bug 1519497 (an issue on LinkedIn), per bug 1519497 comment 6
• cases surfaced from bug 1449346, particularly for things like the tab-strip when there are many tabs (many flex item) and a single tab (a single flex item) is added/removed/modified

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: diagnostic patch: Pref on reflow-count-painting, and enable it for flex & block containers

This is a patch that I'll be using locally to validate the usefulness of the changes here.

It:
(1) flips the pref layout.reflow.showframecounts which makes a little number show up alongside frames of some frame-classes ("leaf frames", mostly), to indicate how many times the frame has been reflowed, as of the most recent time it was repainted.

(2) Turns on this reflow count painting for nsFlexContainerFrame and nsBlockContainerFrame, to help surface whether & how many times flex containers & block-flavored flex items are reflowed in response to changes in testcases here.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: wip fix, partially rebased (ignore)

Here's the WIP patch, rebased.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: wip fix v2 (rebased)

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (view w/ diagnostic patch applied): animated dynamic change inside of a flex item

Here's a testcase I'm using locally.

• The third flex item ("hi") has a repeating change to its intrinsic size, each of which results in its intrinsic size changing and it needing a reflow.
• However, the other flex items ("AAA"/"BBB") don't change size and shouldn't need a reflow.
• (The color animation is simply there in order to force a repaint so that the displayed reflow counts will update.)

In a build with just the diagnostic patch, the reflow counts continuously go up on all three flex items here (twice as fast on the third item, since it needs a remeasurement each time its intrinsic size changes).

In a build with the wip fix as well, the reflow counts don't go up on the first two flex items, which is great & what we'd expect.

Comment 8

[Daniel Holbert [:dholbert]]

Mats points out that we'll want to be sure bug 1480850 doesn't break this optimization (i.e. we need to consider the possibility that align-content:baseline may lead to indirect influences from one flex item's sizing/positioning to the padding-box-size of another flex item).

At least, that's something we'll need to consider when bug 1480850 is fixed.

Comment 9

[Sean Voisen (:svoisen)]

Now that we triage by severity, setting priority to P1 to reflect backlog prioritization on this bug as either in-progress, or planned development in the near term. See https://wiki.mozilla.org/Platform/Layout#Backlog_Tracking_in_Bugzilla

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: Bug 1492538 part 1: Remove unnecessary special-casing on constrained-BSize check in FlexItem::NeedsFinalReflow. r?TYLin

Note: this patch doesn't change behavior of current mozilla-central - it's just reordering some logic, basically.

Background/explanation: in current mozilla-central, we intentionally force a
"final reflow" for flex items (i.e. we return true from NeedsFinalReflow) if we
see that there's a constrained AvailableBSize (i.e. if we're fragmenting). However, the
logic to do that is buried within a basically-unrelated "if
(HadMeasuringReflow())" special case.

This patch pulls that check out of this unrelated special-case, so that we
detect the constrained AvailableBSize earlier and return earlier (indicating
more eagerly/up-front that we need a final reflow).

This patch is necessary because a later patch in this queue will add additional
special cases to FlexItem::NeedsFinalReflow, which will aim to make us skip
the final reflow in more cases. But for now, we want this
contrained-AvailableBSize check to "dominate" and eagerly force a reflow,
regardless of that soon-to-be-added logic.

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: Bug 1492538 part 2: Cache flex items' content-box sizes after reflowing them, & skip subsequent reflow if the size matches and the item's subtree isn't dirty. r?TYLin

Depends on D78821

Comment 12

[Daniel Holbert [:dholbert]]

"part 2" is still in progress, and I still need to add some tests:

• perf tests to verify that reflow counts are better (& don't regress) in various situations
• "break tests" which should pass both before & after the patch, to verify that e.g. a tweak to a flex item's border/padding does still successfully get that flex item reflowed/repainted correctly (even though it may not impact the content-box size which is what I'm conditioning this optimization on).

part 1 is pretty trivial & should be OK to review already, though.

Comment 13

[Daniel Holbert [:dholbert]]

Try run with latest patch (linux only, but hopefully representative enough):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a3bdb418f6a7d5fdf35a445772f1409962850d0

Pretty sure all the orange there is known/unrelated intermittent.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/00674992b58a
part 1: Remove unnecessary special-casing on constrained-BSize check in FlexItem::NeedsFinalReflow. r=TYLin
https://hg.mozilla.org/integration/autoland/rev/1aa53285261e
part 2: Cache flex items' content-box sizes after reflowing them, & skip subsequent reflow if the size matches and the item's subtree isn't dirty. r=TYLin

Comment 15

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/00674992b58a
https://hg.mozilla.org/mozilla-central/rev/1aa53285261e

Comment 16

[Florin Strugariu [:Bebe]]

== Change summary for alert #26468 (as of Thu, 09 Jul 2020 04:47:14 GMT) ==

Improvements:

29% allrecipes loadtime android-hw-g5-7-0-arm7-api-16-shippable opt cold 7,925.75 -> 5,645.42
26% allrecipes LastVisualChange android-hw-g5-7-0-arm7-api-16-shippable opt cold 7,923.50 -> 5,861.42
21% allrecipes SpeedIndex android-hw-g5-7-0-arm7-api-16-shippable opt cold 4,462.75 -> 3,520.17
20% allrecipes PerceptualSpeedIndex android-hw-g5-7-0-arm7-api-16-shippable opt cold 4,128.08 -> 3,303.67
15% allrecipes ContentfulSpeedIndex android-hw-g5-7-0-arm7-api-16-shippable opt cold 3,875.33 -> 3,290.33
9% allrecipes android-hw-g5-7-0-arm7-api-16-shippable opt cold 2,115.19 -> 1,918.94

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=26468

Comment 17

[Daniel Holbert [:dholbert]]

(In reply to Florin Strugariu [:Bebe] (needinfo me) from comment #16)

Improvements:

That's great news! This is expected to improve performance for some cases.

I didn't know we had an allrecipes testsuite, but I'm glad to know everyone can get their recipes up faster now :)

Comment 18

[Andrew Creskey [:acreskey]]

Daniel, there wouldn't happen to a pref to flip this reflow off/on?
Would like to measure the impact in some other scenarios.

Comment 19

[Daniel Holbert [:dholbert]]

No, there's not a pref to toggle this optimization on/off.

I can add one (on Nightly) next week if that'd be helpful, though.

Comment 20

[Andrew Creskey [:acreskey]]

Thanks Dan,
I'm looking at this through Fenix now and I've isolated when the geckoview was updated so I can test those adjacent days.
It's not perfect, but it will give me some data.
Maybe wait and see what this shows?