cc: Expand invalidation to full tile when recording is missing for the tile.

cc: Expand invalidation to full tile when recording is missing for the tile.

Currently we do a walk over all tiles in the pending tree's pile, but
there can be 1 million x 1 million tiles on some pages, which makes this
method take multiple _seconds_ to complete.

These loops were added in r184525 which gave the tradeoffs that led to
them, which are that we want to only use a single pile for all the tiles
on this layer, but we don't want raster tiles on the active layer (when
this activates) that can't be rastered by the pile they are attached to.

Instead of walking every pile-tile, to find the tiles that are not
present in the current recording and invalidate them, we expand
invalidations outside the interest rect to cover the full recording tiles,
and we expand invalidation inside the interest rect to include any raster
tiles that don't have a recording (such as in the offscreen animating gif
case). We give this expanded invalidation to the pending layer, causing
it to drop rastered tiles from the active tree that intersect with an
unrecorded area.

R=enne
BUG=371839
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277964

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278196

[danakj, 2014-05-22 20:20:33]

enne's out so PTAL david/vlad.

[enne (OOO), 2014-05-27 21:09:22]

https://codereview.chromium.org/294163009/diff/1/cc/layers/picture_layer_impl.cc
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/294163009/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:691:
invalidation_.Union(outside_recorded_viewport);
There's a performance bug here.  As the comment on the member variable suggests,
recorded viewport is empty if it's not valid.

If something declines to be recorded (animated gif offscreen but within the
recorded viewport), then recorded viewport will be set to empty.  If recorded
viewport is empty then you union the entire pile into invalidation.  So
offscreen animated gifs => reraster the world.

Should dropping tiles / adding invalidations outside the recorded viewport be
the job of picture layer and picture pile, who might be able to access the
desired recorded viewport vs. the actual pessimistic one which may be empty for
edge cases?

[danakj, 2014-06-13 21:08:35]

Ok this does the invalidation expansion to full recording-tiles and everything
outside the recording viewport in PicturePile and passes that as the
invalidation to PictureLayerImpl

PTAL

[enne (OOO), 2014-06-13 21:16:44]

https://codereview.chromium.org/294163009/diff/40001/cc/resources/picture_pil...
File cc/resources/picture_pile.cc (right):

https://codereview.chromium.org/294163009/diff/40001/cc/resources/picture_pil...
cc/resources/picture_pile.cc:175:
invalidation_across_frames.Subtract(interest_rect);
The one bad part about this is that if you have a single long page that fits
into memory and has no invalidations then we'll have to reraster tiles we could
have kept around.  Do you actually need this outside invalidation here?  I think
the code below that invalidates tiles will do the same thing?

[danakj, 2014-06-13 21:19:18]

https://codereview.chromium.org/294163009/diff/40001/cc/resources/picture_pil...
File cc/resources/picture_pile.cc (right):

https://codereview.chromium.org/294163009/diff/40001/cc/resources/picture_pil...
cc/resources/picture_pile.cc:175:
invalidation_across_frames.Subtract(interest_rect);
On 2014/06/13 21:16:44, enne wrote:
> The one bad part about this is that if you have a single long page that fits
> into memory and has no invalidations then we'll have to reraster tiles we
could
> have kept around.  Do you actually need this outside invalidation here?  I
think
> the code below that invalidates tiles will do the same thing?

our goal is to invalidate all tiles on the pending layer that we don't have a
recording for. anything outside of this rect may not have a recording for it.

the code below does not do the same, it just invalidates things touched by the
invalidation rect which could be empty and then it would do nothing.

i think?

alternatively we need to know what we have recordings for outside the interest
rect, which we could compute but that's as expensive (and the same thing) as
what we were doing in PictureLayerImpl before, unless we save this data
somewhere?

[enne (OOO), 2014-06-13 21:23:20]

https://codereview.chromium.org/294163009/diff/40001/cc/resources/picture_pil...
File cc/resources/picture_pile.cc (right):

https://codereview.chromium.org/294163009/diff/40001/cc/resources/picture_pil...
cc/resources/picture_pile.cc:175:
invalidation_across_frames.Subtract(interest_rect);
On 2014/06/13 21:19:18, danakj wrote:
> On 2014/06/13 21:16:44, enne wrote:
> > The one bad part about this is that if you have a single long page that fits
> > into memory and has no invalidations then we'll have to reraster tiles we
> could
> > have kept around.  Do you actually need this outside invalidation here?  I
> think
> > the code below that invalidates tiles will do the same thing?
> 
> our goal is to invalidate all tiles on the pending layer that we don't have a
> recording for. anything outside of this rect may not have a recording for it.
> 
> the code below does not do the same, it just invalidates things touched by the
> invalidation rect which could be empty and then it would do nothing.
> 
> i think?
> 
> alternatively we need to know what we have recordings for outside the interest
> rect, which we could compute but that's as expensive (and the same thing) as
> what we were doing in PictureLayerImpl before, unless we save this data
> somewhere?

The only recordings that are dropped are ones that have been invalidated, so
ones that are external to recorded_viewport are still there.  recorded_viewport
is just an optimization to quickly early out of a few hash map lookups to see if
tiles exist.  There may still be valid tiles outside of it.

[danakj, 2014-06-13 21:45:53]

On Fri, Jun 13, 2014 at 5:23 PM, <enne@chromium.org> wrote:

>
> https://codereview.chromium.org/294163009/diff/40001/cc/
> resources/picture_pile.cc
> File cc/resources/picture_pile.cc (right):
>
> https://codereview.chromium.org/294163009/diff/40001/cc/
> resources/picture_pile.cc#newcode175
> cc/resources/picture_pile.cc:175:
> invalidation_across_frames.Subtract(interest_rect);
> On 2014/06/13 21:19:18, danakj wrote:
>
>> On 2014/06/13 21:16:44, enne wrote:
>> > The one bad part about this is that if you have a single long page
>>
> that fits
>
>> > into memory and has no invalidations then we'll have to reraster
>>
> tiles we
>
>> could
>> > have kept around.  Do you actually need this outside invalidation
>>
> here?  I
>
>> think
>> > the code below that invalidates tiles will do the same thing?
>>
>
>  our goal is to invalidate all tiles on the pending layer that we don't
>>
> have a
>
>> recording for. anything outside of this rect may not have a recording
>>
> for it.
>
>  the code below does not do the same, it just invalidates things
>>
> touched by the
>
>> invalidation rect which could be empty and then it would do nothing.
>>
>
>  i think?
>>
>
>  alternatively we need to know what we have recordings for outside the
>>
> interest
>
>> rect, which we could compute but that's as expensive (and the same
>>
> thing) as
>
>> what we were doing in PictureLayerImpl before, unless we save this
>>
> data
>
>> somewhere?
>>
>
> The only recordings that are dropped are ones that have been
> invalidated, so ones that are external to recorded_viewport are still
> there.  recorded_viewport is just an optimization to quickly early out
> of a few hash map lookups to see if tiles exist.  There may still be
> valid tiles outside of it.
>

Right, but *may* is the crux of the performance issue this is trying to
solve. We were checking every tile outside the recorded viewport to see if
it had a recording, and this was taking like 10 seconds to do. So.. we need
to do something dumber. If we don't just drop everything outside the
recording viewport, what else do you suggest?

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

[danakj, 2014-06-13 22:06:56]

Ok fixed up, InvalidateOnTileBoundaryInflated should cover this stuff already so
no new test. PTAL

[enne (OOO), 2014-06-13 22:26:42]

https://codereview.chromium.org/294163009/diff/60001/cc/resources/picture_pil...
File cc/resources/picture_pile.cc (right):

https://codereview.chromium.org/294163009/diff/60001/cc/resources/picture_pil...
cc/resources/picture_pile.cc:200:
gfx::UnionRects(tiling_.TileBoundsWithBorder(left_tile, top_tile),
I think
invalidation_expanded_to_recording_tiles.Union(ExpandRectToTileBoundsWithBorders(invalid_rect))
will do what you're doing here.

However, the previous code didn't include borders, so I'm wondering if this will
overinvalidate.  I think this should just be TileBounds.

Here's the example I'm thinking of.  Say you have three recording tiles |A|B|C|.
 There's an invalidation that's on the edge of C, but B's border overlaps.  So
this code here will say invalid_rect_filling_tiles = union(B,C).  If you include
B and C's border in that rect, then on the compositor thread when we toss tiles
that intersect invalidations we will also toss tiles that could be entirely
rastered by A because they are hit by B's borders.

[enne (OOO), 2014-06-13 22:34:13]

https://codereview.chromium.org/294163009/diff/60001/cc/resources/picture_pil...
File cc/resources/picture_pile.cc (right):

https://codereview.chromium.org/294163009/diff/60001/cc/resources/picture_pil...
cc/resources/picture_pile.cc:200:
gfx::UnionRects(tiling_.TileBoundsWithBorder(left_tile, top_tile),
Actually, more importantly, you should only inflate rects for tiles that are not
rerecorded.  So, this logic needs to move into the for loop below.

[danakj, 2014-06-16 20:43:41]

PTAL

[enne (OOO), 2014-06-17 00:02:54]

https://codereview.chromium.org/294163009/diff/100001/cc/base/tiling_data.cc
File cc/base/tiling_data.cc (right):

https://codereview.chromium.org/294163009/diff/100001/cc/base/tiling_data.cc#...
cc/base/tiling_data.cc:142: const gfx::Rect& rect) const {
OOPS.  Thanks!

https://codereview.chromium.org/294163009/diff/100001/cc/base/tiling_data.cc#...
cc/base/tiling_data.cc:161: int index_right =
LastBorderTileXIndexFromSrcCoord(rect.right());
What happened to your "I need a -1 here" thought?

https://codereview.chromium.org/294163009/diff/100001/cc/resources/picture_pi...
File cc/resources/picture_pile.cc (right):

https://codereview.chromium.org/294163009/diff/100001/cc/resources/picture_pi...
cc/resources/picture_pile.cc:199: SubtractRegions(*invalidation,
interest_rect_over_tiles);
Region ops like this make me sad, but this is probably the right thing to do in
the abstract.  Maybe in the future if we write a "intersect these two rects and
return 1-2 rects" function we can optimize this.

https://codereview.chromium.org/294163009/diff/100001/cc/resources/picture_pi...
File cc/resources/picture_pile_base.h (right):

https://codereview.chromium.org/294163009/diff/100001/cc/resources/picture_pi...
cc/resources/picture_pile_base.h:36: // If non-empty, all pictures tiles inside
this rect are recorded.
Can you also maybe say that there may be recorded tiles outside this rect (but
no non-recorded tiles inside)? I think that detail is non-obvious and maybe
could have been better commented in the first place.

[danakj, 2014-06-17 18:51:34]

PTAL

https://codereview.chromium.org/294163009/diff/100001/cc/base/tiling_data.cc
File cc/base/tiling_data.cc (right):

https://codereview.chromium.org/294163009/diff/100001/cc/base/tiling_data.cc#...
cc/base/tiling_data.cc:161: int index_right =
LastBorderTileXIndexFromSrcCoord(rect.right());
On 2014/06/17 00:02:54, enne wrote:
> What happened to your "I need a -1 here" thought?

Was going to fix both in a separate CL, but I've got a test now, so I've
included it.

https://codereview.chromium.org/294163009/diff/100001/cc/resources/picture_pi...
File cc/resources/picture_pile.cc (right):

https://codereview.chromium.org/294163009/diff/100001/cc/resources/picture_pi...
cc/resources/picture_pile.cc:199: SubtractRegions(*invalidation,
interest_rect_over_tiles);
On 2014/06/17 00:02:54, enne wrote:
> Region ops like this make me sad, but this is probably the right thing to do
in
> the abstract.  Maybe in the future if we write a "intersect these two rects
and
> return 1-2 rects" function we can optimize this.

For now I used Rect::Subtract instead, which may invalidate stuff inside the
interest rect at the corners of it.

https://codereview.chromium.org/294163009/diff/100001/cc/resources/picture_pi...
File cc/resources/picture_pile_base.h (right):

https://codereview.chromium.org/294163009/diff/100001/cc/resources/picture_pi...
cc/resources/picture_pile_base.h:36: // If non-empty, all pictures tiles inside
this rect are recorded.
On 2014/06/17 00:02:54, enne wrote:
> Can you also maybe say that there may be recorded tiles outside this rect (but
> no non-recorded tiles inside)? I think that detail is non-obvious and maybe
> could have been better commented in the first place.

Done.

[danakj, 2014-06-17 18:54:13]

Updated the patch description.

[enne (OOO), 2014-06-17 19:14:01]

lgtm

[danakj, 2014-06-17 21:55:47]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-06-17 21:56:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/danakj@chromium.org/294163009/120001

[commit-bot: I haz the power, 2014-06-18 07:12:40]

Change committed as 277964

[danakj, 2014-06-18 16:32:26]

The CQ bit was checked by danakj@chromium.org

[danakj, 2014-06-18 16:33:06]

This was reverted because I changed the |invalidation| Region while iterating on
it which caused a UAF. Woops. Now with better memory access.

[commit-bot: I haz the power, 2014-06-18 16:33:30]

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

[commit-bot: I haz the power, 2014-06-18 23:48:20]

Change committed as 278196