cc: Use asynchronous set pixels API for impl-side painting.

cc: Use asynchronous set pixels API for impl-side painting.

BUG=161338
TEST=manual


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

[reveman, 2012-12-04 23:04:32]

This makes the tile manager use the async set pixels API.

These changes need to land first:
https://bugs.webkit.org/show_bug.cgi?id=103995
https://codereview.chromium.org/11412043/

[nduca, 2012-12-04 23:59:26]

https://codereview.chromium.org/11348384/diff/1/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://codereview.chromium.org/11348384/diff/1/cc/tile_manager.cc#newcode60
cc/tile_manager.cc:60: PendingSetPixels::PendingSetPixels(
I think this is overkill. Just do what I told you originally --

managedResource {
  resource  -- set when its allocated
  state { rasterizing, uploading, ready }

Tile::resource_id accessor -- returns resource id only when ready

Pending queue is just a queue of tiles. Done.

https://codereview.chromium.org/11348384/diff/1/cc/tile_manager.cc#newcode235
cc/tile_manager.cc:235: void TileManager::PrepareToDraw() {
Lets call this CheckForCompletedUploads() so its obvious what its doing.

[reveman, 2012-12-05 23:21:18]

PTAL.

https://codereview.chromium.org/11348384/diff/1/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://codereview.chromium.org/11348384/diff/1/cc/tile_manager.cc#newcode60
cc/tile_manager.cc:60: PendingSetPixels::PendingSetPixels(
On 2012/12/04 23:59:26, nduca wrote:
> I think this is overkill. Just do what I told you originally --
> 
> managedResource {
>   resource  -- set when its allocated
>   state { rasterizing, uploading, ready }
> 
> Tile::resource_id accessor -- returns resource id only when ready
> 
> Pending queue is just a queue of tiles. Done.

Done.

https://codereview.chromium.org/11348384/diff/1/cc/tile_manager.cc#newcode235
cc/tile_manager.cc:235: void TileManager::PrepareToDraw() {
On 2012/12/04 23:59:26, nduca wrote:
> Lets call this CheckForCompletedUploads() so its obvious what its doing.

Done. Now CheckForCompletedSetPixels().

[epennerAtGoogle, 2012-12-06 00:33:17]

https://codereview.chromium.org/11348384/diff/10002/cc/tile_manager.cc
File cc/tile_manager.cc (left):

https://codereview.chromium.org/11348384/diff/10002/cc/tile_manager.cc#oldcod...
cc/tile_manager.cc:281: 
Could this possibly just be called at the beginning of managing tiles, and just
keep scheduling management until all tiles are done uploading? If feels like the
only two things we need are redraw and manage.

Also, could we just traverse all tiles rather than keeping a different list? I'm
sure we will end up having a couple traversals of all tiles where this can fit.

[nduca, 2012-12-06 03:19:52]

There are thousands of tiles. There are 10s of uploading tiles.

[nduca, 2012-12-06 03:21:30]

https://codereview.chromium.org/11348384/diff/10002/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://codereview.chromium.org/11348384/diff/10002/cc/tile_manager.cc#newcod...
cc/tile_manager.cc:291: void TileManager::CheckForCompletedSetPixels() {
I had asked for this before, and I'll ask again.

Uploads. These are uploads. Call them uploads. SetPixels is detail.
CheckForCompletedUploads. ScheduleCheckForCompletedUploads.

[epennerAtGoogle, 2012-12-06 04:04:34]

On 2012/12/06 03:19:52, nduca wrote:
> There are thousands of tiles. There are 10s of uploading tiles.

Sure, but you are going to sort the tiles on the impl thread right? So that's
way more expensive than traversing the tiles once.

In any case I don't mind the extra queue, just thought it should go together
with managing the tiles.

[nduca, 2012-12-06 06:05:00]

Oh, I see what you're saying. Here's where my head is at, wdyt?

Roughly, the manage tiles usually will happen at the front of the frame, right
after we get a scroll update for instance. The draw, on the other hand will
eventually happen toward the end of the frame. There's a good chance in those
cases that the uploads we began at the front of the frame have finished by the
time we draw.... so its good to quickly check for those just before we draw, in
case the data is already uploaded.

[epennerAtGoogle, 2012-12-06 06:20:22]

On 2012/12/06 06:05:00, nduca wrote:
> Oh, I see what you're saying. Here's where my head is at, wdyt?
> 
> Roughly, the manage tiles usually will happen at the front of the frame, right
> after we get a scroll update for instance. The draw, on the other hand will
> eventually happen toward the end of the frame. There's a good chance in those
> cases that the uploads we began at the front of the frame have finished by the
> time we draw.... so its good to quickly check for those just before we draw,
in
> case the data is already uploaded.

Ahh okay, I see the possible advantage then. However, I'm personally skeptical
of anything that breaks a frame into more than one piece, especially on the impl
thread where we are not actually rendering.

If we want something really precise we are better of doing it in the GPU
process, like the upload throttling. For example we could add 'predicates' or
something which test a query and mask the quad commands from executing.

[nduca, 2012-12-06 06:24:50]

Its a tantalizing idea. Completion of an upload doesn't mean its going to be
drawn, though so you probably need to think through three subcases:
a) its offscreen
b) its on the pending tree and we're checking for its completion in order to
decide that we can switch to the active tree
c) its on the active tree but we have a low res tile that we could use until it
finishes uploading

I'd be curious to talk about what you mean about breaking a frame into multiple
pieces. But maybe thats for an in-person discussion tomorrow?

[reveman, 2012-12-06 15:28:20]

https://codereview.chromium.org/11348384/diff/10002/cc/tile_manager.cc
File cc/tile_manager.cc (left):

https://codereview.chromium.org/11348384/diff/10002/cc/tile_manager.cc#oldcod...
cc/tile_manager.cc:281: 
On 2012/12/06 00:33:17, epennerAtGoogle wrote:
> Could this possibly just be called at the beginning of managing tiles, and
just
> keep scheduling management until all tiles are done uploading? If feels like
the
> only two things we need are redraw and manage.
> 
> Also, could we just traverse all tiles rather than keeping a different list?
I'm
> sure we will end up having a couple traversals of all tiles where this can
fit.

For minimal latency I think we want to call this just before we evaluate if the
active tree should be updated.

FYI, the more time there is between issuing the beginSetPixels and checking for
its completion the less likely we are to cause an unnecessary command buffer
flush.

Having the queue allows us to check queries in the order they are issued which
also helps avoid unnecessary flushes.

https://codereview.chromium.org/11348384/diff/10002/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://codereview.chromium.org/11348384/diff/10002/cc/tile_manager.cc#newcod...
cc/tile_manager.cc:291: void TileManager::CheckForCompletedSetPixels() {
On 2012/12/06 03:21:30, nduca wrote:
> I had asked for this before, and I'll ask again.
> 
> Uploads. These are uploads. Call them uploads. SetPixels is detail.
> CheckForCompletedUploads. ScheduleCheckForCompletedUploads.

We've moved away from using the "upload" term in relation to resources [1].
Uploads are specific to texture resources and this code is not restricted to a
specific type of resource. I'd really prefer to not start using that term in
resource type independent code again. We don't need to use SetPixels here
though. How about CheckForCompletedTileInitialization()?

[1] http://src.chromium.org/viewvc/chrome?view=rev&revision=165632

[nduca, 2012-12-06 16:20:09]

SetPixels is fine. I wouldn't have approved the change away from uploads
personally but since its done, its done.

[nduca, 2012-12-06 16:37:09]

https://codereview.chromium.org/11348384/diff/10002/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11348384/diff/10002/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:789: void
LayerTreeHostImpl::ScheduleCheckForCompletedSetPixels()
Probably worth explaining why this just does a setNeedsRedraw

https://codereview.chromium.org/11348384/diff/10002/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://codereview.chromium.org/11348384/diff/10002/cc/tile_manager.cc#newcod...
cc/tile_manager.cc:298: // Set pixel tasks complete in the order they are
posted.
does async upload extension guarantee completion of uploads in the order that
they were issued?

https://codereview.chromium.org/11348384/diff/10002/cc/tile_manager.cc#newcod...
cc/tile_manager.cc:465: managed_tile_state.resource = resource.Pass();
this is getting really insane to me that you have resource and resource_id, and
sometimes one is set and not the other.

PLease pleas please please please make this cleaner.

Again, from my PoV, this would have been so much cleaner all along:
1. put the resource_id into the managed tile state up front
2. have a managed_state.status = {ready, rasterizing, uploading}
3. mutate the status as needed
4. remove resource_id() accessor; ready_resource_id() then can be 0 if non-ready

[epennerAtGoogle, 2012-12-06 17:53:44]

Couple comments in one.

I get the potential latency advantage for this scheduling complexity, but I'm
debating it might not be justified. It's only say ~8ms in practice that we are
saving. If we are scrolling/animating we should not care about paint latency *at
all*. Some browsers practically stop painting entirely... On the other hand if
we are not scrolling/animating we are better off doing something that will save
a lot more than 8ms... Such as blocking on the upload completion on the GPU
(with a sync point or something). This doesn't exist yet but it could be added. 

My point about breaking frames into pieces is about this type of thing. Games
just have one 'scheduled action': renderFrame.

[epennerAtGoogle, 2012-12-06 18:00:11]

Sound good on the queue of uploading tiles... On a side-note I've seen a crazy
number of 'put changed' traces when testing async uploads. Could this be these
flushes? We should chat about the exact conditions that cause flushes. Should we
be recycling a single set of transfer buffers?

[reveman, 2012-12-06 18:18:50]

On 2012/12/06 18:00:11, epennerAtGoogle wrote:
> Sound good on the queue of uploading tiles... On a side-note I've seen a crazy
> number of 'put changed' traces when testing async uploads. Could this be these
> flushes? We should chat about the exact conditions that cause flushes. Should
we
> be recycling a single set of transfer buffers?

make sure you're at r171046 or later as you'll see a lot of unnecessary flushes
otherwise.

[reveman, 2012-12-06 18:32:49]

On 2012/12/06 17:53:44, epennerAtGoogle wrote:
> Couple comments in one.
> 
> I get the potential latency advantage for this scheduling complexity, but I'm
> debating it might not be justified. It's only say ~8ms in practice that we are
> saving. If we are scrolling/animating we should not care about paint latency
*at
> all*. Some browsers practically stop painting entirely... On the other hand if
> we are not scrolling/animating we are better off doing something that will
save
> a lot more than 8ms... Such as blocking on the upload completion on the GPU
> (with a sync point or something). This doesn't exist yet but it could be
added. 
> 
> My point about breaking frames into pieces is about this type of thing. Games
> just have one 'scheduled action': renderFrame.

this doesn't just affect main thread paint latency, it affects impl side paint
latency too which is important when scrolling. but i still agree with you
though. it's not clear at this point that the added complexity is justified.

[epennerAtGoogle, 2012-12-06 20:36:36]

> this doesn't just affect main thread paint latency, it affects impl side paint
> latency too which is important when scrolling. but i still agree with you
> though. it's not clear at this point that the added complexity is justified.

I actually don't think it is important really. It will eliminate checkerboard
tiles ~8ms faster, but we have already failed if we show checkerboard to begin
with.

Talked with Nat and he convinced me the complexity is worth it when the impl
side isn't very far ahead of the GPU (the ideal case).

However, Nat I forgot to mention sync points again as an alternative. When
something other than scrolling is driving the frame, we can add a blocking sync
before the draw calls. This way we can always issue the draw immediately and not
risk missing the vsync. WDYT?

[reveman, 2012-12-06 20:40:34]

https://codereview.chromium.org/11348384/diff/10002/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11348384/diff/10002/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:789: void
LayerTreeHostImpl::ScheduleCheckForCompletedSetPixels()
On 2012/12/06 16:37:09, nduca wrote:
> Probably worth explaining why this just does a setNeedsRedraw

Done.

https://codereview.chromium.org/11348384/diff/10002/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://codereview.chromium.org/11348384/diff/10002/cc/tile_manager.cc#newcod...
cc/tile_manager.cc:298: // Set pixel tasks complete in the order they are
posted.
On 2012/12/06 16:37:09, nduca wrote:
> does async upload extension guarantee completion of uploads in the order that
> they were issued?

Yes and it's advantages not to relax this requirement unless we actually make
use of it on the service side.

https://codereview.chromium.org/11348384/diff/10002/cc/tile_manager.cc#newcod...
cc/tile_manager.cc:465: managed_tile_state.resource = resource.Pass();
On 2012/12/06 16:37:09, nduca wrote:
> this is getting really insane to me that you have resource and resource_id,
and
> sometimes one is set and not the other.
> 
> PLease pleas please please please make this cleaner.
> 
> Again, from my PoV, this would have been so much cleaner all along:
> 1. put the resource_id into the managed tile state up front
> 2. have a managed_state.status = {ready, rasterizing, uploading}
> 3. mutate the status as needed
> 4. remove resource_id() accessor; ready_resource_id() then can be 0 if
non-ready

I got rid of ManagedTileState::resource_id by just replacing resource_id() with
a non-inlined version that is conditional on resource_is_being_initialized.

Passing the ownership of ResourcePool::Resource to the tile right away requires
additional changes. When a raster task completes, we call
AssignGpuMemoryToTiles() to determine if we should abort the initialization or
not. The resource can't be freed at this point because we still need to release
the pixel buffer. Here are some options:

1. Keep the ownership of the resource with the raster task until beginSetPixels
is called. (current patch)

2. Keep the ownership of the resource on tile. Take ownership of resource back
before calling AssignGpuMemoryToTiles() in OnRasterTaskCompleted() and then give
it back to the tile when beginSetPixels() is called.

3. Split AssignGpuMemoryToTiles() up into two functions. One that sets
can_use_gpu_memory and one that frees resources.

4. Make ResourcePool::Resource aware of pixel buffers.

Let me know which one you prefer or if you have a better solution.

[reveman, 2012-12-06 20:44:25]

> However, Nat I forgot to mention sync points again as an alternative. When
> something other than scrolling is driving the frame, we can add a blocking
sync
> before the draw calls. This way we can always issue the draw immediately and
not
> risk missing the vsync. WDYT?

I like the idea of issuing a sync when we depend on uploads to complete before
we can draw.

[nduca, 2012-12-06 22:19:20]

It works for the active tree. But it doesn't work for the pending tree.

[epennerAtGoogle, 2012-12-07 00:37:48]

On 2012/12/06 22:19:20, nduca wrote:
> It works for the active tree. But it doesn't work for the pending tree.

The latest is looking nice. And I really like the pending queue after all.

[nduca, 2012-12-07 00:48:40]

lgtm, this looks good

[commit-bot: I haz the power, 2012-12-07 00:56:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/11348384/21001

[commit-bot: I haz the power, 2012-12-07 08:18:39]

Change committed as 171721