Disentangle repaint-after-compositing from updateLayerPositionsAfterLayout

Disentangle repaint-after-compositing from updateLayerPositionsAfterLayout

Rather than using updateLayerPositions to propagate mayNeedPaintInvalidation
down the tree, this CL adds the notion of forcing a check for paint
invalidation during the invalidate tree walk. Now, if a RenderBox or a
relatively positioned RenderInline moves, we'll check all its in-flow
descendants for whether they need their paint invalidated.

R=jchaffraix@chromium.org

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

[leviw_travelin_and_unemployed, 2014-07-31 18:19:18]

Are we able to reset the bit in PaintInvalidationState when we cross a
composited boundary?

[abarth-chromium, 2014-07-31 20:32:29]

On 2014/07/31 at 18:19:18, leviw wrote:
> Are we able to reset the bit in PaintInvalidationState when we cross a
composited boundary?

Julien thinks we should be able to.  I'll give it a try.

[abarth-chromium, 2014-08-01 18:57:24]

Ok, this should be ready for review.  I did as leviw suggested and cut off the
forced check at invalidation containers.  Maybe we should use a longer name that
makes that more explicit?

This CL causes a number of tests to need new baselines.  I've verified manually
that we invalidate exactly the same set of pixels for these tests.

[Julien - ping for review, 2014-08-01 21:20:02]

https://codereview.chromium.org/433603004/diff/40001/Source/core/rendering/Pa...
File Source/core/rendering/PaintInvalidationState.cpp (right):

https://codereview.chromium.org/433603004/diff/40001/Source/core/rendering/Pa...
Source/core/rendering/PaintInvalidationState.cpp:64:
m_forceCheckForPaintInvalidation = false;
Let's add a comment about why we clear the flag as it could be difficult to
remember the reason later on.

https://codereview.chromium.org/433603004/diff/40001/Source/core/rendering/Pa...
Source/core/rendering/PaintInvalidationState.cpp:94:
m_forceCheckForPaintInvalidation = false;
Ditto.

https://codereview.chromium.org/433603004/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/433603004/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:1573: if (didFullyInvalidatePaint)
AFAICT this will make us force paint invalidation for other reasons than a
containing block shift. I would bet it's the reason why we have all these extra
invalidations. Those invalidations are not over-painting but they are a sign
that we are doing too much work.

I would be more comfortable if we limited this condition to only when we
actually need it.

https://codereview.chromium.org/433603004/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:1604: if (RenderLayerScrollableArea* area =
layer->scrollableArea()) {
I wouldn't touch this, we spotted that this code doesn't seem right:
enclosingLayer() can repeatedly return the same RenderLayer in a subtree leading
to unneeded invalidations. We will change or refactor this code anyway.

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

https://codereview.chromium.org/433603004/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:1158: for (RenderObject* container =
this->container(); container && !container->mayNeedPaintInvalidation() &&
!container->layoutDidGetCalled(); container = container->container())
Won't this make us over-mark as we used to break early for full paint
invalidation and full paint invalidation if self-painting layer?

[abarth-chromium, 2014-08-01 21:22:52]

https://codereview.chromium.org/433603004/diff/40001/Source/core/rendering/Pa...
File Source/core/rendering/PaintInvalidationState.cpp (right):

https://codereview.chromium.org/433603004/diff/40001/Source/core/rendering/Pa...
Source/core/rendering/PaintInvalidationState.cpp:64:
m_forceCheckForPaintInvalidation = false;
Ok.

https://codereview.chromium.org/433603004/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/433603004/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:1573: if (didFullyInvalidatePaint)
Ok.  I'll tighten this down.

https://codereview.chromium.org/433603004/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:1604: if (RenderLayerScrollableArea* area =
layer->scrollableArea()) {
ok

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

https://codereview.chromium.org/433603004/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:1158: for (RenderObject* container =
this->container(); container && !container->mayNeedPaintInvalidation() &&
!container->layoutDidGetCalled(); container = container->container())
Sure.  I'll undo this part of the change.  The downside is that I'll need to
introduce a version of shouldCheckForPaintInvalidation that does not take a
PaintInvalidationState object.

[abarth-chromium, 2014-08-01 22:22:06]

Done, PTAL

[Julien - ping for review, 2014-08-01 23:50:05]

https://codereview.chromium.org/433603004/diff/80001/LayoutTests/platform/mac...
File LayoutTests/platform/mac/fast/repaint/float-overflow-expected.txt (right):

https://codereview.chromium.org/433603004/diff/80001/LayoutTests/platform/mac...
LayoutTests/platform/mac/fast/repaint/float-overflow-expected.txt:26: [63, 15,
42, 18],
This is my biggest concern with this patch. The code is fine and I have no
objection to it.

While I don't think the extra invalidations are costing us a lot, it's still not
correct to generate tons of extra redundant invalidations. We have tried to
reduce them as we implemented repaint-after-layout and it's a clear regression
from that perspective.

If the patch was solving a correctness bug, a case could be made for the extra
invalidations. However this change is a refactoring so it's a pure regression
from my perspective.

[abarth-chromium, 2014-08-02 01:14:36]

On 2014/08/01 at 23:50:05, jchaffraix wrote:
> If the patch was solving a correctness bug, a case could be made for the extra
invalidations. However this change is a refactoring so it's a pure regression
from my perspective.

This CL is necessary to unlock performance optimizations for Silk.  With repaint
entangled in updateLayerPositionsAfterLayout, I cannot make
updateLayerPositionsAfterLayout go fast.  Currently,
updateLayerPositionsAfterLayout is ~15% of total layout time in a layout-based
animation for search, which is almost entirely wasted time in each frame.

[Julien - ping for review, 2014-08-04 15:56:55]

On 2014/08/02 at 01:14:36, abarth wrote:
> On 2014/08/01 at 23:50:05, jchaffraix wrote:
> > If the patch was solving a correctness bug, a case could be made for the
extra invalidations. However this change is a refactoring so it's a pure
regression from my perspective.
> 
> This CL is necessary to unlock performance optimizations for Silk.  With
repaint entangled in updateLayerPositionsAfterLayout, I cannot make
updateLayerPositionsAfterLayout go fast.  Currently,
updateLayerPositionsAfterLayout is ~15% of total layout time in a layout-based
animation for search, which is almost entirely wasted time in each frame.

I am not doubting the usefulness of the change at all, it's really awesome to
improve updateLayersPositionsAfterLayout. I am just saying that all the
redundant invalidations from this patch are unneeded and are a definite
regression from our current implementation.

[abarth-chromium, 2014-08-04 16:18:43]

On 2014/08/04 at 15:56:55, jchaffraix wrote:
> I am not doubting the usefulness of the change at all, it's really awesome to
improve updateLayersPositionsAfterLayout. I am just saying that all the
redundant invalidations from this patch are unneeded and are a definite
regression from our current implementation.

What approach would you suggest?

[Julien - ping for review, 2014-08-04 16:37:15]

On 2014/08/04 at 16:18:43, abarth wrote:
> On 2014/08/04 at 15:56:55, jchaffraix wrote:
> > I am not doubting the usefulness of the change at all, it's really awesome
to improve updateLayersPositionsAfterLayout. I am just saying that all the
redundant invalidations from this patch are unneeded and are a definite
regression from our current implementation.
> 
> What approach would you suggest?

We need to understand why we are getting all these redundant invalidations,
especially on the 2 floats repaint tests. My hunch would be that we would want
to mark the whole subtree on location change only for a positioned element.

[Julien - ping for review, 2014-08-04 18:23:49]

On 2014/08/04 at 16:37:15, Julien Chaffraix - PST wrote:
> On 2014/08/04 at 16:18:43, abarth wrote:
> > On 2014/08/04 at 15:56:55, jchaffraix wrote:
> > > I am not doubting the usefulness of the change at all, it's really awesome
to improve updateLayersPositionsAfterLayout. I am just saying that all the
redundant invalidations from this patch are unneeded and are a definite
regression from our current implementation.
> > 
> > What approach would you suggest?
> 
> We need to understand why we are getting all these redundant invalidations,
especially on the 2 floats repaint tests. My hunch would be that we would want
to mark the whole subtree on location change only for a positioned element.

We talked about this with Adam. The extra invalidations that we see are due to
applying the forced invalidations to more renderers (we used to do that only for
renderers with a RenderLayer).

Also upon inspection, the invalidations are new and not redundant (just
contained in a previous ones which is different) so it's not as bad as I
thought.

I don't think I should block this patch, we can figure out how to cut the new
invalidations if it makes sense (and if it hurts us, which is unlikely). LGTM.

[abarth-chromium, 2014-08-04 19:06:48]

Thanks Julien!

[abarth-chromium, 2014-08-04 19:07:34]

The CQ bit was checked by abarth@chromium.org

[commit-bot: I haz the power, 2014-08-04 19:08:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/abarth@chromium.org/433603004/100001

[commit-bot: I haz the power, 2014-08-04 20:19:18]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_chromium_gn_comp...)
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_chromium_gn_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20595)

[commit-bot: I haz the power, 2014-08-04 21:01:04]

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

[commit-bot: I haz the power, 2014-08-04 21:01:07]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20615)

[abarth-chromium, 2014-08-04 21:38:50]

The CQ bit was checked by abarth@chromium.org

[commit-bot: I haz the power, 2014-08-04 21:39:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/abarth@chromium.org/433603004/100001

[commit-bot: I haz the power, 2014-08-04 22:36:53]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20641)

[commit-bot: I haz the power, 2014-08-04 23:19:57]

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

[commit-bot: I haz the power, 2014-08-04 23:19:59]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20658)

[abarth-chromium, 2014-08-04 23:45:42]

The CQ bit was checked by abarth@chromium.org

[commit-bot: I haz the power, 2014-08-04 23:47:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/abarth@chromium.org/433603004/100001

[commit-bot: I haz the power, 2014-08-05 00:30:01]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20677)

[commit-bot: I haz the power, 2014-08-05 01:11:38]

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

[commit-bot: I haz the power, 2014-08-05 01:11:40]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20691)

[abarth-chromium, 2014-08-05 01:21:34]

The CQ bit was checked by abarth@chromium.org

[commit-bot: I haz the power, 2014-08-05 01:22:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/abarth@chromium.org/433603004/100001

[commit-bot: I haz the power, 2014-08-05 02:25:30]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20704)

[commit-bot: I haz the power, 2014-08-05 02:33:29]

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

[commit-bot: I haz the power, 2014-08-05 02:33:30]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20720)

[abarth-chromium, 2014-08-05 06:29:24]

The CQ bit was checked by abarth@chromium.org

[commit-bot: I haz the power, 2014-08-05 06:30:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/abarth@chromium.org/433603004/100001

[commit-bot: I haz the power, 2014-08-05 06:39:32]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20770)

[commit-bot: I haz the power, 2014-08-05 07:23:42]

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

[commit-bot: I haz the power, 2014-08-05 07:23:43]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20775)

[abarth-chromium, 2014-08-05 16:10:45]

The CQ bit was checked by abarth@chromium.org

[commit-bot: I haz the power, 2014-08-05 16:12:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/abarth@chromium.org/433603004/100001

[abarth-chromium, 2014-08-05 18:13:31]

Committed patchset #6 manually as 179514 (presubmit successful).