Ensure deterministic heap prioritization of raster task nodes

Ensure deterministic heap prioritization of raster task nodes

Previously, task nodes were all fed the same priority. While certain
heap implementations ensured a consistent prioritization given the same
task priority and input ordering, this is not guaranteed by the spec.
In particular, Android's libc++ implementation appears to differ in this
respect, resulting in tasks from different sets being processed in an
order different from node creation order.

Avoid this by including the task set type in the node priority. All
conforming heap implementations should now yield the same ordering
with respect to task set type.

BUG=427718
Committed: https://crrev.com/d2f00426f585653d2c8d6e16081c3a53538d4e1e
Cr-Commit-Position: refs/heads/master@{#313972}

[jdduke (slow), 2015-01-30 17:35:05]

jdduke@chromium.org changed reviewers:
+ vmpstr@chromium.org

[jdduke (slow), 2015-01-30 17:35:06]

vmpstr@: PTAL, thanks (see https://codereview.chromium.org/835633003/ for more
context).

[vmpstr, 2015-01-30 18:22:28]

On 2015/01/30 17:35:06, jdduke wrote:
> vmpstr@: PTAL, thanks (see https://codereview.chromium.org/835633003/ for more
> context).

This would violate some conditions, since kTaskSetFinishedTaskPriority +
max(task_set) >= kTileTaskPriorityBase. I think the patch is the correct fix,
but can you update kTileTaskPriorityBase to be something > 2 (maybe like 10u),
rename kTaskSetFinishedTaskPriority to kTaskSetFinishedTaskPriorityBase and
update the comment near there explaining what's going on? :) (this is all in
tile_task_worker_pool.cc around lines 92-98)

Alternatively, we really only care that ALL happens after the rest of the
signals, so you could add a separate priority for that, but that would make the
rest of the code not as clean, so I vote for the previous approach.

[jdduke (slow), 2015-01-30 18:43:35]

On 2015/01/30 18:22:28, vmpstr wrote:
> On 2015/01/30 17:35:06, jdduke wrote:
> > vmpstr@: PTAL, thanks (see https://codereview.chromium.org/835633003/ for
more
> > context).
> 
> This would violate some conditions, since kTaskSetFinishedTaskPriority +
> max(task_set) >= kTileTaskPriorityBase. I think the patch is the correct fix,
> but can you update kTileTaskPriorityBase to be something > 2 (maybe like 10u),
> rename kTaskSetFinishedTaskPriority to kTaskSetFinishedTaskPriorityBase and
> update the comment near there explaining what's going on? :) (this is all in
> tile_task_worker_pool.cc around lines 92-98)
> 
> Alternatively, we really only care that ALL happens after the rest of the
> signals, so you could add a separate priority for that, but that would make
the
> rest of the code not as clean, so I vote for the previous approach.

Done. I was tempted to switch the types to be enums to allow static_assert, but
it looks like there are some other related TODOs so I held off.

[vmpstr, 2015-01-30 18:51:58]

lgtm

https://codereview.chromium.org/875573006/diff/20001/cc/resources/tile_task_w...
File cc/resources/tile_task_worker_pool.cc (right):

https://codereview.chromium.org/875573006/diff/20001/cc/resources/tile_task_w...
cc/resources/tile_task_worker_pool.cc:101: unsigned
TileTaskWorkerPool::kTileTaskPriorityBase = 10U;
nit: lower case u for consistency.

[jdduke (slow), 2015-01-30 19:02:05]

Thanks for review!

https://codereview.chromium.org/875573006/diff/20001/cc/resources/tile_task_w...
File cc/resources/tile_task_worker_pool.cc (right):

https://codereview.chromium.org/875573006/diff/20001/cc/resources/tile_task_w...
cc/resources/tile_task_worker_pool.cc:101: unsigned
TileTaskWorkerPool::kTileTaskPriorityBase = 10U;
On 2015/01/30 18:51:58, vmpstr wrote:
> nit: lower case u for consistency.

ooph, good catch, done.

[jdduke (slow), 2015-01-30 19:02:08]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2015-01-30 19:03:24]

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

[commit-bot: I haz the power, 2015-01-30 20:12:17]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-01-30 20:14:18]

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