cc: Switch tiling set eviction queue to consider combined priority.

cc: Switch tiling set eviction queue to consider combined priority.

Tile manager and eviction tile priority queues are using combined
priorities to schedule tiles, so tiling set eviction queues should use
those same priorities in order to return tiles in expected order.

R=vmiura, danakj, vmpstr

Committed: https://crrev.com/359e73882ed83a773dc06a9457411975ca589d82
Cr-Commit-Position: refs/heads/master@{#324101}

[USE eero AT chromium.org, 2015-03-31 17:35:45]

PTAL.

This aligns tiling set eviction queue with
https://codereview.chromium.org/924613002

[vmpstr, 2015-04-02 17:30:29]

This look fine overall. Would you say that in order to achieve the same result
as this patch, but starting with this:
https://codereview.chromium.org/1038793002/

all that really needs to change is remove tree_priority and change
IsSharedOutOfOrderTile?

I don't mind landing this first, but my patch will need to land as well, hence
my question.

https://codereview.chromium.org/1051473002/diff/1/cc/resources/eviction_tile_...
File cc/resources/eviction_tile_priority_queue.cc (left):

https://codereview.chromium.org/1051473002/diff/1/cc/resources/eviction_tile_...
cc/resources/eviction_tile_priority_queue.cc:93: ++it) {
nit: keep the braces, since the for loop is two lines. Or change the for loop to
be range based for with const auto&

https://codereview.chromium.org/1051473002/diff/1/cc/resources/tiling_set_evi...
File cc/resources/tiling_set_eviction_queue.cc (left):

https://codereview.chromium.org/1051473002/diff/1/cc/resources/tiling_set_evi...
cc/resources/tiling_set_eviction_queue.cc:155: if
(tile->required_for_activation() != required_for_activation)
Is there a reason why switched these conditions?

https://codereview.chromium.org/1051473002/diff/1/cc/resources/tiling_set_evi...
File cc/resources/tiling_set_eviction_queue.cc (right):

https://codereview.chromium.org/1051473002/diff/1/cc/resources/tiling_set_evi...
cc/resources/tiling_set_eviction_queue.cc:97: if
(tile->required_for_activation() != required_for_activation)
Would this case be skipped by IsSharedOutOfOrderTile? We're in a spiral
iterator, which means the bin on this tree is at most SOON, and required for
activation tiles come from NOW bin.

[USE eero AT chromium.org, 2015-04-07 13:17:07]

https://codereview.chromium.org/1051473002/diff/1/cc/resources/eviction_tile_...
File cc/resources/eviction_tile_priority_queue.cc (left):

https://codereview.chromium.org/1051473002/diff/1/cc/resources/eviction_tile_...
cc/resources/eviction_tile_priority_queue.cc:93: ++it) {
On 2015/04/02 17:30:29, vmpstr wrote:
> nit: keep the braces, since the for loop is two lines. Or change the for loop
to
> be range based for with const auto&

Done.

https://codereview.chromium.org/1051473002/diff/1/cc/resources/tiling_set_evi...
File cc/resources/tiling_set_eviction_queue.cc (left):

https://codereview.chromium.org/1051473002/diff/1/cc/resources/tiling_set_evi...
cc/resources/tiling_set_eviction_queue.cc:155: if
(tile->required_for_activation() != required_for_activation)
On 2015/04/02 17:30:29, vmpstr wrote:
> Is there a reason why switched these conditions?

For optimisation. Testing rfa status is much faster check than testing ooo
status.

But because this code is going away anyway, I reverted the change. Let's
optimise for patch size instead of for performance.

https://codereview.chromium.org/1051473002/diff/1/cc/resources/tiling_set_evi...
File cc/resources/tiling_set_eviction_queue.cc (right):

https://codereview.chromium.org/1051473002/diff/1/cc/resources/tiling_set_evi...
cc/resources/tiling_set_eviction_queue.cc:97: if
(tile->required_for_activation() != required_for_activation)
On 2015/04/02 17:30:29, vmpstr wrote:
> Would this case be skipped by IsSharedOutOfOrderTile? We're in a spiral
> iterator, which means the bin on this tree is at most SOON, and required for
> activation tiles come from NOW bin.

Yes, I replaced it with DCHECK. I also reverted the test order change.

[USE eero AT chromium.org, 2015-04-07 13:30:15]

On 2015/04/02 17:30:29, vmpstr wrote:
> Would you say that in order to achieve the same result
> as this patch, but starting with this:
> https://codereview.chromium.org/1038793002/
> all that really needs to change is remove tree_priority and change
> IsSharedOutOfOrderTile?

Yes, if this were to be done on top of
https://codereview.chromium.org/1038793002/, that would be all what is needed
(provided that tree_priority removals include unit test changes).

And if you rebase https://codereview.chromium.org/1038793002/ on top of this,
https://codereview.chromium.org/1038793002/ should not need to modify
IsSharedOutOfOrderTile, any more.

[vmpstr, 2015-04-07 19:15:03]

On 2015/04/07 13:30:15, e_hakkinen wrote:
> On 2015/04/02 17:30:29, vmpstr wrote:
> > Would you say that in order to achieve the same result
> > as this patch, but starting with this:
> > https://codereview.chromium.org/1038793002/
> > all that really needs to change is remove tree_priority and change
> > IsSharedOutOfOrderTile?
> 
> Yes, if this were to be done on top of
> https://codereview.chromium.org/1038793002/, that would be all what is needed
> (provided that tree_priority removals include unit test changes).
> 
> And if you rebase https://codereview.chromium.org/1038793002/ on top of this,
> https://codereview.chromium.org/1038793002/ should not need to modify
> IsSharedOutOfOrderTile, any more.

Ok that sounds good. This lgtm and I'll rebase on top of it.

[USE eero AT chromium.org, 2015-04-07 19:18:23]

The CQ bit was checked by e.hakkinen@samsung.com

[commit-bot: I haz the power, 2015-04-07 19:19:15]

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

[commit-bot: I haz the power, 2015-04-07 20:09:59]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-04-07 20:11:31]

Patchset 2 (id:??) landed as
https://crrev.com/359e73882ed83a773dc06a9457411975ca589d82
Cr-Commit-Position: refs/heads/master@{#324101}