Move paint invalidation after compositing update

Move paint invalidation after compositing update

Now that repaint-after-layout is the new world, we can make
invalidation its own phase. Moving it after compositing update
removes chicken-and-egg bugs that we had in the invalidation code.

The new code invalidates from the layout roots up to the RenderView
so that we can always do a full tree traversal. This shouldn't
trigger extra invalidations and yield to a more simple code.

I checked the new baselines visually and it's overall a neutral
change, where we do better one some tests
(e.g. fast/repaint/intermediate-layout-position-clip.html) and we
do worse on others (e.g. fast/repaint/layout-state-scrolloffset*).
The bulk of the text changes are invalidation going to a different
GraphicsLayer which indicates that we had some compositing
container changes not accounted for.

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

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

[abarth-chromium, 2014-06-24 22:57:27]

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

https://codereview.chromium.org/351673007/diff/20001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:962: m_paintInvalidationSubtreeRoot =
rootForThisLayout;
What happens if there are multiple layouts between two compositing updates? 
Will the last one overwrite this value and cause us to potentially miss
invalidations?

[esprehn, 2014-06-25 00:20:32]

Do we really need this subtree stuff yet? Can we just walk the entire render
tree to start and then add the subtree root in a separate patch?

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

https://codereview.chromium.org/351673007/diff/20001/Source/core/frame/FrameV...
Source/core/frame/FrameView.h:443: RenderObject* m_paintInvalidationSubtreeRoot;
I think this is a use-after-free, what updates this if the renderer is
destroyed?

[leviw_travelin_and_unemployed, 2014-06-25 00:21:42]

On 2014/06/25 at 00:20:32, esprehn wrote:
> Do we really need this subtree stuff yet? Can we just walk the entire render
tree to start and then add the subtree root in a separate patch?

That's what we talked about on IRC. Julien said he'd spin a patch with that
change.

[Julien - ping for review, 2014-06-25 22:33:31]

On 2014/06/25 at 00:21:42, leviw wrote:
> On 2014/06/25 at 00:20:32, esprehn wrote:
> > Do we really need this subtree stuff yet? Can we just walk the entire render
tree to start and then add the subtree root in a separate patch?
> 
> That's what we talked about on IRC. Julien said he'd spin a patch with that
change.

I evaluated this option but I think that's not a great alternative (contrary to
what I said previously). The reason is that we have to implement some code to
mark up to the common ancestor (akin to what subtree layout root does), which
will yield to extra invalidations (the code is not smart enough to handle them).
It's also more complicated than using a straight HashSet and just iterating over
the accumulated roots.

My biggest concern with using a HashSet is the potential for (mostly) unbounded
growth but running the solution on fast/ (1/3 of our test cases) I found out
that we have at most 6 entries, with most calls to invalidateTree having 0 or 1.
The reason is that forcing repeated layout ends up returning the same subtree
layout root.

[Julien - ping for review, 2014-06-25 22:34:30]

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

https://codereview.chromium.org/351673007/diff/20001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:962: m_paintInvalidationSubtreeRoot =
rootForThisLayout;
On 2014/06/24 22:57:26, abarth wrote:
> What happens if there are multiple layouts between two compositing updates? 
> Will the last one overwrite this value and cause us to potentially miss
> invalidations?

That's indeed a bug. I will fix it and land a test case for that.

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

https://codereview.chromium.org/351673007/diff/20001/Source/core/frame/FrameV...
Source/core/frame/FrameView.h:443: RenderObject* m_paintInvalidationSubtreeRoot;
On 2014/06/25 00:20:31, esprehn wrote:
> I think this is a use-after-free, what updates this if the renderer is
> destroyed?

Yeah, I thought I had included the part about clearing it like
m_layoutSubtreeRoot but I forgot.

[Julien - ping for review, 2014-06-25 23:01:13]

New patch up for review, it should address the reviews' comments.

[leviw_travelin_and_unemployed, 2014-06-25 23:05:50]

Out of curiosity, have you compared local perf results before/after?

[Julien - ping for review, 2014-06-26 00:02:25]

On 2014/06/25 at 23:05:50, leviw wrote:
> Out of curiosity, have you compared local perf results before/after?

I haven't done any performance measurement for this change but if you have
something in mind, I could definitely check it out.

[Julien - ping for review, 2014-07-01 16:12:17]

Following a discussion with Elliott on #blink, he convinced me to do the full
RenderView invalidation all the time. New patch up for review that does exactly
that.

[abarth-chromium, 2014-07-01 20:02:19]

https://codereview.chromium.org/351673007/diff/100001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/351673007/diff/100001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:966:
container->setMayNeedPaintInvalidation(true);
    void setMayNeedPaintInvalidation(bool b)
    {
        m_bitfields.setMayNeedPaintInvalidation(b);

        // Make sure our parent is marked as needing invalidation.
        if (b && parent() && !parent()->mayNeedPaintInvalidation())
            parent()->setMayNeedPaintInvalidation(b);
    }

[esprehn, 2014-07-01 20:03:34]

This lgtm, but I don't think that extra ancestor walk is needed. You already
walk up to the root inside setMayNeedPaintInvalidation. That also means that
previously when we would only start at the layoutRoot we were leaving the
MayNeedPaintInvalidation bits on ancestors of the layout root.

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

https://codereview.chromium.org/351673007/diff/40001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:711: ASSERT_NOT_REACHED();
RELEASE_ASSERT(!m_layoutSubtreeRoot->documentBeingDestroyed());

https://codereview.chromium.org/351673007/diff/100001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/351673007/diff/100001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:966:
container->setMayNeedPaintInvalidation(true);
This shouldn't be needed, setMayNeedPaintInvalidation already walks up through
all the parent()'s and the container() is going to be one of those.

[Julien - ping for review, 2014-07-01 21:44:36]

https://codereview.chromium.org/351673007/diff/100001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/351673007/diff/100001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:966:
container->setMayNeedPaintInvalidation(true);
On 2014/07/01 20:03:34, esprehn wrote:
> This shouldn't be needed, setMayNeedPaintInvalidation already walks up through
> all the parent()'s and the container() is going to be one of those.

I do need to call
rootForThisLayout->container()->setMayNeedPaintInvalidation(true) as we may not
have called it inside the subtree (and the containers' chain may not have been
marked for layout too). As you both pointed out, we don't need the for loop
though. Here is some better code that I will use for the final patch:

if (RenderObject* container = rootForThisLayout->container())
   container->setMayNeedPaintInvalidation(true);

[Julien - ping for review, 2014-07-01 22:36:44]

The CQ bit was checked by jchaffraix@chromium.org

[commit-bot: I haz the power, 2014-07-01 22:37:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jchaffraix@chromium.org/351673007/140001

[commit-bot: I haz the power, 2014-07-02 01:47:46]

Change committed as 177349

[ojan, 2014-07-02 03:33:58]

https://codereview.chromium.org/351673007/diff/140001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/351673007/diff/140001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:955: // The following loop ensures that we mark
up all renderers up to the RenderView
This comment is out of data now that this isn't a loop anymore.

https://codereview.chromium.org/351673007/diff/140001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:960:
container->setMayNeedPaintInvalidation(true);
Why not just have this be rootForThisLayout->setMayNeedPaintInvalidation(true)?

[lushnikov, 2014-07-02 06:35:28]

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

The reason for reverting is: Tests
fast/repaint/intermediate-layout-position-clip.html and
fast/repaint/table-shrink-row-repaint.html start to fail after this change

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40....

[Julien - ping for review, 2014-07-02 18:27:06]

https://codereview.chromium.org/351673007/diff/140001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/351673007/diff/140001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:955: // The following loop ensures that we mark
up all renderers up to the RenderView
On 2014/07/02 03:33:58, OOO-only-code-yellow-reviews wrote:
> This comment is out of data now that this isn't a loop anymore.

oops, got catch. I have amended the comment to avoid mentioning the loop. Also
removed the comment as it's a more general issue.

https://codereview.chromium.org/351673007/diff/140001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:960:
container->setMayNeedPaintInvalidation(true);
On 2014/07/02 03:33:58, OOO-only-code-yellow-reviews wrote:
> Why not just have this be
rootForThisLayout->setMayNeedPaintInvalidation(true)?

It's not needed to mark rootForThisLayout for mayNeedPaintInvalidation as it was
laid out and it's in this spirit that I have started at the container. Marking
it with setMayNeedPaintInvalidation will skip some optimization we have now so I
would rather not do it. Also conceptually it doesn't need it and
mayNeedPaintInvalidation is a giant hammer we have to force paint invalidation
on a renderer.

[Julien - ping for review, 2014-07-02 19:51:54]

On 2014/07/02 at 06:35:28, lushnikov wrote:
> A revert of this CL has been created in
https://codereview.chromium.org/369483004/ by lushnikov@chromium.org.
> 
> The reason for reverting is: Tests
fast/repaint/intermediate-layout-position-clip.html and
fast/repaint/table-shrink-row-repaint.html start to fail after this change
> 
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40....

The reasons for reverting were actually expected changes:
- intermediate-layout-position-clip.html looked like a regression but it's
actually a progression (I assume it's due to the interaction with
https://codereview.chromium.org/204843002 that we didn't see the change before)
- table-shrink-row-repaint.html wasn't added to TestExpectations due to another
change touching it but it was expected to change.

I will land the change again.

[Julien - ping for review, 2014-07-02 19:52:00]

The CQ bit was checked by jchaffraix@chromium.org

[commit-bot: I haz the power, 2014-07-02 19:52:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jchaffraix@chromium.org/351673007/160001

[commit-bot: I haz the power, 2014-07-02 19:56:42]

Change committed as 177415

[lushnikov, 2014-07-02 19:59:56]

On 2014/07/02 19:56:42, I haz the power (commit-bot) wrote:
> Change committed as 177415

Nice, thank you.