269905 - Need to cache frame emptiness

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We've seen in some bugs that nsIFrame::IsEmpty() can become a performance
problem, because it sometimes requires traversing the entire frame tree under
the frame. To mitigate those problems and to avoid problems in bug 209694, which
calls IsEmpty() significantly more, I want to cache the fact that a frame is
empty in its frame state. I can free one state bit (NS_FRAME_IN_REFLOW, which is
only used in assertions). We may need more state bits and I need to figure out
how to invalidate the cached emptiness state whenever it might have changed.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I was hoping to inalidate the cached state when we build the reflow path tree
for an incremental reflow, because content and style changes that could change
emptiness will cause incremental reflows, but I think emptiness can also change
during resize reflows (e.g. a frame gets split and that part corresponding to
non-empty content goes to the next line).

Comment 2

[Boris Zbarsky [:bzbarsky]]

Note that we have some state bits begging to be removed...
NS_FRAME_HAS_LOADED_IMAGES and NS_FRAME_SYNC_FRAME_AND_VIEW (bug 97934 covers
the latter).

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> NS_FRAME_HAS_LOADED_IMAGES

What is this?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Oh, looks like bryner has turned that one into NS_FRAME_IS_BOX...

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm not sure why it would be deleting a frame here.

Could be frame parenting confusion I guess.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Oops, the previous comment was meant for another bug.

Anyway, perhaps a better approach would be to cache *line* emptiness instead,
and cache selectively. Add a new method CachedIsEmpty(). It caches the emptiness
state under the following restrictions:
-- The cache is only valid while we are in Reflow() for the block the line
belongs to. It is only valid between marking lines dirty due to reflow reasons
and the end of reflowing this block. The caller must avoid calling
CachedIsEmpty() outside these situations.
-- The cache is not valid if the line is dirty. In this case CachedIsEmpty()
will fall through to IsEmpty().
-- The cache must be invalidated after reflowing the line.
I think that those restrictions are enough to guarantee that CachedIsEmpty()
will be consistent with IsEmpty() if we have valid cached state.
Note that these rules allow the emptiness state to be cached between reflows on
lines that don't get marked dirty, even though the state can only be used during
reflow of the block.

With these rules most calls to line->IsEmpty() in nsBlockFrame will be
convertible to CachedIsEmpty() (with the noticeable exception of
nsBlockFrame::IsEmpty()), and so will the new calls in bug 209694.
CachedIsEmpty() can cache its state in the line state bits, which there are
plenty of.

Comment 7

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

Aren't many of the cases when you'd want to test emptiness going to be outside
of the reflow of the block?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah.

But maintaining generally valid emptiness state requires us to invalidate or
update that state every time something changes in the child frame subtree.
That's quite a lot of code and would be fragile to maintain.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: line emptiness caching patch

This is quite simple.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 166054 [details] [diff] [review]
line emptiness caching patch

unambitious, perhaps, but seems to work

Comment 11

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

Comment on attachment 166054 [details] [diff] [review]
line emptiness caching patch

r+sr=dbaron

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in. Small Tp reduction in btek only. But hopefully it will mitigate the
damage from bug 209694.