gpu: Report time spent performing async texture uploads.

gpu: Report time spent performing async texture uploads.

BUG=167358
TBR=gman@chromium.org

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

[reveman, 2012-12-22 02:36:11]

This adds async upload time to the total upload time. We might want to report
async upload time separately, which should be easy to add if the current version
of this patch looks good.

Eric, I included the shared memory cleanup but we can of course leave that to a
separate CL if you prefer?

[epenner, 2012-12-22 03:22:42]

https://codereview.chromium.org/11659020/diff/1/ui/gl/async_pixel_transfer_de...
File ui/gl/async_pixel_transfer_delegate_android.cc (right):

https://codereview.chromium.org/11659020/diff/1/ui/gl/async_pixel_transfer_de...
ui/gl/async_pixel_transfer_delegate_android.cc:243: base::SharedMemory*
shared_memory,
Is there no way to keep this as one parameter?  Not sure it's worth the other
cleanup..

https://codereview.chromium.org/11659020/diff/1/ui/gl/async_pixel_transfer_de...
ui/gl/async_pixel_transfer_delegate_android.cc:350: base::Unretained(this),
I don't think it's safe to pass this pointer as is. Uploads can out-live the
transfer-delegate.  The state is kept alive.

Hmm, it looks like the delegate either needs to support weak pointer, or the
uploads stats could be stored in the thread object.

[epenner, 2012-12-22 20:48:17]

https://codereview.chromium.org/11659020/diff/1/ui/gl/async_pixel_transfer_de...
File ui/gl/async_pixel_transfer_delegate_android.cc (right):

https://codereview.chromium.org/11659020/diff/1/ui/gl/async_pixel_transfer_de...
ui/gl/async_pixel_transfer_delegate_android.cc:243: base::SharedMemory*
shared_memory,
On 2012/12/22 03:22:42, epenner wrote:
> Is there no way to keep this as one parameter?  Not sure it's worth the other
> cleanup. 

Hmm, could we just pass a "Safe" shared memory parameter in addition to the old
struct?  Then it will be easy to test the unsafe version (I still want to test
that it has no performance impact)

[reveman, 2012-12-22 21:56:24]

ptal

https://codereview.chromium.org/11659020/diff/1/ui/gl/async_pixel_transfer_de...
File ui/gl/async_pixel_transfer_delegate_android.cc (right):

https://codereview.chromium.org/11659020/diff/1/ui/gl/async_pixel_transfer_de...
ui/gl/async_pixel_transfer_delegate_android.cc:243: base::SharedMemory*
shared_memory,
On 2012/12/22 20:48:17, epenner wrote:
> On 2012/12/22 03:22:42, epenner wrote:
> > Is there no way to keep this as one parameter?  Not sure it's worth the
other
> > cleanup. 
> 
> Hmm, could we just pass a "Safe" shared memory parameter in addition to the
old
> struct?  Then it will be easy to test the unsafe version (I still want to test
> that it has no performance impact)

I guess we could do that. I removed shared_memory_size and
shared_memory_data_size parameters from the latest patch as it seemed
unnecessary to pass those to the other thread. I'll switch to a "safe" shm mem
parameter if you prefer that.

https://codereview.chromium.org/11659020/diff/1/ui/gl/async_pixel_transfer_de...
ui/gl/async_pixel_transfer_delegate_android.cc:350: base::Unretained(this),
On 2012/12/22 03:22:42, epenner wrote:
> I don't think it's safe to pass this pointer as is. Uploads can out-live the
> transfer-delegate.  The state is kept alive.
> 
> Hmm, it looks like the delegate either needs to support weak pointer, or the
> uploads stats could be stored in the thread object.

added support for weak pointer in latest patch. it would be nice to make the
transfer thread part of the transfer delegate so it will be shut down and all
pending tasks are completed when destroying the delegate. Looks like
TransferStateInternal::thread_texture_id would have to be registered with the
transfer thread so it could be deleted at thread cleanup.

[epenner, 2013-01-03 23:59:15]

lgtm

Sorry for the delay! (was gone last week). 

> I guess we could do that. I removed shared_memory_size and
> shared_memory_data_size parameters from the latest patch as it seemed
> unnecessary to pass those to the other thread. I'll switch to a "safe" shm mem
> parameter if you prefer that.

Looks really good as you did it. Nice an easy to test without the dupe as well.

> added support for weak pointer in latest patch. it would be nice to make the
> transfer thread part of the transfer delegate so it will be shut down and all
> pending tasks are completed when destroying the delegate. Looks like
> TransferStateInternal::thread_texture_id would have to be registered with the
> transfer thread so it could be deleted at thread cleanup.

I actually like it the way you did it with the weak pointer hidden in the
Android implementation.

The delegate used to be a singleton and own the thread actually. I changed it to
delegate-per-decoder, partially for easier testing and partially because I think
it is more flexible (the implementation can have per-decoder state as well as
global state in the singleton thread).

[nduca, 2013-01-05 04:47:55]

lgtmrubberstamp

[commit-bot: I haz the power, 2013-01-05 21:57:03]

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

[commit-bot: I haz the power, 2013-01-05 22:10:10]

Retried try job too often on ios_dbg_simulator for step(s) sync_unit_tests,
unit_tests

[commit-bot: I haz the power, 2013-01-06 17:24:31]

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

[commit-bot: I haz the power, 2013-01-06 17:36:45]

Retried try job too often on ios_dbg_simulator for step(s) sync_unit_tests,
unit_tests

[commit-bot: I haz the power, 2013-01-08 17:06:21]

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

[commit-bot: I haz the power, 2013-01-08 17:54:18]

Change committed as 175548