Implement a paint offset cache for slimming paint v2

Implement a paint offset cache for slimming paint v2

This patch implements paint offset cache as described in:
https://docs.google.com/document/d/1oY8P7a7G4W2LYOvT3VyqUNKOOek6S_e-jD2XjM_7g9U/view

This patch is large but only a few new concepts have been added:
1) A LayoutPoint for paint offset is now passed into all layout drawing
recorders.
2) The display item list temporarily stores the previous paint offsets.
These will be moved onto the LayoutObject themselves once
https://codereview.chromium.org/1315213002 lands.
3) If a new paint offset is detected during paint, the recorded offset is
updated and the layout object is marked as invalid in the display list.
4) Asserts have been added that paint offsets do not change between
phases. DisplayItemList::m_clientsWithPaintOffsetInvalidations checks
for emitting cached display items after the paint offset was invalidated.
DisplayItemList::invalidatePaintOffset checks for existing non-cached
display items added before the paint offset was invalidated.

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

[pdr., 2015-08-27 01:41:32]

pdr@chromium.org changed reviewers:
+ trchen@chromium.org, wangxianzhu@chromium.org

[pdr., 2015-08-27 01:43:19]

Ready for an initial review

[Xianzhu, 2015-08-27 17:20:10]

https://codereview.chromium.org/1315993004/diff/1/Source/core/layout/LayoutOb...
File Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/1315993004/diff/1/Source/core/layout/LayoutOb...
Source/core/layout/LayoutObject.h:1438: mutable LayoutPoint m_cachedPaintOffset;
Would m_previousPaintOffset be better, as we don't use it as a cache (to speedup
access), but compare it to the current paint offset to do invalidation?

https://codereview.chromium.org/1315993004/diff/1/Source/core/paint/BlockFlow...
File Source/core/paint/BlockFlowPainter.cpp (right):

https://codereview.chromium.org/1315993004/diff/1/Source/core/paint/BlockFlow...
Source/core/paint/BlockFlowPainter.cpp:67: bool skipRecording =
LayoutObjectDrawingRecorder::useCachedDrawingIfPossible(*paintInfo.context,
m_layoutBlockFlow, DisplayItem::SelectionGap, paintOffset);
I remember your idea of DrawingParameters when adding
useCachedDrawingIfPossible. Perhaps it's time to use it?

[Stephen Chennney, 2015-08-27 17:25:40]

schenney@chromium.org changed reviewers:
+ schenney@chromium.org

[Stephen Chennney, 2015-08-27 17:25:41]

https://codereview.chromium.org/1315993004/diff/1/Source/platform/graphics/pa...
File Source/platform/graphics/paint/DisplayItemList.cpp (right):

https://codereview.chromium.org/1315993004/diff/1/Source/platform/graphics/pa...
Source/platform/graphics/paint/DisplayItemList.cpp:146:
m_validlyCachedClients.remove(client);
My understanding was that the SkPicture did not need to be re-recorded for paint
offset changes. Am I wrong? Am I right but we're not supporting it yet? Is
something else going on?

[pdr., 2015-08-28 03:31:41]

Updated! PTAL

The big changes in the latest patchset are to move the previous paint offset to
a map on DisplayItemList (temporarily), and address some (online, offline)
reviewer comments.

https://codereview.chromium.org/1315993004/diff/1/Source/core/layout/LayoutOb...
File Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/1315993004/diff/1/Source/core/layout/LayoutOb...
Source/core/layout/LayoutObject.h:1438: mutable LayoutPoint m_cachedPaintOffset;
On 2015/08/27 at 17:20:10, Xianzhu wrote:
> Would m_previousPaintOffset be better, as we don't use it as a cache (to
speedup access), but compare it to the current paint offset to do invalidation?

In the latest patch I've moved the cache to the display item list because we
don't want to increase the size of LayoutObject just yet. This will be
temporary.

I've switched the name as you suggested (m_previousPaintOffsets, etc).

https://codereview.chromium.org/1315993004/diff/1/Source/core/paint/BlockFlow...
File Source/core/paint/BlockFlowPainter.cpp (right):

https://codereview.chromium.org/1315993004/diff/1/Source/core/paint/BlockFlow...
Source/core/paint/BlockFlowPainter.cpp:67: bool skipRecording =
LayoutObjectDrawingRecorder::useCachedDrawingIfPossible(*paintInfo.context,
m_layoutBlockFlow, DisplayItem::SelectionGap, paintOffset);
On 2015/08/27 at 17:20:10, Xianzhu wrote:
> I remember your idea of DrawingParameters when adding
useCachedDrawingIfPossible. Perhaps it's time to use it?

I'm afraid I don't remember what DrawingParameters are. Can you expand on this
idea?

In any case, I did this specific change in a separate patch
(https://src.chromium.org/viewvc/blink?view=rev&revision=201284).

https://codereview.chromium.org/1315993004/diff/1/Source/platform/graphics/pa...
File Source/platform/graphics/paint/DisplayItemList.cpp (right):

https://codereview.chromium.org/1315993004/diff/1/Source/platform/graphics/pa...
Source/platform/graphics/paint/DisplayItemList.cpp:146:
m_validlyCachedClients.remove(client);
On 2015/08/27 at 17:25:41, Stephen Chenney wrote:
> My understanding was that the SkPicture did not need to be re-recorded for
paint offset changes. Am I wrong? Am I right but we're not supporting it yet? Is
something else going on?

You're thinking along the right lines for sure, this is just a tricky case.

We're just going to use the cache to catch invalidations we may have missed,
instead of optimizing the pure-translation case for now. This is because
subpixel snapping throws a wrench in translations: we need to re-record the
drawing display item if the snapping would have changed. I'd like to revisit
pixel-aligned paintOffset changes as an optimization in the future though.

I've added this point to our doc under "considerations":
https://docs.google.com/document/d/1oY8P7a7G4W2LYOvT3VyqUNKOOek6S_e-jD2XjM_7g...

[Xianzhu, 2015-08-28 03:59:00]

https://codereview.chromium.org/1315993004/diff/1/Source/core/paint/BlockFlow...
File Source/core/paint/BlockFlowPainter.cpp (right):

https://codereview.chromium.org/1315993004/diff/1/Source/core/paint/BlockFlow...
Source/core/paint/BlockFlowPainter.cpp:67: bool skipRecording =
LayoutObjectDrawingRecorder::useCachedDrawingIfPossible(*paintInfo.context,
m_layoutBlockFlow, DisplayItem::SelectionGap, paintOffset);
On 2015/08/28 03:31:41, pdr wrote:
> On 2015/08/27 at 17:20:10, Xianzhu wrote:
> > I remember your idea of DrawingParameters when adding
> useCachedDrawingIfPossible. Perhaps it's time to use it?
> 
> I'm afraid I don't remember what DrawingParameters are. Can you expand on this
> idea?
> 
> In any case, I did this specific change in a separate patch
> (https://src.chromium.org/viewvc/blink?view=rev&revision=201284).

It is actually DrawingRecorderCacheParameters which you mentioned in
https://codereview.chromium.org/1220583004/#msg4 :)

[pdr., 2015-08-28 04:34:01]

On 2015/08/28 at 03:59:00, wangxianzhu wrote:
>
https://codereview.chromium.org/1315993004/diff/1/Source/core/paint/BlockFlow...
> File Source/core/paint/BlockFlowPainter.cpp (right):
> 
>
https://codereview.chromium.org/1315993004/diff/1/Source/core/paint/BlockFlow...
> Source/core/paint/BlockFlowPainter.cpp:67: bool skipRecording =
LayoutObjectDrawingRecorder::useCachedDrawingIfPossible(*paintInfo.context,
m_layoutBlockFlow, DisplayItem::SelectionGap, paintOffset);
> On 2015/08/28 03:31:41, pdr wrote:
> > On 2015/08/27 at 17:20:10, Xianzhu wrote:
> > > I remember your idea of DrawingParameters when adding
> > useCachedDrawingIfPossible. Perhaps it's time to use it?
> > 
> > I'm afraid I don't remember what DrawingParameters are. Can you expand on
this
> > idea?
> > 
> > In any case, I did this specific change in a separate patch
> > (https://src.chromium.org/viewvc/blink?view=rev&revision=201284).
> 
> It is actually DrawingRecorderCacheParameters which you mentioned in
https://codereview.chromium.org/1220583004/#msg4 :)

Ahh! I had forgotten about that. I do think it would be more useful now. Mind if
we explore it in a followup?


For other reviewers: the idea is to use a struct to ensure the DrawingRecorders
use the same data for useCachedDrawingIfPossible and the recorder itself. For
example:

This patch:
if (LayoutObjectDrawingRecorder::useCachedDrawingIfPossible(context, client,
PaintPhase::SomePaintPhase, paintOffset)
    return;
LayoutObjectDrawingRecorder recorder(context, client,
PaintPhase::SomePaintPhase, paintOffset, ...);

What if the paint phase or paint offset is changed between the two calls?

Proposal:
DrawingRecorder::CacheParameters recorderCacheParameters(context, client,
PaintPhase::SomePhase, paintOffset);
if
(LayoutObjectDrawingRecorder::useCachedDrawingIfPossible(recorderCacheParameters))
    return;
LayoutObjectDrawingRecorder recorder(recorderCacheParameters, ...);

[Xianzhu, 2015-08-28 16:18:26]

On 2015/08/28 04:34:01, pdr wrote:
> On 2015/08/28 at 03:59:00, wangxianzhu wrote:
> >
>
https://codereview.chromium.org/1315993004/diff/1/Source/core/paint/BlockFlow...
> > File Source/core/paint/BlockFlowPainter.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1315993004/diff/1/Source/core/paint/BlockFlow...
> > Source/core/paint/BlockFlowPainter.cpp:67: bool skipRecording =
> LayoutObjectDrawingRecorder::useCachedDrawingIfPossible(*paintInfo.context,
> m_layoutBlockFlow, DisplayItem::SelectionGap, paintOffset);
> > On 2015/08/28 03:31:41, pdr wrote:
> > > On 2015/08/27 at 17:20:10, Xianzhu wrote:
> > > > I remember your idea of DrawingParameters when adding
> > > useCachedDrawingIfPossible. Perhaps it's time to use it?
> > > 
> > > I'm afraid I don't remember what DrawingParameters are. Can you expand on
> this
> > > idea?
> > > 
> > > In any case, I did this specific change in a separate patch
> > > (https://src.chromium.org/viewvc/blink?view=rev&revision=201284).
> > 
> > It is actually DrawingRecorderCacheParameters which you mentioned in
> https://codereview.chromium.org/1220583004/#msg4 :)
> 
> Ahh! I had forgotten about that. I do think it would be more useful now. Mind
if
> we explore it in a followup?
>

lgtm.

> 
> For other reviewers: the idea is to use a struct to ensure the
DrawingRecorders
> use the same data for useCachedDrawingIfPossible and the recorder itself. For
> example:
> 
> This patch:
> if (LayoutObjectDrawingRecorder::useCachedDrawingIfPossible(context, client,
> PaintPhase::SomePaintPhase, paintOffset)
>     return;
> LayoutObjectDrawingRecorder recorder(context, client,
> PaintPhase::SomePaintPhase, paintOffset, ...);
> 
> What if the paint phase or paint offset is changed between the two calls?
> 
> Proposal:
> DrawingRecorder::CacheParameters recorderCacheParameters(context, client,
> PaintPhase::SomePhase, paintOffset);
> if
>
(LayoutObjectDrawingRecorder::useCachedDrawingIfPossible(recorderCacheParameters))
>     return;
> LayoutObjectDrawingRecorder recorder(recorderCacheParameters, ...);

[chrishtr, 2015-08-28 17:10:46]

chrishtr@chromium.org changed reviewers:
+ chrishtr@chromium.org

[chrishtr, 2015-08-28 17:10:47]

https://codereview.chromium.org/1315993004/diff/80001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItemList.cpp (right):

https://codereview.chromium.org/1315993004/diff/80001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemList.cpp:115: void
DisplayItemList::invalidate(DisplayItemClient client)
Don't you also want to invalidate the paint offset here? This will handle
invalidation types that are not changes of paint offset.

https://codereview.chromium.org/1315993004/diff/80001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemList.cpp:173: bool
DisplayItemList::paintOffsetWasRecorded(DisplayItemClient client, const
LayoutPoint& paintOffset) const
This name doesn't seem quite right. Shouldn't it be paintOffsetIsUnchanged?

[pdr., 2015-08-28 17:37:51]

On 2015/08/28 at 17:10:47, chrishtr wrote:
>
https://codereview.chromium.org/1315993004/diff/80001/Source/platform/graphic...
> File Source/platform/graphics/paint/DisplayItemList.cpp (right):
> 
>
https://codereview.chromium.org/1315993004/diff/80001/Source/platform/graphic...
> Source/platform/graphics/paint/DisplayItemList.cpp:115: void
DisplayItemList::invalidate(DisplayItemClient client)
> Don't you also want to invalidate the paint offset here? This will handle
invalidation types that are not changes of paint offset.
> 
>
https://codereview.chromium.org/1315993004/diff/80001/Source/platform/graphic...
> Source/platform/graphics/paint/DisplayItemList.cpp:173: bool
DisplayItemList::paintOffsetWasRecorded(DisplayItemClient client, const
LayoutPoint& paintOffset) const
> This name doesn't seem quite right. Shouldn't it be paintOffsetIsUnchanged?

Much better name. Done.

[Xianzhu, 2015-08-28 19:25:39]

I think I found a way to make a small change to removes the usage of
m_previousPaintInvalidationRect and m_previousLocation by the end of today. If
you can wait until that CL is landed, you can base this on it and avoid the
temporary complexities.

https://codereview.chromium.org/1315993004/diff/120001/Source/platform/graphi...
File Source/platform/graphics/paint/DisplayItemList.cpp (right):

https://codereview.chromium.org/1315993004/diff/120001/Source/platform/graphi...
Source/platform/graphics/paint/DisplayItemList.cpp:147:
m_validlyCachedClients.remove(client);
You can call invalidate(client) here, and change the second ASSERT in
invalidate() to the following:

    // Paint invalidation may occur before or during painting. If the latter,
    // we should not invalidate any client which has already had display items.
    ASSERT(!m_newDisplayItemIndicesByClient.contains(client));

Then you can remove line 151-155 below.

[Xianzhu, 2015-08-28 21:09:52]

When trying to remove m_previousPositionFromPaintInvalidationBacking dependency
for spv2, I encountered linebox paint offset issue which is because lineboxes
are not LayoutObjects so change of their paint offsets are not checked. This
happens only when the containing block changes location without layout. (If the
line box moves within the containing block there must have been some layout and
invalidation.)

Enclosing descendant drawings into a translation seems to fix the issue (as well
as the issue of repaint-on-location-change of descendants of the containing
block):
<translation>
  drawings of descendants in coordinates of the containing block
</translation>

Perhaps again pixel-snapping is the blocker the above?

I think we can land this CL first before we resolve the above issue.

[pdr., 2015-09-01 22:33:26]

On 2015/08/28 at 19:25:39, wangxianzhu wrote:
> I think I found a way to make a small change to removes the usage of
m_previousPaintInvalidationRect and m_previousLocation by the end of today. If
you can wait until that CL is landed, you can base this on it and avoid the
temporary complexities.
> 
>
https://codereview.chromium.org/1315993004/diff/120001/Source/platform/graphi...
> File Source/platform/graphics/paint/DisplayItemList.cpp (right):
> 
>
https://codereview.chromium.org/1315993004/diff/120001/Source/platform/graphi...
> Source/platform/graphics/paint/DisplayItemList.cpp:147:
m_validlyCachedClients.remove(client);
> You can call invalidate(client) here, and change the second ASSERT in
invalidate() to the following:
> 
>     // Paint invalidation may occur before or during painting. If the latter,
>     // we should not invalidate any client which has already had display
items.
>     ASSERT(!m_newDisplayItemIndicesByClient.contains(client));
> 
> Then you can remove line 151-155 below.

I've hit a few bugs in the subsequence code after making this change. Mind if I
go ahead with this CL and fix this in a followup?

[Xianzhu, 2015-09-01 23:04:09]

On 2015/09/01 22:33:26, pdr wrote:
> On 2015/08/28 at 19:25:39, wangxianzhu wrote:
> > I think I found a way to make a small change to removes the usage of
> m_previousPaintInvalidationRect and m_previousLocation by the end of today. If
> you can wait until that CL is landed, you can base this on it and avoid the
> temporary complexities.
> > 
> >
>
https://codereview.chromium.org/1315993004/diff/120001/Source/platform/graphi...
> > File Source/platform/graphics/paint/DisplayItemList.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1315993004/diff/120001/Source/platform/graphi...
> > Source/platform/graphics/paint/DisplayItemList.cpp:147:
> m_validlyCachedClients.remove(client);
> > You can call invalidate(client) here, and change the second ASSERT in
> invalidate() to the following:
> > 
> >     // Paint invalidation may occur before or during painting. If the
latter,
> >     // we should not invalidate any client which has already had display
> items.
> >     ASSERT(!m_newDisplayItemIndicesByClient.contains(client));
> > 
> > Then you can remove line 151-155 below.
> 
> I've hit a few bugs in the subsequence code after making this change. Mind if
I
> go ahead with this CL and fix this in a followup?

Please go ahead.

[pdr., 2015-09-01 23:05:20]

On 2015/09/01 at 23:04:09, wangxianzhu wrote:
> On 2015/09/01 22:33:26, pdr wrote:
> > On 2015/08/28 at 19:25:39, wangxianzhu wrote:
> > > I think I found a way to make a small change to removes the usage of
> > m_previousPaintInvalidationRect and m_previousLocation by the end of today.
If
> > you can wait until that CL is landed, you can base this on it and avoid the
> > temporary complexities.
> > > 
> > >
> >
https://codereview.chromium.org/1315993004/diff/120001/Source/platform/graphi...
> > > File Source/platform/graphics/paint/DisplayItemList.cpp (right):
> > > 
> > >
> >
https://codereview.chromium.org/1315993004/diff/120001/Source/platform/graphi...
> > > Source/platform/graphics/paint/DisplayItemList.cpp:147:
> > m_validlyCachedClients.remove(client);
> > > You can call invalidate(client) here, and change the second ASSERT in
> > invalidate() to the following:
> > > 
> > >     // Paint invalidation may occur before or during painting. If the
latter,
> > >     // we should not invalidate any client which has already had display
> > items.
> > >     ASSERT(!m_newDisplayItemIndicesByClient.contains(client));
> > > 
> > > Then you can remove line 151-155 below.
> > 
> > I've hit a few bugs in the subsequence code after making this change. Mind
if I
> > go ahead with this CL and fix this in a followup?
> 
> Please go ahead.

Thanks! I'm actively working on the other bug, just want to reduce the rebase
timesink because the code this patch touches keeps moving.

[pdr., 2015-09-01 23:05:22]

The CQ bit was checked by pdr@chromium.org

[pdr., 2015-09-01 23:05:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
wangxianzhu@chromium.org
Link to the patchset: https://codereview.chromium.org/1315993004/#ps160001
(title: "Rebase more (world moved in the past hour)")

[commit-bot: I haz the power, 2015-09-01 23:05:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1315993004/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1315993004/160001

[commit-bot: I haz the power, 2015-09-02 00:58:04]

Committed patchset #9 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201589