Optimize repaint on FrameView resize

Optimize repaint on FrameView resize

Previously we always repaint the full FrameView on resize.

Don't mark full repaint if the resize is height-only and writing mode
is horizontal. Any descendants needing repaint because of FrameView
resize will still schedule their own repaints.

BUG=258219
TEST=WebFrameTest.sizeChangeRepaint

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170427

[Xianzhu, 2014-03-26 17:30:00]

As https://codereview.chromium.org/197283038/ and
https://codereview.chromium.org/204813002/ touch the rendering core and may need
more time to pass review, this change optimizes repaint only.

[Xianzhu, 2014-03-26 17:36:25]

The new optional parameter 'fullRepaintLayer' of RenderObject::setNeedsLayout()
may look not good, but eventually we'll remove it after we separate repaint and
layout requirements of StyleDifference (for now StyleDifferenceLayout implies
full layout and full repaint). trchen@ is working on it.

[esprehn, 2014-03-26 17:46:16]

lgtm, Ojan might want to take a look too since he has opinions about how
setNeedsLayout should work.

https://codereview.chromium.org/213103003/diff/1/Source/core/frame/FrameView.cpp
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/213103003/diff/1/Source/core/frame/FrameView....
Source/core/frame/FrameView.cpp:1751: renderView->setNeedsLayout(MarkOnlyThis,
0, false);
You should not pass MarkOnlyThis. It doesn't really matter for a RenderView, but
it's wrong in general so I'd suggest not doing it.

[Xianzhu, 2014-03-26 18:08:39]

https://codereview.chromium.org/213103003/diff/1/Source/core/frame/FrameView.cpp
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/213103003/diff/1/Source/core/frame/FrameView....
Source/core/frame/FrameView.cpp:1751: renderView->setNeedsLayout(MarkOnlyThis,
0, false);
On 2014/03/26 17:46:17, esprehn wrote:
> You should not pass MarkOnlyThis. It doesn't really matter for a RenderView,
but
> it's wrong in general so I'd suggest not doing it.

Done.

[Xianzhu, 2014-03-27 16:31:23]

ojan@, would you please take a look?

[ojan, 2014-03-28 01:51:36]

https://codereview.chromium.org/213103003/diff/20001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/213103003/diff/20001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:944: if
(!renderView()->style()->isHorizontalWritingMode() || oldSize.width() !=
m_size.width())
I'm not sure I follow this...isn't it also the case that for a width change, any
descendants that need to repaint will issue their own repaints?

https://codereview.chromium.org/213103003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/213103003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:614: void setNeedsLayout(MarkingBehavior =
MarkContainingBlockChain, SubtreeLayoutScope* = 0, bool fullRepaintLayer =
true);
We typically don't do booleans like this. This should be an enum instead. That
way it's clear in the calling code what this value represents.

https://codereview.chromium.org/213103003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:1281: setLayerNeedsFullRepaint();
Really this code is wrong. I think we should probably have a FIXME to make this
code go away entirely, or at least to make fullRepaintLayer default to false.

[ojan, 2014-03-28 02:18:16]

To be clear, I'm not saying the patch is wrong...I just don't understand repaint
well enough to understand why height and width changes are different.

[Xianzhu, 2014-03-28 16:26:00]

https://codereview.chromium.org/213103003/diff/20001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/213103003/diff/20001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:944: if
(!renderView()->style()->isHorizontalWritingMode() || oldSize.width() !=
m_size.width())
On 2014/03/28 01:51:36, ojan wrote:
> I'm not sure I follow this...isn't it also the case that for a width change,
any
> descendants that need to repaint will issue their own repaints?

Added comments here.

https://codereview.chromium.org/213103003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/213103003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:614: void setNeedsLayout(MarkingBehavior =
MarkContainingBlockChain, SubtreeLayoutScope* = 0, bool fullRepaintLayer =
true);
On 2014/03/28 01:51:36, ojan wrote:
> We typically don't do booleans like this. This should be an enum instead. That
> way it's clear in the calling code what this value represents.

Done.

https://codereview.chromium.org/213103003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:1281: setLayerNeedsFullRepaint();
On 2014/03/28 01:51:36, ojan wrote:
> Really this code is wrong. I think we should probably have a FIXME to make
this
> code go away entirely, or at least to make fullRepaintLayer default to false.

Done.

trchen@ is working on separating repaint and layout requirements in
StyleDifference. After that, perhaps we can remove the new parameter and the
above statement.

[eseidel, 2014-03-28 16:36:40]

https://codereview.chromium.org/213103003/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/213103003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:1286: if (repaintLayer == RepaintLayer &&
hasLayer())
This repaint call is just wrong IMO.  See crbug.com/325569.  Layout's job is to
adjust if needed and only issue repaints if needed.  It should not be assumed
that just because we mark something as needing layout it will need repaint.

This call was added years ago, but I think we should work to remove it now.

[eseidel, 2014-03-28 16:41:54]

I'm OK with this change.  This fix would not be necessary if we fixed
crbug.com/325569 first.  But I can understand that this would be easier to do
first, and this does move us in a good direction, even if we're going to rip
some of this code out.

I worry we're making this change w/o fully understanding the situation and could
be breaking something subtle which we would have caught by trying to do the
larger fix.  But OK.  lgtm.

[Xianzhu, 2014-03-28 16:53:07]

On 2014/03/28 16:41:54, eseidel wrote:
> I'm OK with this change.  This fix would not be necessary if we fixed
> crbug.com/325569 first.  But I can understand that this would be easier to do
> first, and this does move us in a good direction, even if we're going to rip
> some of this code out.
> 

Fixing crbug.com/325569 will eliminate the full layer repaint by
RenderObject::setNeedsLayout(), but won't cover FrameView::m_doFullRepaint. This
fix needs both of them.

> I worry we're making this change w/o fully understanding the situation and
could
> be breaking something subtle which we would have caught by trying to do the
> larger fix.  But OK.  lgtm.

Thanks.

Will CQ. Fingers crossed.

[Xianzhu, 2014-03-28 16:53:14]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-03-28 16:53:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/213103003/40001

[commit-bot: I haz the power, 2014-03-28 17:00:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-28 17:00:25]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[ojan, 2014-03-28 17:16:19]

Can you explain what's different between width and height?

eseidel maybe you understand?

[Xianzhu, 2014-03-28 17:21:25]

On 2014/03/28 17:16:19, ojan wrote:
> Can you explain what's different between width and height?

For most real world cases, the page contains text which is layout according to
the width, so width change will be more likely to cause full layout and repaint.

Another reason that I care more about height change is that bug 258219 is about
the layout/repaint cost of Android top-control auto show/hide which affects
height only.

[Xianzhu, 2014-03-28 17:31:16]

I tried not to mark full repaint for width change and vertical writing mode. It
caused a few repaint layout tests to generate many more small repaint rects,
while the combined repaint rects still cover the full pages. In the cases,
layouts without marking full repaint cost more about managing more layout states
(RenderView::pushLayoutState()) and more discrete smaller repaint rects.

[eseidel, 2014-03-28 18:33:24]

Layout is width in, height out.  Except when dealing with vertical writing mode,
in which case it's height in, width out.

[ojan, 2014-03-28 18:51:38]

On 2014/03/28 17:21:25, Xianzhu wrote:
> Another reason that I care more about height change is that bug 258219 is
about
> the layout/repaint cost of Android top-control auto show/hide which affects
> height only.

I understand this motivation. I'm just trying to keep the codebase as simple and
maintainable as possible.

On 2014/03/28 17:31:16, Xianzhu wrote:
> I tried not to mark full repaint for width change and vertical writing mode.
It
> caused a few repaint layout tests to generate many more small repaint rects,
> while the combined repaint rects still cover the full pages. In the cases,
> layouts without marking full repaint cost more about managing more layout
states
> (RenderView::pushLayoutState()) and more discrete smaller repaint rects.

I see, doing the full repaint is actually a performance optimization to avoid
computing repaint rects in the case where most the page is changing, which is
the common case when the width changes. We need to think about this more. Cases
where most of the page content is in a fixed width container are hurt by this
(e.g. google+ or facebook). Maybe repaint after layout will change the
performance side of this equation such that we don't need this code at all
anymore.

In either case, I'm fine with this patch landing. I just wanted to understand
more. I think the comment in the code is helpful.

On 2014/03/28 18:33:24, eseidel wrote:
> Layout is width in, height out.  Except when dealing with vertical writing
mode,
> in which case it's height in, width out.

Right, but that doesn't answer the question about why height/width are different
with respect repaint.

[ojan, 2014-03-28 18:51:42]

The CQ bit was checked by ojan@chromium.org

[commit-bot: I haz the power, 2014-03-28 18:51:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/213103003/40001

[eseidel, 2014-03-28 18:56:26]

I think we should wiat until Ojan is back from lunch and we can discuss more.

[Xianzhu, 2014-03-28 18:56:42]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2014-03-28 18:57:23]

The CQ bit was unchecked by wangxianzhu@chromium.org

[Xianzhu, 2014-03-28 19:05:44]

I uploaded a new Patch Set (to fix nullptr break) after ojan@ checked the CQ
bit, and checked CQ bit of the new Patch Set, then saw eseidel@'s comment, so
unchecked the CQ bit. I'll be on vacation in afternoon. Please help check CQ bit
if no more concerns after discussion :)

A question about repaint-after-layout: would it eliminate the need of
LayoutState in some cases like what FrameView::m_doFullRepaint does for now?

[ojan, 2014-03-29 00:39:35]

The CQ bit was checked by ojan@chromium.org

[commit-bot: I haz the power, 2014-03-29 00:39:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/213103003/60001

[Julien - ping for review, 2014-03-29 01:01:26]

lgtm

I think this works but it's _very_ fragile as it relies on the implicit
disabling of LayoutRepainter for the RenderView and the checks in
RenderView::layout to avoid relaying out the <html> renderer (which would
trigger a full viewport invalidation). As we have some good coverage, this
shouldn't be too bad though.

https://codereview.chromium.org/213103003/diff/60001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/213103003/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:950: if
(!renderView()->style()->isHorizontalWritingMode() || oldSize.width() !=
m_size.width())
I don't think the isHorizontalWritingMode() check is really working. Most
web-authors put the writing mode on the <html> or the <body> so you will miss
these cases with this check.

https://codereview.chromium.org/213103003/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/213103003/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:1285: // FIXME: This may be incorrect, but
needed at least because StyleDifferenceLayout implies repaint for now.
This is not strictly incorrect: it's conservative and in some cases unneeded
which is different AFAICT.

Also this FIXME is not really explaining the 'why' it's required or what we
could do to improve the situation so let's try to improve it:

// StyleDifferenceLayout is used for different cases currently, one of which is
that our content changed which mandates an invalidation.
// FIXME: We should be able to skip this automatic invalidation (see
crbug.com/325569).

[Julien - ping for review, 2014-03-29 01:01:38]

The CQ bit was unchecked by jchaffraix@chromium.org

[ojan, 2014-03-29 01:15:45]

https://codereview.chromium.org/213103003/diff/60001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/213103003/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:950: if
(!renderView()->style()->isHorizontalWritingMode() || oldSize.width() !=
m_size.width())
On 2014/03/29 01:01:26, Julien Chaffraix - PST wrote:
> I don't think the isHorizontalWritingMode() check is really working. Most
> web-authors put the writing mode on the <html> or the <body> so you will miss
> these cases with this check.

The html and body writing modes get propagated up to the RenderView.

[Xianzhu, 2014-03-30 22:26:52]

https://codereview.chromium.org/213103003/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/213103003/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:1285: // FIXME: This may be incorrect, but
needed at least because StyleDifferenceLayout implies repaint for now.
On 2014/03/29 01:01:26, Julien Chaffraix - PST wrote:
> This is not strictly incorrect: it's conservative and in some cases unneeded
> which is different AFAICT.
> 
> Also this FIXME is not really explaining the 'why' it's required or what we
> could do to improve the situation so let's try to improve it:
> 
> // StyleDifferenceLayout is used for different cases currently, one of which
is
> that our content changed which mandates an invalidation.
> // FIXME: We should be able to skip this automatic invalidation (see
> crbug.com/325569).

Done.

[Xianzhu, 2014-03-30 22:27:02]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-03-30 22:27:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/213103003/80001

[eseidel, 2014-03-30 22:42:07]

This CL does two things:

1.  It further tweaks an optimization in layout() to trade the cost of doing a
full repaint with the cost of repaint upkeep during layout.  This optimization
is not well factored or documented, but we believe that you're making it better.
 It would be nice to factor this code out into some sort of helper method which
made more obvious its purpose.

2.  This code makes a partial fix to crbug.com/325569.  I worry slightly that
this partial fix may expose some of the bugs that full fix of crbug.com/32556
would have to fight through, and that by only doing a partial fix we may just be
setting ourselves up to discover those bugs in rarer scenerios where they might
slip into stable. :)

That said, especially if you're interested in continuing work on either of these
fixes, I'm OK with landing this change as is.  I don't believe this makes the
current code worse, but we still have a long way to make it better.

[esprehn, 2014-03-30 23:21:02]

I'll look at this on Monday, I'm a little concerned about flooding ourself
with invalidations. The peacekeeper benchmark (the colored rects at the
start) shows what it looks like to issue thousands of invalidations. The cc
tile management and invalidation logic starts to really slow you down.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-03-31 07:02:26]

Change committed as 170427

[Xianzhu, 2014-03-31 16:16:25]

On 2014/03/30 23:21:02, esprehn wrote:
> I'll look at this on Monday, I'm a little concerned about flooding ourself
> with invalidations. The peacekeeper benchmark (the colored rects at the
> start) shows what it looks like to issue thousands of invalidations. The cc
> tile management and invalidation logic starts to really slow you down.

How about repaint-after-layout?

[Xianzhu, 2014-04-11 18:23:18]

A revert of this CL has been created in
https://codereview.chromium.org/235323005/ by wangxianzhu@chromium.org.

The reason for reverting is: Under-repaint if body has percentage positioned or
bottom aligned background-image when changing height.

Will work on bug 358460 first.

BUG=362407.