1474771 - Experiment propagating NS_FRAME_IS_DIRTY once in PresShell::FrameNeedsReflow instead of in ReflowInput::Init

Status: RESOLVED FIXED

(Core :: Layout, task, P3)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Following bug 1308876, FrameNeedsReflow marks one frame dirty, and then during reflow, when a dirty frame is reflowed, it marks its current children dirty (in ReflowInput::Init).

This is good for cache locality, as we don't go through the whole sub-tree when marking its root frame dirty.

However, this complicates reflow, as a dirty frame needs to mark its children dirty, but also needs to mark "nephews" which are pulled from later (not yet reflowed) frames, as these pulled frames don't have their dirty bit set yet.

Bug 1459937 is trying to fix one such case, but it is quite complex as every place that pulls frames into its children list must correctly identify if it's pulling forwards or backwards, to know if the pulled frame should be marked dirty or not!
Hopefully it's most efficient, but it adds "clever" code and potential bugs; and we would need to audit all such frame-pulling operations (now and in the future) to correctly handle every possible cases.

So I would like to experiment with marking *all* appropriate frames dirty at once from FrameNeedsReflow, so we won't need to propagate that bit during reflow anymore.

The downside would be a potential performance hit, as we would potentially visit lots of frames to set this bit. However I think that in such cases, we may already be visiting the sub-tree anyway, e.g., to MarkIntrinsicISizesDirty().

But if the performance is not affected, this change would be much simpler than what we currently need to do (e.g., in bug 1459937), and would automatically cover all reparenting cases -- Unless there are still cases where we could be pulling frames from a non-dirty sub-tree? (TBD)

Comment 1

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

When this lands, bug 1459937 should be backed out.

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Update:

My initial patch added the preemptive NS_FRAME_IS_DIRTY marking in FrameNeedsReflow, and changed the marking in ReflowInput::Init into an assertion that all child frames should already be marked dirty... And unfortunately that failed very quickly (which is why I continued work on bug 1459937, which would be useful if this one here doesn't work or takes too much time).

After some debugging and extra logging, I noticed that these frames that were expected to be dirty were in fact dirty in a previous run of ReflowInput::Init (within the same reflow run)! But as they were reflowed once, and when involved in further reflow, they had lost their dirty bit.

So in fact this assertion failure may be good news: The proposed experiment is possibly performing fewer redundant frame reflows.

I launched a try with the FrameNeedsReflow patch, but without the assertion, and though it shows some oranges, it's not too much of a disaster. Obviously it won't be trivial, but if I/someone can successfully make this work well, it may be worth the effort.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0586c5d5f388f0145de689c7f17326016cfd8b2b

Comment 3

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

Hmmm.  That reminds me:  there are some callers that don't go through FrameNeedsReflow -- those that set NS_FRAME_IS_DIRTY in the middle of reflow, e.g., in the column code.  I'm not sure they need to do this, but if they do, they probably need to be patched to set NS_FRAME_IS_DIRTY on the subtree -- although maybe it's better to fix them not to set NS_FRAME_IS_DIRTY at all (maybe NS_FRAME_HAS_DIRTY_CHILDREN is enough?).  Could those callers be the source of the assertions?

Comment 4

[Jan Henning [:JanH]]

(In reply to David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7 from comment #3)
> Hmmm.  That reminds me:  there are some callers that don't go through
> FrameNeedsReflow -- those that set NS_FRAME_IS_DIRTY in the middle of
> reflow, e.g., in the column code.

Or the font inflation code [1] as well, I think?

[1] https://dxr.mozilla.org/mozilla-central/rev/c2e3be6a1dd352b969a45f0b85e87674e24ad284/layout/generic/ReflowInput.cpp#607-678

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Thank you Jan.
I searched for all occurrences of `AddStateBits(NS_FRAME_IS_DIRTY)` and replaced them with the new `MarkSubtreeDirty()`; The font inflation code was part of this change.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9782aefca9fa0636bc8621d3285a4f31c9cbedaf

Comment 6

[Jan Henning [:JanH]]

Nice. While this wasn't the actual cause of bug 1428670, I did notice during my investigation that the font inflation code there was still relying on the pre-bug 1308876 behaviour, were marking a parent as dirty was sufficient to subsequently cause all children to mark themselves as dirty, too. So I was planning to fix that particular instance as part of working on bug 1428670, but you've saved me the trouble, thank you.

Comment 7

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

Are you planning to look into this further, or should we get somebody else assigned here?

Comment 8

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Sorry, I shouldn't have kept it, especially since I've now been away from layout for a couple of months. Thank you for reminding me.
NI: back to you, to pick a new assignee.

The last patch I had working is:
https://hg.mozilla.org/try/rev/e9866f8d1d76

I've just done a naive* rebase there:
https://hg.mozilla.org/try/rev/08e027f82b75
(try in progress: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb24cf596ebfab62a8016f2edd3b64dc3f061fe8 , looks like an a11y test is broken.)

• No real merge issue, but it's possible it's now broken and/or incomplete, not covering all locations where AddStateBits(NS_FRAME_IS_DIRTY) should be turned into MarkSubtreeDirty(). Also MarkFrameChildrenDirty is new, I don't know whether it's different from my MarkSubtreeDirty, to be investigated!

Please ask me if you have further questions.

Comment 9

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

Try run of patch merged to tip:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=249695c05ba5dc6fb0817adb02427feae240b09c&group_state=expanded

Comment 10

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

Here's a new try push with (a) additional code changes from a code audit a few months ago and (b) a test fix for the test failure seen in the previous try push.

Comment 11

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

... and another try push with some additional changes.

Comment 12

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

Attachment: Bug 1474771 - Adjust accessibility test_list.html to deal with frames having NS_FRAME_IS_DIRTY more often. r?Jamie

NS_FRAME_IS_DIRTY being set on a frame has always meant that that
frame and all of its descendants need to be reflowed. What the main
patch on this bug is changing is when it gets propagated to descendants;
prior to that patch it gets propagated during reflow, whereas after the
patch it gets propagated when the bit is set. This means that
NS_FRAME_IS_DIRTY is now (after the patch) a somewhat more reliable
indicator of whether a frame requires reflow than it was before.

However, this has a strange interaction with the particular sequence of
operations that test_list.html performs. In particular, test_list.html:
(1) makes a style change that requires reflow,
(2) flushes style (but not layout), and
(3) gets the name of an accessible whose layout was dirtied by the style change.

Getting this name ends up here:

nsTextFrame::GetRenderedText at layout/generic/nsTextFrame.cpp:9600
nsTextEquivUtils::AppendTextEquivFromTextContent at accessible/base/nsTextEquivUtils.cpp:127
nsTextEquivUtils::AppendFromAccessible at accessible/base/nsTextEquivUtils.cpp:174
nsTextEquivUtils::AppendFromAccessibleChildren at accessible/base/nsTextEquivUtils.cpp:162
nsTextEquivUtils::GetNameFromSubtree at accessible/base/nsTextEquivUtils.cpp:39
mozilla::a11y::Accessible::NativeName at accessible/generic/Accessible.cpp:1991
mozilla::a11y::HyperTextAccessible::NativeName at accessible/generic/HyperTextAccessible.cpp:1760
mozilla::a11y::Accessible::Name at accessible/generic/Accessible.cpp:149
mozilla::a11y::xpcAccessible::GetName at accessible/xpcom/xpcAccessible.cpp:245
NS_InvokeByIndex at xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_x86_64_unix.S:106

What the main patch in the bug has changed is whether this nsTextFrame
has the NS_FRAME_IS_DIRTY bit set at this point. This means that
GetRenderedText will bail out without reporting the text, because the
frame is marked dirty.

On the assumption that this action -- getting accessible names while
frames are dirty -- is an artificial situation set up in the test and
not a real-world situation that we need to care about, I'm proposing
that we make the straightforward adjustment to the test: flush layout
in addition to flushing style at step (2) above, so that the test works
again. As long as that assumption is correct, I think this should be
fine. (I'd note that this is the only test that breaks as a result of
the patch.)

If, on the other hand, we actually do care about what accessible names
return while layout state is dirty and reflow is needed, we should
probably improve nsTextFrame::GetRenderedText so that it can somehow
report useful state even when NS_FRAME_IS_DIRTY is set, or something
else more complicated.

Comment 13

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

Attachment: Bug 1474771 - Propagate NS_FRAME_IS_DIRTY to descendants when marking as dirty rather than during reflow. r?dholbert

This simplifies dealing with frames that are pushed/pulled between
continuations during reflow, allows us to avoid the complexity of the
fix to 1459937, and hopefully fixes some of the regressions from bug
1308876.

This disables the changes from bug 1459937 by commenting out a single
line in ReparentFrameInternal in nsBlockFrame.cpp, but all the added
code will be removed in the following patch.

Co-authored-by: Gerald Squelart <[e-mail]>
Co-authored-by: L. David Baron <[e-mail]>

Depends on D36423

Comment 14

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

Attachment: Bug 1474771 - Revert all changes from bug 1459937 since they are no longer needed. r?dholbert

(The single line that made them active was commented out in the previous patch.)

Depends on D36424

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/61732637cead
Adjust accessibility test_list.html to deal with frames having NS_FRAME_IS_DIRTY more often.  r=Jamie
https://hg.mozilla.org/integration/autoland/rev/ae0a517daf97
Propagate NS_FRAME_IS_DIRTY to descendants when marking as dirty rather than during reflow.  r=dholbert
https://hg.mozilla.org/integration/autoland/rev/c70683bb3522
Revert all changes from bug 1459937 since they are no longer needed.  r=dholbert

Comment 16

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/61732637cead
https://hg.mozilla.org/mozilla-central/rev/ae0a517daf97
https://hg.mozilla.org/mozilla-central/rev/c70683bb3522

Comment 17

[Alexandru Ionescu (needinfo me) [:alexandrui]]

== Change summary for alert #21762 (as of Sat, 06 Jul 2019 20:00:14 GMT) ==

Improvements:

2% tp5o windows10-64-shippable opt e10s stylo 120.45 -> 117.94

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

Comment 18

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

Yeah, this graph is reasonably convincing that the improvement came from this bug. I'm a little bit surprised, but not all that surprised.

Comment 19

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

It seems like the subtest data is only available on mozilla-central and not autoland, but it seems like there were substantial improvements on a subset of the sites including 56.com and dailymail.co.uk.

Comment 20

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

... also myspace.com. Possibly some smaller changes on a few other sites.