cc: Complete pending tile uploads if they are needed for tree activation

cc: Complete pending tile uploads if they are needed for tree activation

When we are checking whether resources are ready for tree activation,
force any pending tile upload to complete. This allows us to render
immediately and avoid polling for tile upload completion.

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

[Sami, 2013-02-27 18:01:08]

This patch implements the renderer-side shallow async upload flushing logic to
let us render sooner when only a few uploads are pending. It depends on
https://codereview.chromium.org/12210129/ and
https://bugs.webkit.org/show_bug.cgi?id=110987. PTAL.

[Sami, 2013-03-04 19:05:36]

This version shuffles the logic so that we flush uploads while we are checking
whether the new tree is ready for activation.

I'm seeing some black tiles flashing by with the combination of this and
https://codereview.chromium.org/12210129/, so consider this a rough draft.

[Sami, 2013-03-07 19:33:02]

Okay, I think this one is ready for review. I've tested it on Nexus 10 and
Galaxy S3 and it seems to be performing nicely without any additional jank.
Please take a look, thanks.

[reveman, 2013-03-07 20:31:58]

I think this is a good approach but to get ideal latency out of this we probably
need to flush the uploads a bit earlier. areVisibleResourcesReady() is currently
only called just before we draw. When not in smoothness mode we might want to do
a areVisibleResourcesReady() pass after issuing each batch of uploads. I think
this is better left as a follow up work though.

https://codereview.chromium.org/12321053/diff/16001/cc/picture_layer_impl.cc
File cc/picture_layer_impl.cc (right):

https://codereview.chromium.org/12321053/diff/16001/cc/picture_layer_impl.cc#...
cc/picture_layer_impl.cc:512:
layerTreeImpl()->tile_manager()->FlushPendingTileUploadIfNeeded(*iter);
We should avoid this when in "smoothness takes priority" mode. Probably also
when animations are running. I think should be as conservative as possible in
this first patch to prevent any kind of smoothness regression.

https://codereview.chromium.org/12321053/diff/16001/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://codereview.chromium.org/12321053/diff/16001/cc/tile_manager.cc#newcod...
cc/tile_manager.cc:459: managed_tile_state.resource_is_being_initialized =
false;
I like this approach of not moving the tile out of tiles_with_pending_upload_
until the query has passed.

This introduces a new state for the tile though. A state where we know the
upload will be done at the time we draw. I think this deserves a new raster
state instead of keeping it in UPLOAD_STATE.

[Sami, 2013-03-08 18:01:10]

> I think this is a good approach but to get ideal latency out of this we
probably
> need to flush the uploads a bit earlier. areVisibleResourcesReady() is
currently
> only called just before we draw. When not in smoothness mode we might want to
do
> a areVisibleResourcesReady() pass after issuing each batch of uploads. I think
> this is better left as a follow up work though.

That's a good point. Currently on Android checking as late as possible is the
best strategy to minimize the chances of blocking. However when we start
ordering uploads based on the waits as with your idle uploads, we should get
that information to the GPU process sooner. I think we need to do some more
testing to see much earlier we can do that.

https://codereview.chromium.org/12321053/diff/16001/cc/picture_layer_impl.cc
File cc/picture_layer_impl.cc (right):

https://codereview.chromium.org/12321053/diff/16001/cc/picture_layer_impl.cc#...
cc/picture_layer_impl.cc:512:
layerTreeImpl()->tile_manager()->FlushPendingTileUploadIfNeeded(*iter);
On 2013/03/07 20:31:58, David Reveman wrote:
> We should avoid this when in "smoothness takes priority" mode. Probably also
> when animations are running. I think should be as conservative as possible in
> this first patch to prevent any kind of smoothness regression.

Agreed, let's be careful about this. I've added a check for smoothness and
animations.

https://codereview.chromium.org/12321053/diff/16001/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://codereview.chromium.org/12321053/diff/16001/cc/tile_manager.cc#newcod...
cc/tile_manager.cc:459: managed_tile_state.resource_is_being_initialized =
false;
On 2013/03/07 20:31:58, David Reveman wrote:
> This introduces a new state for the tile though. A state where we know the
> upload will be done at the time we draw. I think this deserves a new raster
> state instead of keeping it in UPLOAD_STATE.

Done, I've added UPLOAD_FLUSHED_STATE.

[reveman, 2013-03-08 18:56:21]

On 2013/03/08 18:01:10, Sami wrote:
> > I think this is a good approach but to get ideal latency out of this we
> probably
> > need to flush the uploads a bit earlier. areVisibleResourcesReady() is
> currently
> > only called just before we draw. When not in smoothness mode we might want
to
> do
> > a areVisibleResourcesReady() pass after issuing each batch of uploads. I
think
> > this is better left as a follow up work though.
> 
> That's a good point. Currently on Android checking as late as possible is the
> best strategy to minimize the chances of blocking. However when we start
> ordering uploads based on the waits as with your idle uploads, we should get
> that information to the GPU process sooner. I think we need to do some more
> testing to see much earlier we can do that.
> 

I've done some testing with the new worker pool, idle uploads and this patch on
desktop. It works very well. Not sure that flushing earlier is needed. It
reduces the latency a bit but throughput is just as good without such a change.

[reveman, 2013-03-08 18:57:35]

lgtm, eric, nat what do you think?

[epenner, 2013-03-09 02:30:34]

On 2013/03/08 18:57:35, David Reveman wrote:
> lgtm, eric, nat what do you think?

Same comment as the new GL entry point: I don't see a good way to implement
re-ordering of uploads on the thread, unless we pass all uploads in one
GL/ResourceProvider entry-point (it could be like glDeleteTextures which takes
an array of ids).

However, since re-ordering is not strictly required we could start this way.
Just let me know your thoughts and then LGTM (and after the one comment is
addressed).

[Sami, 2013-03-11 11:40:04]

On 2013/03/09 02:30:34, epenner wrote:
> Same comment as the new GL entry point: I don't see a good way to implement
> re-ordering of uploads on the thread, unless we pass all uploads in one
> GL/ResourceProvider entry-point (it could be like glDeleteTextures which takes
> an array of ids).

Alternatively the GPU process could pipeline waits so that we would only
actually perform them once you bind the texture you waited on earlier. However
passing a list of textures to wait on like you said would be less error prone
and probably more efficient.

> However, since re-ordering is not strictly required we could start this way.

Yeah, I'd like to revisit the re-ordering later with more data. It seems like
reordering would help most when we're scrolling and old tiles fall off the
screen, but right now we're not doing the waits at all in smoothness mode.

> Just let me know your thoughts and then LGTM (and after the one comment is
> addressed).

Sorry for being dense but which comment are you referring to?

[epenner, 2013-03-11 19:18:40]

Sorry! For some reason it didn't publish this comment before.

https://codereview.chromium.org/12321053/diff/22001/cc/resource_provider.cc
File cc/resource_provider.cc (right):

https://codereview.chromium.org/12321053/diff/22001/cc/resource_provider.cc#n...
cc/resource_provider.cc:1136: UnlockForWrite(id);
Can you explain why we do this only when there is no wait inserted?  It seems
like it should be the opposite to me.

In any case it seems fishy that if you call this function twice in a row that
UnlockForWrite will get called the second time through since we set it to false
the first time.

[nduca, 2013-03-11 19:41:47]

https://codereview.chromium.org/12321053/diff/22001/cc/resource_provider.h
File cc/resource_provider.h (right):

https://codereview.chromium.org/12321053/diff/22001/cc/resource_provider.h#ne...
cc/resource_provider.h:283: void InsertWaitForSetPixels(ResourceId id);
ForceSetPixelsToComplete?

https://codereview.chromium.org/12321053/diff/22001/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://codereview.chromium.org/12321053/diff/22001/cc/tile_manager.cc#newcod...
cc/tile_manager.cc:131: case UPLOAD_FLUSHED_STATE:
I'm not sure I'm wild about flush as a term here. Flush means to me ~= glFlush
which means "we have submitted it" which I dont actually think is what you
intend here. Can you please come up with something more consistent in naming?

[reveman, 2013-03-12 06:25:58]

https://codereview.chromium.org/12321053/diff/22001/cc/resource_provider.cc
File cc/resource_provider.cc (right):

https://codereview.chromium.org/12321053/diff/22001/cc/resource_provider.cc#n...
cc/resource_provider.cc:1147: DCHECK(resource->locked_for_write);
you need a "|| resource->wait_for_set_pixels_inserted" here too as the tile
manager might call abort anytime before DidSetPixelsComplete returns true.

https://codereview.chromium.org/12321053/diff/22001/cc/resource_provider.cc#n...
cc/resource_provider.cc:1165: UnlockForWrite(id);
and you'll need your wait_for_set_pixels_inserted check from
DidSetPixelsComplete here too.

[Sami, 2013-03-12 14:13:07]

Thanks everyone. All comments addressed, another look please?

https://codereview.chromium.org/12321053/diff/22001/cc/resource_provider.cc
File cc/resource_provider.cc (right):

https://codereview.chromium.org/12321053/diff/22001/cc/resource_provider.cc#n...
cc/resource_provider.cc:1136: UnlockForWrite(id);
On 2013/03/11 19:18:40, epenner wrote:
> Can you explain why we do this only when there is no wait inserted?  It seems
> like it should be the opposite to me.
>
> In any case it seems fishy that if you call this function twice in a row that
> UnlockForWrite will get called the second time through since we set it to
false
> the first time.

I guess the confusion comes from the fact that forcing the upload to complete
also unlocks the resource, so we have to check here whether we already unlocked.
I'll rework this so that we keep the resource locked but allow reading/sampling
from it if the completion was forced.

https://codereview.chromium.org/12321053/diff/22001/cc/resource_provider.cc#n...
cc/resource_provider.cc:1147: DCHECK(resource->locked_for_write);
Well spotted, done.

https://codereview.chromium.org/12321053/diff/22001/cc/resource_provider.cc#n...
cc/resource_provider.cc:1165: UnlockForWrite(id);
On 2013/03/12 06:25:58, David Reveman wrote:
> and you'll need your wait_for_set_pixels_inserted check from
> DidSetPixelsComplete here too.

Done.

https://codereview.chromium.org/12321053/diff/22001/cc/resource_provider.h
File cc/resource_provider.h (right):

https://codereview.chromium.org/12321053/diff/22001/cc/resource_provider.h#ne...
cc/resource_provider.h:283: void InsertWaitForSetPixels(ResourceId id);
On 2013/03/11 19:41:47, nduca wrote:
> ForceSetPixelsToComplete?

sgtm, done.

https://codereview.chromium.org/12321053/diff/22001/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://codereview.chromium.org/12321053/diff/22001/cc/tile_manager.cc#newcod...
cc/tile_manager.cc:131: case UPLOAD_FLUSHED_STATE:
On 2013/03/11 19:41:47, nduca wrote:
> I'm not sure I'm wild about flush as a term here. Flush means to me ~= glFlush
> which means "we have submitted it" which I dont actually think is what you
> intend here. Can you please come up with something more consistent in naming?

Agreed. I'll call it forced upload completion here and elsewhere like you
suggested.

[nduca, 2013-03-12 17:40:19]

Is there any distinction between forced_upload_completion_state and just
upload_state?

[nduca, 2013-03-12 17:41:00]

aka i think we could just make the force call move us to upload state and
dispense with the added enum.

[Sami, 2013-03-12 17:55:01]

On 2013/03/12 17:41:00, nduca wrote:
> aka i think we could just make the force call move us to upload state and
> dispense with the added enum.

The only real difference between the states is the value of
resource_is_being_initialized. David asked me to add the new enum in comment #4,
and maybe it's useful for tracking the number of tiles in different bins but I
don't feel too strongly about it.

[reveman, 2013-03-12 17:57:01]

On 2013/03/12 17:41:00, nduca wrote:
> aka i think we could just make the force call move us to upload state and
> dispense with the added enum.

Tiles transition from "upload" to "forced upload" state when a tile upload is
forced to complete. Sami didn't have this state in the original patch but I
asked him to add it as I found it hard to determine the state of tiles
otherwise. ie. what prevents us from calling ForceTileUpload() twice..

[reveman, 2013-03-12 18:03:17]

Long term I also like to see us move away from duplicate properties such as
resource_is_being_initialized and instead just use the state to determine if a
resource is ready. I think adding a new state is in line with that.

[nduca, 2013-03-12 18:24:47]

LGTM. Sami, can you please do a followup to remove is_resource_being_uploaded?

[epenner, 2013-03-12 19:50:28]

On 2013/03/12 18:24:47, nduca wrote:
> LGTM. Sami, can you please do a followup to remove is_resource_being_uploaded?

LGTM.

Someone sometime should convert the API to take an array of textures, but the
flow looks good and will apply in the same manner.

[reveman, 2013-03-12 20:23:30]

eric, instead of using an array, would it be enough to just require a main gpu
thread message loop round-trip to transition from sync upload back to async
uploads again? that way we would make sure to pick up any additional wait
requests before kicking off more async work.

[epenner, 2013-03-12 20:47:19]

On 2013/03/12 20:23:30, David Reveman wrote:
> eric, instead of using an array, would it be enough to just require a main gpu
> thread message loop round-trip to transition from sync upload back to async
> uploads again? that way we would make sure to pick up any additional wait
> requests before kicking off more async work.

That sounds like it could work! Actually we discussed something similar before,
where uploads could also be batched per GL flush.

I think being notified of both 'flush begin' and 'flush end' in the delegate
would allow us to do all sorts of stuff to improve efficiency. Flush begin can
be used to bind uploads (instead of doing that in make current).

If you guys like that approach then that sounds good to me! If we batch all our
uploads in the client into one shallow flush, then we could batch work on the
GPU side very efficiently.

[reveman, 2013-03-12 21:41:36]

Sounds good. Uploads are already batched using shallow flush so this should
"just work" if implemented properly on the gpu side.

[nduca, 2013-03-13 06:00:00]

lgtm

[commit-bot: I haz the power, 2013-03-13 10:46:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/12321053/35001

[commit-bot: I haz the power, 2013-03-13 11:15:08]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-03-13 11:43:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/12321053/58001

[commit-bot: I haz the power, 2013-03-13 12:02:28]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-03-13 12:06:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/12321053/58001

[commit-bot: I haz the power, 2013-03-13 12:19:45]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-03-13 13:28:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/12321053/58001

[commit-bot: I haz the power, 2013-03-13 13:43:06]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-03-13 15:02:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/12321053/58001

[commit-bot: I haz the power, 2013-03-13 15:13:17]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-03-13 18:47:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/12321053/58001

[commit-bot: I haz the power, 2013-03-13 20:28:34]

Change committed as 187931