cc: Bump up pending tree now tiles order for smoothness mode.

cc: Bump up pending tree now tiles order for smoothness mode.

In smoothness takes priority mode, we should ensure to return tiles
that are required for activation sooner than eventually bin. The reason
for this is that we don't want to starve activation and the prepainting
required for smoothness should already be covered by the soon bin.

Additionally, we have to ensure not to mark these tiles as violating
memory policy, since required for activation tiles need special
treatment (unfortunately). The reason for this is that activation is
currently tied to updating our recordings and smoothness mode would
otherwise starve activation even if we're past the recording region.

BUG=422819
R=reveman

Committed: https://crrev.com/0ad654fa43a6c361c901f027b6c5aa3adf3dfbca
Cr-Commit-Position: refs/heads/master@{#299386}

[vmpstr, 2014-10-13 18:51:52]

PTAL

[reveman, 2014-10-13 19:45:13]

https://codereview.chromium.org/651503004/diff/80001/cc/resources/raster_tile...
File cc/resources/raster_tile_priority_queue.cc (right):

https://codereview.chromium.org/651503004/diff/80001/cc/resources/raster_tile...
cc/resources/raster_tile_priority_queue.cc:82: // activation starvation.
I don't see how this used to starve activation differently than SOON tiles on
active tree would. It seems fine to change the priority in
SMOOTHNESS_TAKES_PRIORITY mode as this patch does here but I don't see it as
some kind of unique situation where activation is starved. This is just
prioritizing the active tree differently, lowers the priority of the active tree
a bit afaict.

Did this used to be prioritized differently? If there's indeed some unique
situation where activation is starved and this is not just a change in priority,
then we should be able to add a test for it.

https://codereview.chromium.org/651503004/diff/80001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (left):

https://codereview.chromium.org/651503004/diff/80001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:529: return true;
can we keep this NOTREACHED statement?

https://codereview.chromium.org/651503004/diff/80001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/651503004/diff/80001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:548: }
Looks like this is just being tacked on as a quick fix. Can we refactor this
function so it more appropriately takes priority on both trees into account?
Maybe take both active_prio and pending_prio into account in the switch
statement above. Either ignoring the tree priority or by adding a second level
switch statement based on it that makes us do whatever is appropriate.

[vmpstr, 2014-10-13 20:15:59]

Patchset #5 (id:160001) has been deleted

[vmpstr, 2014-10-13 20:20:14]

PTAL. I think the new approach is much better. I think it also fixes the
problem, but it will take me a bit of time to test.

https://codereview.chromium.org/651503004/diff/80001/cc/resources/raster_tile...
File cc/resources/raster_tile_priority_queue.cc (right):

https://codereview.chromium.org/651503004/diff/80001/cc/resources/raster_tile...
cc/resources/raster_tile_priority_queue.cc:82: // activation starvation.
On 2014/10/13 19:45:11, reveman wrote:
> I don't see how this used to starve activation differently than SOON tiles on
> active tree would. It seems fine to change the priority in
> SMOOTHNESS_TAKES_PRIORITY mode as this patch does here but I don't see it as
> some kind of unique situation where activation is starved. This is just
> prioritizing the active tree differently, lowers the priority of the active
tree
> a bit afaict.
> 
> Did this used to be prioritized differently? If there's indeed some unique
> situation where activation is starved and this is not just a change in
priority,
> then we should be able to add a test for it. 

In the pre-iterator approach, tiles that are required for activation were always
non-never, which means that once a list of tiles to be considered was
constructed, the activation tiles were simply tacked on at the end (in
smoothness takes priority, of course). Since the iterator approach can't do this
type of a post-process step of tacking things on, we need to be mindful of the
initial order in which the tiles are returned.

What this means in this particular case is that, yes, this is just a change of
priority for tiles that are potentially required for activation. However, in a
prepaint-only memory policy, this also means that we'll actually get to the
required for activation tiles before we abort iteration due to memory policy
violation. In other words, this is the order of tiles in smoothness mode:

active now
---------------- bare minimum*
active soon
required for activation (regardless of active bin)
---------------- allow prepaint*
active eventually
---------------- allow anything*

Bare minimum mode would only cut off the active now tiles. That mode is
currently used for testing only and it's not really appropriate for smoothness
mode (ganesh might use this, but ganesh would also not have a pending tree in
general).

Allow prepaint is pretty common for low mem devices, as I understand it and we
want to ensure that activation tiles are not pruned by this mode.

Without this file's change, but with the rest of the changes in this patch, the
order would like like this:

active now
---------------- bare minimum*
active soon
---------------- allow prepaint*
active eventually
required for activation (regardless of active bin)
---------------- allow anything*

This would fix this for most cases and fix the jank on the bots, but anytime
we're in allow prepaint only mode, we'd run into the same issue.

https://codereview.chromium.org/651503004/diff/80001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (left):

https://codereview.chromium.org/651503004/diff/80001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:529: return true;
On 2014/10/13 19:45:11, reveman wrote:
> can we keep this NOTREACHED statement?

Done.

https://codereview.chromium.org/651503004/diff/80001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/651503004/diff/80001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:548: }
On 2014/10/13 19:45:11, reveman wrote:
> Looks like this is just being tacked on as a quick fix. Can we refactor this
> function so it more appropriately takes priority on both trees into account?
> Maybe take both active_prio and pending_prio into account in the switch
> statement above. Either ignoring the tree priority or by adding a second level
> switch statement based on it that makes us do whatever is appropriate.

Reworked this a bit.

[reveman, 2014-10-13 21:07:36]

lgtm

https://codereview.chromium.org/651503004/diff/80001/cc/resources/raster_tile...
File cc/resources/raster_tile_priority_queue.cc (right):

https://codereview.chromium.org/651503004/diff/80001/cc/resources/raster_tile...
cc/resources/raster_tile_priority_queue.cc:82: // activation starvation.
On 2014/10/13 20:20:13, vmpstr wrote:
> On 2014/10/13 19:45:11, reveman wrote:
> > I don't see how this used to starve activation differently than SOON tiles
on
> > active tree would. It seems fine to change the priority in
> > SMOOTHNESS_TAKES_PRIORITY mode as this patch does here but I don't see it as
> > some kind of unique situation where activation is starved. This is just
> > prioritizing the active tree differently, lowers the priority of the active
> tree
> > a bit afaict.
> > 
> > Did this used to be prioritized differently? If there's indeed some unique
> > situation where activation is starved and this is not just a change in
> priority,
> > then we should be able to add a test for it. 
> 
> In the pre-iterator approach, tiles that are required for activation were
always
> non-never, which means that once a list of tiles to be considered was
> constructed, the activation tiles were simply tacked on at the end (in
> smoothness takes priority, of course). Since the iterator approach can't do
this
> type of a post-process step of tacking things on, we need to be mindful of the
> initial order in which the tiles are returned.
> 
> What this means in this particular case is that, yes, this is just a change of
> priority for tiles that are potentially required for activation. However, in a
> prepaint-only memory policy, this also means that we'll actually get to the
> required for activation tiles before we abort iteration due to memory policy
> violation. In other words, this is the order of tiles in smoothness mode:
> 
> active now
> ---------------- bare minimum*
> active soon
> required for activation (regardless of active bin)
> ---------------- allow prepaint*
> active eventually
> ---------------- allow anything*
> 
> Bare minimum mode would only cut off the active now tiles. That mode is
> currently used for testing only and it's not really appropriate for smoothness
> mode (ganesh might use this, but ganesh would also not have a pending tree in
> general).
> 
> Allow prepaint is pretty common for low mem devices, as I understand it and we
> want to ensure that activation tiles are not pruned by this mode.
> 
> Without this file's change, but with the rest of the changes in this patch,
the
> order would like like this:
> 
> active now
> ---------------- bare minimum*
> active soon
> ---------------- allow prepaint*
> active eventually
> required for activation (regardless of active bin)
> ---------------- allow anything*
> 
> This would fix this for most cases and fix the jank on the bots, but anytime
> we're in allow prepaint only mode, we'd run into the same issue.

The change looks fine to me. It's just the comment that I'm not a 100% sure
about. Instead of "activation starvation" can it say something along the lines
of "process the pending tree to allow tiles required for activation to be
initialized when memory policy only allows prepaint"?

[vmpstr, 2014-10-13 21:33:40]

https://codereview.chromium.org/651503004/diff/80001/cc/resources/raster_tile...
File cc/resources/raster_tile_priority_queue.cc (right):

https://codereview.chromium.org/651503004/diff/80001/cc/resources/raster_tile...
cc/resources/raster_tile_priority_queue.cc:82: // activation starvation.
On 2014/10/13 21:07:36, reveman wrote:
> On 2014/10/13 20:20:13, vmpstr wrote:
> > On 2014/10/13 19:45:11, reveman wrote:
> > > I don't see how this used to starve activation differently than SOON tiles
> on
> > > active tree would. It seems fine to change the priority in
> > > SMOOTHNESS_TAKES_PRIORITY mode as this patch does here but I don't see it
as
> > > some kind of unique situation where activation is starved. This is just
> > > prioritizing the active tree differently, lowers the priority of the
active
> > tree
> > > a bit afaict.
> > > 
> > > Did this used to be prioritized differently? If there's indeed some unique
> > > situation where activation is starved and this is not just a change in
> > priority,
> > > then we should be able to add a test for it. 
> > 
> > In the pre-iterator approach, tiles that are required for activation were
> always
> > non-never, which means that once a list of tiles to be considered was
> > constructed, the activation tiles were simply tacked on at the end (in
> > smoothness takes priority, of course). Since the iterator approach can't do
> this
> > type of a post-process step of tacking things on, we need to be mindful of
the
> > initial order in which the tiles are returned.
> > 
> > What this means in this particular case is that, yes, this is just a change
of
> > priority for tiles that are potentially required for activation. However, in
a
> > prepaint-only memory policy, this also means that we'll actually get to the
> > required for activation tiles before we abort iteration due to memory policy
> > violation. In other words, this is the order of tiles in smoothness mode:
> > 
> > active now
> > ---------------- bare minimum*
> > active soon
> > required for activation (regardless of active bin)
> > ---------------- allow prepaint*
> > active eventually
> > ---------------- allow anything*
> > 
> > Bare minimum mode would only cut off the active now tiles. That mode is
> > currently used for testing only and it's not really appropriate for
smoothness
> > mode (ganesh might use this, but ganesh would also not have a pending tree
in
> > general).
> > 
> > Allow prepaint is pretty common for low mem devices, as I understand it and
we
> > want to ensure that activation tiles are not pruned by this mode.
> > 
> > Without this file's change, but with the rest of the changes in this patch,
> the
> > order would like like this:
> > 
> > active now
> > ---------------- bare minimum*
> > active soon
> > ---------------- allow prepaint*
> > active eventually
> > required for activation (regardless of active bin)
> > ---------------- allow anything*
> > 
> > This would fix this for most cases and fix the jank on the bots, but anytime
> > we're in allow prepaint only mode, we'd run into the same issue.
> 
> The change looks fine to me. It's just the comment that I'm not a 100% sure
> about. Instead of "activation starvation" can it say something along the lines
> of "process the pending tree to allow tiles required for activation to be
> initialized when memory policy only allows prepaint"?

Done.

[vmpstr, 2014-10-13 21:33:59]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2014-10-13 21:35:29]

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

[commit-bot: I haz the power, 2014-10-13 23:03:56]

Committed patchset #6 (id:260001)

[commit-bot: I haz the power, 2014-10-13 23:05:49]

Patchset 6 (id:??) landed as
https://crrev.com/0ad654fa43a6c361c901f027b6c5aa3adf3dfbca
Cr-Commit-Position: refs/heads/master@{#299386}