cc: Separate raster and decode prepaint regions.

cc: Separate raster and decode prepaint regions.

This patch separates raster and decode distances to be
independent of each other. Specifically, any tile in prepaint
that is further than 1000 screen space pixels from the
viewport will not be rasterized. Instead, we will pull out
the images on those tiles and predecode those instead.

R=enne, ericrk
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/70a29c1298e666c465a8e75530575eda1aeaa37c
Cr-Commit-Position: refs/heads/master@{#398677}

[vmpstr, 2016-05-24 20:22:07]

Description was changed from

==========
[WIP] cc: Separate raster and decode prepaint regions.

BUG=
==========

to

==========
[WIP] cc: Separate raster and decode prepaint regions.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[vmpstr, 2016-05-24 21:30:46]

vmpstr@chromium.org changed reviewers:
+ enne@chromium.org, ericrk@chromium.org

[vmpstr, 2016-05-24 21:30:46]

Please take a look. Preliminary patch.

[ericrk, 2016-05-24 23:42:34]

https://codereview.chromium.org/2007163002/diff/20001/cc/tiles/tile_manager.cc
File cc/tiles/tile_manager.cc (right):

https://codereview.chromium.org/2007163002/diff/20001/cc/tiles/tile_manager.c...
cc/tiles/tile_manager.cc:651: if (!tile->required_for_activation() &&
!tile->required_for_draw() &&
Just to make sure - low resolution tiles are not required for activation/draw,
so they may be ignored if they're outside the preraster distance. I'd assume
this is fine, but given that you had to check for activation/draw, I wonder if
there are some cases where this might cause problems?

Also, where do we skip images for low resolution tiles - probably fine, but want
to make sure we skip them in the predecode path as well.

https://codereview.chromium.org/2007163002/diff/20001/cc/trees/layer_tree_set...
File cc/trees/layer_tree_settings.cc (right):

https://codereview.chromium.org/2007163002/diff/20001/cc/trees/layer_tree_set...
cc/trees/layer_tree_settings.cc:113:
max_preraster_distance_in_screen_pixels(1000),
I'm curious if we should use an even lower limit for GPU? With the skewport
target time set to 0.2s for GPU, you'd have to scroll at a rate of 5000 pixels
per second to hit this - additionally, GPU should be able to raster more quickly
than SW, especially if no image decodes are necessary.

[enne (OOO), 2016-05-25 00:35:41]

I think I would be more convinced about what to do here with some data in terms
of how much raster vs decode we need.  I think the general approach of doing
what we normally do, but switching some amount raster to decode seems good to
me.

Maybe we could decrease the predecode size too, although my intuition is that
images far offscreen will still be valid (and we already prevent animated gifs
from updating in Blink).

Do you think there could be some equivalent UMA stats here, similar to the "how
often are prerastered tiles actually used" metric?

[vmpstr, 2016-06-07 20:43:02]

Description was changed from

==========
[WIP] cc: Separate raster and decode prepaint regions.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Separate raster and decode prepaint regions.

This patch separates raster and decode distances to be
independent of each other. Specifically, any tile in prepaint
that is further than 1000 screen space pixels from the
viewport will not be rasterized. Instead, we will pull out
the images on those tiles and predecode those instead.

R=enne, ericrk
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[vmpstr, 2016-06-07 20:43:55]

We've added a bunch of UMA stats that I think capture most things that we want
to measure with this patch. I'm going to write up a one-pager to describe the
current state of those stats. However, other than that I think this patch is
ready for review. Please take a look.

https://codereview.chromium.org/2007163002/diff/20001/cc/tiles/tile_manager.cc
File cc/tiles/tile_manager.cc (right):

https://codereview.chromium.org/2007163002/diff/20001/cc/tiles/tile_manager.c...
cc/tiles/tile_manager.cc:651: if (!tile->required_for_activation() &&
!tile->required_for_draw() &&
On 2016/05/24 23:42:34, ericrk wrote:
> Just to make sure - low resolution tiles are not required for activation/draw,
> so they may be ignored if they're outside the preraster distance. I'd assume
> this is fine, but given that you had to check for activation/draw, I wonder if
> there are some cases where this might cause problems?
> 
> Also, where do we skip images for low resolution tiles - probably fine, but
want
> to make sure we skip them in the predecode path as well.

Yeah, LOW_RESOLUTION tiles are neither required for draw or activation. The if
check ensures that if it _is_ required then it is rasterized always. So, low
resolution tiles would be skipped by this same check. I can add explicit code
not to add those tiles into tiles_to_process_for_images if you'd like. I don't
think it matters too much and low res tiles also don't have images enabled
anyway.

https://codereview.chromium.org/2007163002/diff/20001/cc/trees/layer_tree_set...
File cc/trees/layer_tree_settings.cc (right):

https://codereview.chromium.org/2007163002/diff/20001/cc/trees/layer_tree_set...
cc/trees/layer_tree_settings.cc:113:
max_preraster_distance_in_screen_pixels(1000),
On 2016/05/24 23:42:34, ericrk wrote:
> I'm curious if we should use an even lower limit for GPU? With the skewport
> target time set to 0.2s for GPU, you'd have to scroll at a rate of 5000 pixels
> per second to hit this - additionally, GPU should be able to raster more
quickly
> than SW, especially if no image decodes are necessary.

That's entirely possible. Eventually I'd like this to be close to 0, however I'm
trying to deliberately be a bit conservative so that we don't break things if
our assumptions about imageless raster speed are wrong. Do you think this is a
good value to start with?

[vmpstr, 2016-06-07 22:45:08]

On 2016/06/07 20:43:55, vmpstr wrote:
> I'm going to write up a one-pager to describe the
> current state of those stats.

Here it is
https://docs.google.com/document/d/1GEZhzzVTmCiuIUEOdOtg5sWwKE_BLKVRpx1Lnz53D...

[enne (OOO), 2016-06-07 22:55:37]

Thanks for the analysis.  lgtm

I am shocked that 56% of prepaint tiles are wasted work.  I knew it probably
wasn't great, but didn't think it was going to be that bad.  T_T

[ericrk, 2016-06-08 00:24:50]

On 2016/06/07 22:55:37, enne wrote:
> Thanks for the analysis.  lgtm
> 
> I am shocked that 56% of prepaint tiles are wasted work.  I knew it probably
> wasn't great, but didn't think it was going to be that bad.  T_T

lgtm - one question - could pinning these additional images for longer duration
force us into constant at-raster mode for any invalidations of on-screen
content? Do we have an UMA stat we can monitor for the % of images which are
decoded at-raster?

[vmpstr, 2016-06-08 00:27:13]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2016-06-08 00:27:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2007163002/40001

[vmpstr, 2016-06-08 00:28:24]

On 2016/06/08 00:24:50, ericrk wrote:
> On 2016/06/07 22:55:37, enne wrote:
> > Thanks for the analysis.  lgtm
> > 
> > I am shocked that 56% of prepaint tiles are wasted work.  I knew it probably
> > wasn't great, but didn't think it was going to be that bad.  T_T
> 
> lgtm - one question - could pinning these additional images for longer
duration
> force us into constant at-raster mode for any invalidations of on-screen
> content? Do we have an UMA stat we can monitor for the % of images which are
> decoded at-raster?

It's possible. However, it shouldn't happen since we'd be scheduling now tiles
first. That means we would be pinning images in the now bit before pinning off
screen images.

[commit-bot: I haz the power, 2016-06-08 01:49:29]

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

[commit-bot: I haz the power, 2016-06-08 01:49:30]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[vmpstr, 2016-06-08 20:28:08]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2016-06-08 20:28:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2007163002/40001

[commit-bot: I haz the power, 2016-06-08 21:17:34]

Description was changed from

==========
cc: Separate raster and decode prepaint regions.

This patch separates raster and decode distances to be
independent of each other. Specifically, any tile in prepaint
that is further than 1000 screen space pixels from the
viewport will not be rasterized. Instead, we will pull out
the images on those tiles and predecode those instead.

R=enne, ericrk
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Separate raster and decode prepaint regions.

This patch separates raster and decode distances to be
independent of each other. Specifically, any tile in prepaint
that is further than 1000 screen space pixels from the
viewport will not be rasterized. Instead, we will pull out
the images on those tiles and predecode those instead.

R=enne, ericrk
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-06-08 21:17:35]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-08 21:21:14]

Description was changed from

==========
cc: Separate raster and decode prepaint regions.

This patch separates raster and decode distances to be
independent of each other. Specifically, any tile in prepaint
that is further than 1000 screen space pixels from the
viewport will not be rasterized. Instead, we will pull out
the images on those tiles and predecode those instead.

R=enne, ericrk
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Separate raster and decode prepaint regions.

This patch separates raster and decode distances to be
independent of each other. Specifically, any tile in prepaint
that is further than 1000 screen space pixels from the
viewport will not be rasterized. Instead, we will pull out
the images on those tiles and predecode those instead.

R=enne, ericrk
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/70a29c1298e666c465a8e75530575eda1aeaa37c
Cr-Commit-Position: refs/heads/master@{#398677}
==========

[commit-bot: I haz the power, 2016-06-08 21:21:15]

Patchset 3 (id:??) landed as
https://crrev.com/70a29c1298e666c465a8e75530575eda1aeaa37c
Cr-Commit-Position: refs/heads/master@{#398677}

[Primiano Tucci (use gerrit), 2016-06-29 17:21:28]

On 2016/06/08 21:21:15, commit-bot: I haz the power wrote:
> Patchset 3 (id:??) landed as
> https://crrev.com/70a29c1298e666c465a8e75530575eda1aeaa37c
> Cr-Commit-Position: refs/heads/master@{#398677}

Howdy, I think this CL caused big improvements on the system health benchmark
(see crbug.com/624467) (which is great)
Only one question: is it expected that, while GPU PSS memory went down (together
with the cc and gpu categories) the renderer process memory went up (even if
less than the total)?
more details in  crbug.com/624467, can somebody take a look there plz?