Experiment propagating NS_FRAME_IS_DIRTY once in PresShell::FrameNeedsReflow instead of in ReflowInput::Init
Categories
(Core :: Layout, task, P3)
Tracking
()
People
(Reporter: mozbugz, Assigned: dbaron)
References
(Regressed 1 open bug)
Details
(Whiteboard: [layout-sydney-2018])
Attachments
(3 files)
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)
Assignee | ||
Comment 1•6 years ago
|
||
When this lands, bug 1459937 should be backed out.
Reporter | ||
Comment 2•6 years ago
|
||
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
Assignee | ||
Comment 3•6 years ago
|
||
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?
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Comment 4•6 years ago
|
||
(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
Reporter | ||
Comment 5•6 years ago
|
||
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•6 years ago
|
||
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.
Assignee | ||
Comment 7•5 years ago
|
||
Are you planning to look into this further, or should we get somebody else assigned here?
Reporter | ||
Comment 8•5 years ago
|
||
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 intoMarkSubtreeDirty()
. AlsoMarkFrameChildrenDirty
is new, I don't know whether it's different from myMarkSubtreeDirty
, to be investigated!
Please ask me if you have further questions.
Assignee | ||
Comment 9•5 years ago
|
||
Try run of patch merged to tip:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=249695c05ba5dc6fb0817adb02427feae240b09c&group_state=expanded
Assignee | ||
Comment 10•5 years ago
|
||
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.
Assignee | ||
Comment 11•5 years ago
|
||
Assignee | ||
Comment 12•5 years ago
|
||
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.
Assignee | ||
Comment 13•5 years ago
|
||
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 <gsquelart@mozilla.com>
Co-authored-by: L. David Baron <dbaron@dbaron.org>
Depends on D36423
Assignee | ||
Comment 14•5 years ago
|
||
(The single line that made them active was commented out in the previous patch.)
Depends on D36424
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Comment 15•5 years ago
|
||
Pushed by dbaron@mozilla.com: 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•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/61732637cead
https://hg.mozilla.org/mozilla-central/rev/ae0a517daf97
https://hg.mozilla.org/mozilla-central/rev/c70683bb3522
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Comment 17•5 years ago
|
||
== 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
Assignee | ||
Comment 18•5 years ago
|
||
Yeah, this graph is reasonably convincing that the improvement came from this bug. I'm a little bit surprised, but not all that surprised.
Assignee | ||
Comment 19•5 years ago
|
||
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
.
Assignee | ||
Comment 20•5 years ago
|
||
... also myspace.com
. Possibly some smaller changes on a few other sites.
Description
•