gpu: Add the ability to wait on upload completion.

gpu: Add the ability to wait on upload completion.

Patch originally by Eric Penner <epenner@chromium.org>;.

We currently have high latency if need to wait for uploads
in the renderer client. If we can do a shallow client side
wait it will be much better. This is effectively the
same as doing synchronous uploads, but exposes less driver
bug surface area then mixing uploads across threads.

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

[epenner, 2013-02-12 02:37:28]

Ptal. I whipped this up because it appears we hit a driver bug with doing both
async+sync uploads.

If we could figure out a secret hand-shake to invoke this shallow wait, we can
do it entirely with the async calls. Alternatively we could plumb in an
"asyncTransferShallowWait" function, but I'm worried about that being difficult
to cherry-pick.

[nduca, 2013-02-12 03:36:17]

How does this approach compare to advertising a GL extension that we dont
support mixed async sync uploads? Are all devices affected or just n4?

[epenner, 2013-02-12 04:18:12]

On 2013/02/12 03:36:17, nduca wrote:
> How does this approach compare to advertising a GL extension that we dont
> support mixed async sync uploads? Are all devices affected or just n4?

I'm not against supporting different extension levels, but we could up having a
different level for each driver ;)

If we had extension levels, currently we would need to disable it for Qualcomm
(correctness bug) and IMG (terrible sync performance). Compared with this
solution which would work on all devices.

[nduca, 2013-02-12 04:22:02]

But this solution makes the gpu wait on the async upload. Thats probably a perf
hit in its own right, no?

[nduca, 2013-02-12 04:22:24]

s/makes the gpu wait/makes the sync uploads wait on the async uploads/

[epenner, 2013-02-12 04:22:39]

I should note that maybe we can just fix the Qualcomm issue with a bit more
investigation. IMG is a lost cause to some extent, so we just need to
special-case it into a very tightly controlled path.

[epenner, 2013-02-12 04:28:50]

On 2013/02/12 04:22:39, epenner wrote:
> I should note that maybe we can just fix the Qualcomm issue with a bit more
> investigation. IMG is a lost cause to some extent, so we just need to
> special-case it into a very tightly controlled path.

> But this solution makes the gpu wait on the async upload. Thats probably a
perf hit in its own right, no?

I think it shouldn't be any larger of a perf hit than sync uploads. If we issue
say 10 uploads and then block, it's the same as doing 10 sync uploads.

That said, if we can fix qualcomm we can just disable sync uploads for IMG in
the compositor and we'll be good.

[Sami, 2013-02-12 20:11:50]

Thanks for this Eric. Sorry I wasn't able to try it out yet today.

I wonder if we could just allow N outstanding async uploads and only start
waiting on the renderer side if we go beyond N? Then we could just composite the
new frame immediately after uploading, and the GPU thread would know to delay
the commands until the uploads have finished -- or does it have the smarts to do
that?

To avoid having sync uploads wait on async uploads we could just avoid sync
uploads completely if there are outstanding async ones. It's probably a good
idea anyway to avoid spamming the GPU.

[Sami, 2013-02-12 20:14:05]

Oh, and it occurs to me that we'll need to support mixing sync/async uploads for
canvas and WebGL. I haven't seen any corruption problems on Nexus 4 with those,
but maybe I haven't tried texture-upload heavy sites.

[epenner, 2013-02-12 20:49:07]

Yes Sami that was my thinking. Something like:
- If we aren't blocked on uploads, don't do anything special
- If we are blocked on <=N uploads, add a shallowWait and draw
- If we are blocked on >N upload, wait for completion and draw

Unfortunately we don't have the ability to trigger the shallowWait at the
moment.

Regarding mixing uploads, the only case I'm worried about is uploading to the
*same texture* on both threads. If you pick one and stick with it, we will be
fine. Also if the Qualcomm bug can be solved we should be fine in all cases
except for IMG performance.

[Sami, 2013-02-13 12:38:49]

As another data point, disabling server-side async uploads fixes the bug on
Nexus 4, so it does indeed seem to be an issue with uploading to the same
texture from two threads like Eric said.

I'll give this patch a try next.

On 2013/02/12 20:49:07, epenner wrote:
> Yes Sami that was my thinking. Something like:
> - If we aren't blocked on uploads, don't do anything special
> - If we are blocked on <=N uploads, add a shallowWait and draw
> - If we are blocked on >N upload, wait for completion and draw
> 

Ok, I think we're talking about the same thing :)

> Unfortunately we don't have the ability to trigger the shallowWait at the
> moment.

How about a new entrypoint just for inserting the server-side wait? Something
based on fences might be a little too fancy. I'll try to sketch this out.
 
> Regarding mixing uploads, the only case I'm worried about is uploading to the
> *same texture* on both threads. If you pick one and stick with it, we will be
> fine. Also if the Qualcomm bug can be solved we should be fine in all cases
> except for IMG performance.

[epenner, 2013-02-13 19:12:44]

> How about a new entrypoint just for inserting the server-side wait? Something
based on fences might be a little too fancy. I'll try to sketch this out.

That would be great. But keep in mind we don't want regular command buffer
fences. We actually want to wait on the upload thread. I was thinking of
literally adding "asyncTransferShallowWait()", which could directly call the new
delegate function.   My worry there was cherry-picking that though... Hence the
idea of a secret hand-shake to invoke it ;)

[Sami, 2013-02-13 19:17:11]

On 2013/02/13 19:12:44, epenner wrote:
> That would be great. But keep in mind we don't want regular command buffer
> fences. We actually want to wait on the upload thread. I was thinking of
> literally adding "asyncTransferShallowWait()", which could directly call the
new
> delegate function.   My worry there was cherry-picking that though... Hence
the
> idea of a secret hand-shake to invoke it ;)

Ah, I see, I didn't realize there was such a big diff in this area. Let's figure
out how to handle that once we get this working on trunk.

[Sami, 2013-02-27 16:44:40]

Eric, I hijacked your patch and added a glWaitAsyncTexImage2D() entry point that
the resource provider can call.

Nat, I think we can minimize the time spent blocking in the GPU thread by
waiting just before rendering the frame consuming the async upload. The thing we
want avoid is polling for upload completion right before the upload finishes and
having to wait another vsync interval before trying again.

[reveman, 2013-02-28 02:12:00]

How about we make WaitAsyncTexImage2DCHROMIUM(target) only wait for the texture
currently bound to target to finish? I think that would be more useful.

[reveman, 2013-03-04 06:10:57]

This looks good. Sami, can you add the texture specific addition and then I
think we can land this?

https://chromiumcodereview.appspot.com/12210129/diff/10001/gpu/command_buffer...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://chromiumcodereview.appspot.com/12210129/diff/10001/gpu/command_buffer...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10173: error::Error
GLES2DecoderImpl::HandleWaitAsyncTexImage2DCHROMIUM(
The android impl can do what it's currently doing but lets have this function
lookup the texture bound to the target and pass it to the async upload delegate
so an impl can do something smarter if possible.

[Sami, 2013-03-04 18:57:47]

This version makes it possible to wait on a particular upload. The Android
delegate implements this using a base::WaitableEvent. I don't think this adds
significant overhead to the normal case where there are no completion waiters --
essentially a mutex lock/unlock -- but let me know if you feel otherwise.

[reveman, 2013-03-04 19:32:58]

On 2013/03/04 18:57:47, Sami wrote:
> This version makes it possible to wait on a particular upload. The Android
> delegate implements this using a base::WaitableEvent. I don't think this adds
> significant overhead to the normal case where there are no completion waiters
--
> essentially a mutex lock/unlock -- but let me know if you feel otherwise.

I'm afraid that using a WaitableEvent for completion in the android version will
cause a performance regression. We're currently avoiding the use of a condition
variable as that causes the upload thread to yield and the gpu thread to wake up
every time an upload completes. It's a side effect of how we're setting thread
priorities.

So just avoid using a WaitableEvent for each upload. You can use a WaitableEvent
to signal that all uploads are complete and the upload thread is going idle. I
think that's good enough for this first version of WaitAsyncTexImage2D.

[Sami, 2013-03-04 19:53:49]

On 2013/03/04 19:32:58, David Reveman wrote:
> I'm afraid that using a WaitableEvent for completion in the android version
will
> cause a performance regression. We're currently avoiding the use of a
condition
> variable as that causes the upload thread to yield and the gpu thread to wake
up
> every time an upload completes. It's a side effect of how we're setting thread
> priorities.

I see. Do you know if that's the case also when there's no-one waiting on the
WaitableEvent? If there is someone waiting, then I think we'd actually prefer
for the upload thread to go to sleep and let the GPU thread start rendering,
right?

[reveman, 2013-03-04 20:35:56]

On 2013/03/04 19:53:49, Sami wrote:
> On 2013/03/04 19:32:58, David Reveman wrote:
> > I'm afraid that using a WaitableEvent for completion in the android version
> will
> > cause a performance regression. We're currently avoiding the use of a
> condition
> > variable as that causes the upload thread to yield and the gpu thread to
wake
> up
> > every time an upload completes. It's a side effect of how we're setting
thread
> > priorities.
> 
> I see. Do you know if that's the case also when there's no-one waiting on the
> WaitableEvent? If there is someone waiting, then I think we'd actually prefer
> for the upload thread to go to sleep and let the GPU thread start rendering,
> right?

That's a very good point. This patch should work well and is also a nice
cleanup!

[reveman, 2013-03-04 20:40:32]

Gregg, this is a minor addition to the CHROMIUM_async_pixel_transfers extension
that is needed to improve impl-side painting latency. Please have a look.

[greggman, 2013-03-04 21:25:52]

lgtm

https://codereview.chromium.org/12210129/diff/19001/gpu/command_buffer/client...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/12210129/diff/19001/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:3444: GPU_CLIENT_LOG("[" <<
GetLogPrefix() << "] glWaitAsyncTexImage2DCHROMIUM()");
log the target with GLES2Util::GetStringTextureTarget(target)?

See line 3405

https://codereview.chromium.org/12210129/diff/19001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/12210129/diff/19001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10175:
async_pixel_transfer_delegate_->WaitForTransferCompletion(
Are you sure you want to wait here vs checking if it's okay to proceed and
returning error::kDeferCommandUntilLater if it's not. As it is this will block
the GPU process where as error::kDeferCommandUntilLater will let the GPU process
process other command buffers.

[Sami, 2013-03-05 12:25:26]

https://codereview.chromium.org/12210129/diff/19001/gpu/command_buffer/client...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/12210129/diff/19001/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:3444: GPU_CLIENT_LOG("[" <<
GetLogPrefix() << "] glWaitAsyncTexImage2DCHROMIUM()");
On 2013/03/04 21:25:53, greggman wrote:
> log the target with GLES2Util::GetStringTextureTarget(target)?
> 
> See line 3405

Well spotted, fixed.

https://codereview.chromium.org/12210129/diff/19001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/12210129/diff/19001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10175:
async_pixel_transfer_delegate_->WaitForTransferCompletion(
On 2013/03/04 21:25:53, greggman wrote:
> Are you sure you want to wait here vs checking if it's okay to proceed and
> returning error::kDeferCommandUntilLater if it's not. As it is this will block
> the GPU process where as error::kDeferCommandUntilLater will let the GPU
process
> process other command buffers.

Yeah, we talked about doing this but shied away for now since we weren't exactly
sure how to make this avoid blocking other contexts. I guess the problem with
kDeferCommandUntilLater is that if there aren't any pending commands the GPU
thread will busy-loop on this same command, right? I can experiment with this
and do a follow-up patch if I find a way to do without blocking everyone.

FWIW, in my tests this call ends up blocking only very rarely, because the
uploads generally finish sooner than we end up needing them. The benefit is that
we can submit the rendering commands sooner and avoid a bubble in the pipeline.

[commit-bot: I haz the power, 2013-03-05 12:33:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/epenner@chromium.org/12210129/25001

[commit-bot: I haz the power, 2013-03-05 17:02:18]

Change committed as 186190

[epenner, 2013-03-08 02:35:50]

On 2013/03/05 17:02:18, I haz the power (commit-bot) wrote:
> Change committed as 186190

LGTM
Thanks for running with this Sami!

Can you describe how we benefit from waiting for a particular upload instead of
all of them? It seems like we will always wait on the last upload in practice,
or if not then we'll just end up waiting on it next frame?

The WaitableEvent per upload is fine I guess but it's implemented with Locks
which is too bad. Maybe I'm paranoid but I don't like the upload thread holding
locks which can block the GPU thread.

[reveman, 2013-03-08 03:42:01]

On 2013/03/08 02:35:50, epenner wrote:
> On 2013/03/05 17:02:18, I haz the power (commit-bot) wrote:
> > Change committed as 186190
> 
> LGTM
> Thanks for running with this Sami!
> 
> Can you describe how we benefit from waiting for a particular upload instead
of
> all of them? It seems like we will always wait on the last upload in practice,
> or if not then we'll just end up waiting on it next frame?

the per upload has less value for the threaded android implementation but is
really useful and easy to take advantage of in the idle uploads implemented by:
https://codereview.chromium.org/12040049/

we might want to stop using a message loop in the threaded android version so we
can take full advantage of this per-upload optimization in the future.

> 
> The WaitableEvent per upload is fine I guess but it's implemented with Locks
> which is too bad. Maybe I'm paranoid but I don't like the upload thread
holding
> locks which can block the GPU thread.

the os should handle these locks just fine. if it doesn't, we're already in
trouble as the upload thread will acquire a lock every time it takes a message
from the message loop.

[Sami, 2013-03-08 11:26:58]

On 2013/03/08 03:42:01, David Reveman wrote:
> we might want to stop using a message loop in the threaded android version so
we
> can take full advantage of this per-upload optimization in the future.

Right, a queue of pending uploads (much like in your WorkerPool refactor) would
allow us to move the waited upload to the front and actually have it complete
sooner.
 
> > The WaitableEvent per upload is fine I guess but it's implemented with Locks
> > which is too bad. Maybe I'm paranoid but I don't like the upload thread
> holding
> > locks which can block the GPU thread.
> 
> the os should handle these locks just fine. if it doesn't, we're already in
> trouble as the upload thread will acquire a lock every time it takes a message
> from the message loop.

The alternative to blocking the GPU thread would be to have it poll for
completion regularly which I think is worse because it would steal CPU time from
the upload.

Here's a crazy idea: have the upload thread estimate when the upload will
complete (it'll be roughly deterministic for glTexSubimage2D) and let the GPU
process serve other contexts until then. However in the cases I've looked at the
other contexts are mostly idle while we wait.

[epenner, 2013-03-08 19:45:27]

Regarding locks, maybe I'm going overboard on that. My thinking was ideally all
locks would go away eventually (including the message loop ones), but it's
probably not worth worrying about.

Okay I understand the idea for the waits now. Thanks for the explanation! It
seems a bit tricky to get it right in the threaded version (non waited-on
textures could slip through between the waits on the main thread).

Could we perhaps provide a list of textures to block on instead?

Also, it seems like its getting good enough to avoid a big re-architecture (to
reduce latency of uploads by avoiding the GL pipeline). Wdyt?