Ping watchdog thread during GpuChannel destruction

Ping watchdog thread during GpuChannel destruction

Currently GpuChannel destruction runs as a single large task in the GPU
process. This can cause watchdog timeouts on very busy systems, as we
can end up doing a huge number of GL commands in a single task.

This change adds logic to ping the watchdog thread at regular intervals
(assuming that destruction is moving forward, and hasn't actually hung).
This should prevent the timeout hangs we're seeing.

In addition to reporting progress in ContextGroup, we also report during
shader, program, and texture deletion, as these are the three cases
where we've actually seen timeouts.

BUG=612219
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/8767b2b934ea9f992399430f0ac78da93c1c9f4b
Cr-Commit-Position: refs/heads/master@{#423751}

[ericrk, 2016-09-28 19:07:20]

Description was changed from

==========
Ping watchdog thread during GpuChannel destruction

Currently GpuChannel destruction runs as a single large task in the GPU
process. This can cause watchdog timeouts on very busy systems, as we
can end up doing a huge number of GL commands in a single task.

This change adds logic to ping the watchdog thread at regular intervals
(assuming that destruction is moving forward, and hasn't actually hung).
This should prevent the timeout hangs we're seeing.

BUG=612219
==========

to

==========
Ping watchdog thread during GpuChannel destruction

Currently GpuChannel destruction runs as a single large task in the GPU
process. This can cause watchdog timeouts on very busy systems, as we
can end up doing a huge number of GL commands in a single task.

This change adds logic to ping the watchdog thread at regular intervals
(assuming that destruction is moving forward, and hasn't actually hung).
This should prevent the timeout hangs we're seeing.

BUG=612219
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[ericrk, 2016-09-28 19:07:26]

The CQ bit was checked by ericrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 19:07:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 19:11:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-28 19:11:19]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[ericrk, 2016-09-28 22:50:52]

The CQ bit was checked by ericrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 22:51:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 23:09:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-28 23:09:09]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ericrk, 2016-09-29 20:39:49]

The CQ bit was checked by ericrk@chromium.org to run a CQ dry run

[ericrk, 2016-09-29 20:40:09]

Patchset #4 (id:60001) has been deleted

[ericrk, 2016-09-29 20:40:14]

Patchset #3 (id:40001) has been deleted

[commit-bot: I haz the power, 2016-09-29 20:40:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ericrk, 2016-09-29 20:40:19]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-09-29 20:51:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-29 20:51:06]

Dry run: Try jobs failed on following builders:
  linux_optional_gpu_tests_rel on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_optional_...)

[ericrk, 2016-09-29 21:13:19]

Patchset #2 (id:80001) has been deleted

[ericrk, 2016-09-29 21:19:17]

Patchset #2 (id:100001) has been deleted

[ericrk, 2016-09-29 21:19:26]

The CQ bit was checked by ericrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 21:19:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ericrk, 2016-09-29 21:24:28]

Description was changed from

==========
Ping watchdog thread during GpuChannel destruction

Currently GpuChannel destruction runs as a single large task in the GPU
process. This can cause watchdog timeouts on very busy systems, as we
can end up doing a huge number of GL commands in a single task.

This change adds logic to ping the watchdog thread at regular intervals
(assuming that destruction is moving forward, and hasn't actually hung).
This should prevent the timeout hangs we're seeing.

BUG=612219
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Ping watchdog thread during GpuChannel destruction

Currently GpuChannel destruction runs as a single large task in the GPU
process. This can cause watchdog timeouts on very busy systems, as we
can end up doing a huge number of GL commands in a single task.

This change adds logic to ping the watchdog thread at regular intervals
(assuming that destruction is moving forward, and hasn't actually hung).
This should prevent the timeout hangs we're seeing.

In addition to reporting progress in ContextGroup, we also report during
shader, program, and texture deletion, as these are the three cases
where we've actually seen timeouts.

BUG=612219
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[ericrk, 2016-09-29 21:33:50]

The CQ bit was checked by ericrk@chromium.org to run a CQ dry run

[ericrk, 2016-09-29 21:34:06]

Patchset #3 (id:140001) has been deleted

[commit-bot: I haz the power, 2016-09-29 21:34:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ericrk, 2016-09-29 21:37:10]

Patchset #2 (id:120001) has been deleted

[ericrk, 2016-09-29 21:45:22]

Patchset #3 (id:180001) has been deleted

[ericrk, 2016-09-29 21:52:22]

The CQ bit was checked by ericrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 21:53:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 22:39:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-29 22:39:19]

Dry run: This issue passed the CQ dry run.

[ericrk, 2016-09-30 05:03:54]

Patchset #1 (id:1) has been deleted

[ericrk, 2016-09-30 05:04:02]

Patchset #1 (id:160001) has been deleted

[ericrk, 2016-09-30 05:04:12]

Patchset #1 (id:200001) has been deleted

[ericrk, 2016-09-30 05:09:06]

ericrk@chromium.org changed reviewers:
+ danakj@chromium.org, vmiura@chromium.org

[ericrk, 2016-09-30 05:09:07]

WDYT about this approach? I wonder if I should leave this out of
ProgramManager/TextureManager/ShaderManager and just handle this in ContextGroup
to start (and see how this affects crash rate)?

However, we've seen cases where program deletion itself can take >10s, so that
wouldn't catch all crashes...

https://codereview.chromium.org/2378583003/diff/70019/gpu/command_buffer/serv...
File gpu/command_buffer/service/context_group.cc (right):

https://codereview.chromium.org/2378583003/diff/70019/gpu/command_buffer/serv...
gpu/command_buffer/service/context_group.cc:507: ReportProgress();
Not sure if I'm too trigger happy with ReportProgress here - I do have a limit
in the implementation to fire at most 1 task per second, so should be safe to
call this a lot.

[vmiura, 2016-09-30 18:24:35]

https://codereview.chromium.org/2378583003/diff/70019/gpu/ipc/service/gpu_com...
File gpu/ipc/service/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/2378583003/diff/70019/gpu/ipc/service/gpu_com...
gpu/ipc/service/gpu_command_buffer_stub.cc:117: // CheckArmed posts a task.
Throttle this so we don't post more than
Rather than adding throttling here, it seems we could make
GpuWatchdogThread::PostAcknowledge() keep track if it posted a task
(acknowledged_ flag) so that it doesn't post OnAcknowledge repeatedly.

The flag would be cleared when the check was re-armed.

OnAcknowledge() earlies out anyway if the check isn't armed so I don't know why
we didn't just do this.

[ericrk, 2016-10-01 06:19:27]

https://codereview.chromium.org/2378583003/diff/70019/gpu/ipc/service/gpu_com...
File gpu/ipc/service/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/2378583003/diff/70019/gpu/ipc/service/gpu_com...
gpu/ipc/service/gpu_command_buffer_stub.cc:117: // CheckArmed posts a task.
Throttle this so we don't post more than
On 2016/09/30 18:24:35, vmiura wrote:
> Rather than adding throttling here, it seems we could make
> GpuWatchdogThread::PostAcknowledge() keep track if it posted a task
> (acknowledged_ flag) so that it doesn't post OnAcknowledge repeatedly.
> 
> The flag would be cleared when the check was re-armed.
> 
> OnAcknowledge() earlies out anyway if the check isn't armed so I don't know
why
> we didn't just do this.

Good point - I don't want to overload this CL, so I've removed the throttling
here and am adding the GpuWatchdogThread tweaks in a new CL.

[ericrk, 2016-10-01 06:19:34]

The CQ bit was checked by ericrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-01 06:19:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-01 07:03:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-01 07:04:00]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[ericrk, 2016-10-03 20:07:02]

The CQ bit was checked by ericrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-03 20:07:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-03 20:56:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-03 20:56:30]

Dry run: This issue passed the CQ dry run.

[ericrk, 2016-10-04 18:33:36]

friendly ping :D

[vmiura, 2016-10-04 21:22:24]

Minor request: Move progress_reporter to be a member of TextureManager and
ShaderManager instead of passing it in the Destroy() methods.

[danakj, 2016-10-05 01:53:49]

https://codereview.chromium.org/2378583003/diff/230001/gpu/command_buffer/ser...
File gpu/command_buffer/service/context_group.cc (right):

https://codereview.chromium.org/2378583003/diff/230001/gpu/command_buffer/ser...
gpu/command_buffer/service/context_group.cc:488: if (progress_reporter_)
When is it null? This looks like "null in tests" which is my bane. Can you make
a StubProgressReporter and use that in tests?

https://codereview.chromium.org/2378583003/diff/230001/gpu/command_buffer/ser...
File gpu/command_buffer/service/program_manager.cc (right):

https://codereview.chromium.org/2378583003/diff/230001/gpu/command_buffer/ser...
gpu/command_buffer/service/program_manager.cc:2486: if (progress_reporter)
same, when is it null?

https://codereview.chromium.org/2378583003/diff/230001/gpu/command_buffer/ser...
File gpu/command_buffer/service/progress_reporter.h (right):

https://codereview.chromium.org/2378583003/diff/230001/gpu/command_buffer/ser...
gpu/command_buffer/service/progress_reporter.h:17: ProgressReporter() = default;
no constructor needed

https://codereview.chromium.org/2378583003/diff/230001/gpu/command_buffer/ser...
gpu/command_buffer/service/progress_reporter.h:23:
DISALLOW_COPY_AND_ASSIGN(ProgressReporter);
you dont need this for interfaces IMO

https://codereview.chromium.org/2378583003/diff/230001/gpu/command_buffer/ser...
File gpu/command_buffer/service/shader_manager.cc (right):

https://codereview.chromium.org/2378583003/diff/230001/gpu/command_buffer/ser...
gpu/command_buffer/service/shader_manager.cc:285: if (progress_reporter)
same, when is it null?

https://codereview.chromium.org/2378583003/diff/230001/gpu/command_buffer/ser...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/2378583003/diff/230001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.cc:369: if (progress_reporter)
same same

https://codereview.chromium.org/2378583003/diff/230001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.cc:374: if (progress_reporter)
ame

https://codereview.chromium.org/2378583003/diff/230001/gpu/ipc/service/gpu_co...
File gpu/ipc/service/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/2378583003/diff/230001/gpu/ipc/service/gpu_co...
gpu/ipc/service/gpu_command_buffer_stub.cc:116: if (auto* watchdog =
channel_->watchdog()) {
when is it null?

https://codereview.chromium.org/2378583003/diff/230001/gpu/ipc/service/gpu_co...
gpu/ipc/service/gpu_command_buffer_stub.cc:492:
base::MakeUnique<GpuCommandBufferProgressReporter>(channel_));
I was surprised at the ownership model here. I would normally expect this
relationship to be: GpuCommandBufferStub is/owns a ProgressReporter and passes a
raw pointer here. Why'd you go with a memory alloc/uniqueptr/passing ownership
of a raw pointer (the channel_) back here?

[ericrk, 2016-10-05 05:16:13]

Patchset #3 (id:250001) has been deleted

[ericrk, 2016-10-05 17:20:42]

Patchset #3 (id:270001) has been deleted

[ericrk, 2016-10-05 17:49:16]

Patchset #3 (id:290001) has been deleted

[ericrk, 2016-10-05 18:04:39]

Patchset #3 (id:310001) has been deleted

[ericrk, 2016-10-05 18:07:32]

Thanks for the feedback. Updated.

https://codereview.chromium.org/2378583003/diff/230001/gpu/command_buffer/ser...
File gpu/command_buffer/service/context_group.cc (right):

https://codereview.chromium.org/2378583003/diff/230001/gpu/command_buffer/ser...
gpu/command_buffer/service/context_group.cc:488: if (progress_reporter_)
On 2016/10/05 01:53:48, danakj wrote:
> When is it null? This looks like "null in tests" which is my bane. Can you
make
> a StubProgressReporter and use that in tests?

Yeah, makes sense. It's actually null in one other case (in proc command
buffer), but that seems fine to use a stub for as well.

https://codereview.chromium.org/2378583003/diff/230001/gpu/command_buffer/ser...
File gpu/command_buffer/service/program_manager.cc (right):

https://codereview.chromium.org/2378583003/diff/230001/gpu/command_buffer/ser...
gpu/command_buffer/service/program_manager.cc:2486: if (progress_reporter)
On 2016/10/05 01:53:48, danakj wrote:
> same, when is it null?
refactored/removed.

https://codereview.chromium.org/2378583003/diff/230001/gpu/command_buffer/ser...
File gpu/command_buffer/service/progress_reporter.h (right):

https://codereview.chromium.org/2378583003/diff/230001/gpu/command_buffer/ser...
gpu/command_buffer/service/progress_reporter.h:17: ProgressReporter() = default;
On 2016/10/05 01:53:48, danakj wrote:
> no constructor needed

ah, yeah, removed.

https://codereview.chromium.org/2378583003/diff/230001/gpu/command_buffer/ser...
gpu/command_buffer/service/progress_reporter.h:23:
DISALLOW_COPY_AND_ASSIGN(ProgressReporter);
On 2016/10/05 01:53:48, danakj wrote:
> you dont need this for interfaces IMO

Done.

https://codereview.chromium.org/2378583003/diff/230001/gpu/command_buffer/ser...
File gpu/command_buffer/service/shader_manager.cc (right):

https://codereview.chromium.org/2378583003/diff/230001/gpu/command_buffer/ser...
gpu/command_buffer/service/shader_manager.cc:285: if (progress_reporter)
On 2016/10/05 01:53:48, danakj wrote:
> same, when is it null?

removed.

https://codereview.chromium.org/2378583003/diff/230001/gpu/command_buffer/ser...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/2378583003/diff/230001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.cc:369: if (progress_reporter)
On 2016/10/05 01:53:48, danakj wrote:
> same same

removed

https://codereview.chromium.org/2378583003/diff/230001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.cc:374: if (progress_reporter)
On 2016/10/05 01:53:48, danakj wrote:
> ame

emoved

https://codereview.chromium.org/2378583003/diff/230001/gpu/ipc/service/gpu_co...
File gpu/ipc/service/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/2378583003/diff/230001/gpu/ipc/service/gpu_co...
gpu/ipc/service/gpu_command_buffer_stub.cc:116: if (auto* watchdog =
channel_->watchdog()) {
On 2016/10/05 01:53:48, danakj wrote:
> when is it null?

added stub and removed.

https://codereview.chromium.org/2378583003/diff/230001/gpu/ipc/service/gpu_co...
gpu/ipc/service/gpu_command_buffer_stub.cc:492:
base::MakeUnique<GpuCommandBufferProgressReporter>(channel_));
On 2016/10/05 01:53:48, danakj wrote:
> I was surprised at the ownership model here. I would normally expect this
> relationship to be: GpuCommandBufferStub is/owns a ProgressReporter and passes
a
> raw pointer here. Why'd you go with a memory alloc/uniqueptr/passing ownership
> of a raw pointer (the channel_) back here?

I agree, this was a weird decision - I was originally thinking this reduced the
modifications to stub, but in retrospect this didn't really pan out and makes
the ownership / lifetime less clear. Made the stub a ProgressReporter, which
makes things cleaner.

https://codereview.chromium.org/2378583003/diff/330001/gpu/command_buffer/ser...
File gpu/command_buffer/service/progress_reporter_stub.h (right):

https://codereview.chromium.org/2378583003/diff/330001/gpu/command_buffer/ser...
gpu/command_buffer/service/progress_reporter_stub.h:14: class
ProgressReporterStub : public ProgressReporter {
I sort of hate making this its own file (so small), but it was used across a lot
of places :/

[ericrk, 2016-10-05 18:29:52]

The CQ bit was checked by ericrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-05 18:30:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ericrk, 2016-10-05 18:53:22]

On 2016/10/04 at 21:22:24, vmiura wrote:
> Minor request: Move progress_reporter to be a member of TextureManager and
ShaderManager instead of passing it in the Destroy() methods.

Is this in case we want to use this in more places within
Texture/Shader/ProgramManager in the future?

I sort of like keeping this as destroy only for the time being - it makes it
clear when this will be used and what it's lifetime is. We can always move this
to a class member in the future if we want to use it more places. WDYT?

[commit-bot: I haz the power, 2016-10-05 18:58:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-05 18:58:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ericrk, 2016-10-05 19:32:55]

Patchset #5 (id:370001) has been deleted

[ericrk, 2016-10-05 19:35:33]

Ok, moved back to passing ProgressReporter via Destroy for
Texture/Shader/ProgramManager. I think I like this a bit better (lifetime is
simpler to manage). vmiura@, let me know if this is OK w/ you.

[vmiura, 2016-10-05 19:56:24]

On 2016/10/05 19:35:33, ericrk wrote:
> Ok, moved back to passing ProgressReporter via Destroy for
> Texture/Shader/ProgramManager. I think I like this a bit better (lifetime is
> simpler to manage). vmiura@, let me know if this is OK w/ you.

I prefer the former, since as soon as we decide we need this in other methods it
will make sense to change it back.

I guess I'm not quite seeing the improvement from this change... it looks like
more code after the last change.

[danakj, 2016-10-05 21:36:16]

LGTM % vmiura

https://codereview.chromium.org/2378583003/diff/390001/gpu/ipc/service/gpu_co...
File gpu/ipc/service/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/2378583003/diff/390001/gpu/ipc/service/gpu_co...
gpu/ipc/service/gpu_command_buffer_stub.cc:478: gmb_factory ?
gmb_factory->AsImageFactory() : nullptr, this);
Can you /* progress_reporter */ on the |this|

https://codereview.chromium.org/2378583003/diff/390001/gpu/ipc/service/gpu_co...
File gpu/ipc/service/gpu_command_buffer_stub.h (right):

https://codereview.chromium.org/2378583003/diff/390001/gpu/ipc/service/gpu_co...
gpu/ipc/service/gpu_command_buffer_stub.h:93: // ProgressReporter implementation
period, or.. colon based on similar comments here.

[ericrk, 2016-10-05 22:36:17]

https://codereview.chromium.org/2378583003/diff/390001/gpu/ipc/service/gpu_co...
File gpu/ipc/service/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/2378583003/diff/390001/gpu/ipc/service/gpu_co...
gpu/ipc/service/gpu_command_buffer_stub.cc:478: gmb_factory ?
gmb_factory->AsImageFactory() : nullptr, this);
On 2016/10/05 21:36:15, danakj wrote:
> Can you /* progress_reporter */ on the |this|

Done.

https://codereview.chromium.org/2378583003/diff/390001/gpu/ipc/service/gpu_co...
File gpu/ipc/service/gpu_command_buffer_stub.h (right):

https://codereview.chromium.org/2378583003/diff/390001/gpu/ipc/service/gpu_co...
gpu/ipc/service/gpu_command_buffer_stub.h:93: // ProgressReporter implementation
On 2016/10/05 21:36:15, danakj wrote:
> period, or.. colon based on similar comments here.

Done.

[ericrk, 2016-10-05 22:37:08]

On 2016/10/05 19:56:24, vmiura wrote:
> On 2016/10/05 19:35:33, ericrk wrote:
> > Ok, moved back to passing ProgressReporter via Destroy for
> > Texture/Shader/ProgramManager. I think I like this a bit better (lifetime is
> > simpler to manage). vmiura@, let me know if this is OK w/ you.
> 
> I prefer the former, since as soon as we decide we need this in other methods
it
> will make sense to change it back.
> 
> I guess I'm not quite seeing the improvement from this change... it looks like
> more code after the last change.

Ok, sounds good, I went back to patch set 4 (passing via constructor), with
Dana's new comments addressed.

[ericrk, 2016-10-05 22:37:22]

The CQ bit was checked by ericrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-05 22:38:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vmiura, 2016-10-05 23:25:54]

On 2016/10/05 22:37:08, ericrk wrote:
> On 2016/10/05 19:56:24, vmiura wrote:
> > On 2016/10/05 19:35:33, ericrk wrote:
> > > Ok, moved back to passing ProgressReporter via Destroy for
> > > Texture/Shader/ProgramManager. I think I like this a bit better (lifetime
is
> > > simpler to manage). vmiura@, let me know if this is OK w/ you.
> > 
> > I prefer the former, since as soon as we decide we need this in other
methods
> it
> > will make sense to change it back.
> > 
> > I guess I'm not quite seeing the improvement from this change... it looks
like
> > more code after the last change.
> 
> Ok, sounds good, I went back to patch set 4 (passing via constructor), with
> Dana's new comments addressed.

LGTM, thanks.

[commit-bot: I haz the power, 2016-10-05 23:45:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-05 23:45:44]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[ericrk, 2016-10-06 00:33:48]

The CQ bit was checked by ericrk@chromium.org

[ericrk, 2016-10-06 00:33:49]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
vmiura@chromium.org
Link to the patchset: https://codereview.chromium.org/2378583003/#ps430001
(title: "add GPU_EXPORT to ProgressReporter")

[commit-bot: I haz the power, 2016-10-06 00:34:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-06 01:43:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-06 01:43:15]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[ericrk, 2016-10-06 17:32:21]

The CQ bit was checked by ericrk@chromium.org

[commit-bot: I haz the power, 2016-10-06 17:32:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-06 18:57:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-06 18:57:45]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ericrk, 2016-10-06 21:26:52]

Patchset #8 (id:440001) has been deleted

[ericrk, 2016-10-06 21:41:34]

Turns out the lifetime of objects in my last patch was wrong.

GpuWatchdogThread (if it exists), always outlives the GpuChannelManager,
GpuChannel, Stubs, and ContextGroups, so no reason for any of these objects to
be involved. Instead, we implement ProgressReporter on the GpuWatchdogThread and
pass this directly to the ContextGroup.

Given that we don't have a watchdog thread in in-process gpu mode, we now accept
nullptr (rather than creating a stub) for ProgressReporter.

[ericrk, 2016-10-06 21:42:58]

The CQ bit was checked by ericrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-06 21:43:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2016-10-06 21:49:54]

LGTM

https://codereview.chromium.org/2378583003/diff/460001/gpu/command_buffer/ser...
File gpu/command_buffer/service/context_group.cc (right):

https://codereview.chromium.org/2378583003/diff/460001/gpu/command_buffer/ser...
gpu/command_buffer/service/context_group.cc:551: ReportProgress();
nit: This one seems unneeded, when you're leaving the function the next task
will run right away?

You've placed them at the end of each step but really conceptually they are
before each step to say "I did things and I might sleep for a while now but dont
worry"

[danakj, 2016-10-06 21:51:02]

https://codereview.chromium.org/2378583003/diff/460001/gpu/command_buffer/ser...
File gpu/command_buffer/service/context_group.cc (right):

https://codereview.chromium.org/2378583003/diff/460001/gpu/command_buffer/ser...
gpu/command_buffer/service/context_group.cc:551: ReportProgress();
On 2016/10/06 21:49:54, danakj wrote:
> nit: This one seems unneeded, when you're leaving the function the next task
> will run right away?
> 
> You've placed them at the end of each step but really conceptually they are
> before each step to say "I did things and I might sleep for a while now but
dont
> worry"

Or maybe it's worth keeping to chain with other functions cuz you're putting
them after work in each case actually so ignore me.

[ericrk, 2016-10-06 21:51:55]

The CQ bit was unchecked by ericrk@chromium.org

[ericrk, 2016-10-06 21:51:55]

The CQ bit was checked by ericrk@chromium.org

[ericrk, 2016-10-06 21:51:56]

The patchset sent to the CQ was uploaded after l-g-t-m from vmiura@chromium.org
Link to the patchset: https://codereview.chromium.org/2378583003/#ps460001
(title: "Fix lifetime and use nullptr")

[commit-bot: I haz the power, 2016-10-06 21:52:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-06 23:26:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-06 23:26:29]

Try jobs failed on following builders:
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[ericrk, 2016-10-06 23:31:18]

The CQ bit was checked by ericrk@chromium.org

[commit-bot: I haz the power, 2016-10-06 23:32:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-07 00:11:07]

Description was changed from

==========
Ping watchdog thread during GpuChannel destruction

Currently GpuChannel destruction runs as a single large task in the GPU
process. This can cause watchdog timeouts on very busy systems, as we
can end up doing a huge number of GL commands in a single task.

This change adds logic to ping the watchdog thread at regular intervals
(assuming that destruction is moving forward, and hasn't actually hung).
This should prevent the timeout hangs we're seeing.

In addition to reporting progress in ContextGroup, we also report during
shader, program, and texture deletion, as these are the three cases
where we've actually seen timeouts.

BUG=612219
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Ping watchdog thread during GpuChannel destruction

Currently GpuChannel destruction runs as a single large task in the GPU
process. This can cause watchdog timeouts on very busy systems, as we
can end up doing a huge number of GL commands in a single task.

This change adds logic to ping the watchdog thread at regular intervals
(assuming that destruction is moving forward, and hasn't actually hung).
This should prevent the timeout hangs we're seeing.

In addition to reporting progress in ContextGroup, we also report during
shader, program, and texture deletion, as these are the three cases
where we've actually seen timeouts.

BUG=612219
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-10-07 00:11:10]

Committed patchset #8 (id:460001)

[commit-bot: I haz the power, 2016-10-07 00:14:10]

Description was changed from

==========
Ping watchdog thread during GpuChannel destruction

Currently GpuChannel destruction runs as a single large task in the GPU
process. This can cause watchdog timeouts on very busy systems, as we
can end up doing a huge number of GL commands in a single task.

This change adds logic to ping the watchdog thread at regular intervals
(assuming that destruction is moving forward, and hasn't actually hung).
This should prevent the timeout hangs we're seeing.

In addition to reporting progress in ContextGroup, we also report during
shader, program, and texture deletion, as these are the three cases
where we've actually seen timeouts.

BUG=612219
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Ping watchdog thread during GpuChannel destruction

Currently GpuChannel destruction runs as a single large task in the GPU
process. This can cause watchdog timeouts on very busy systems, as we
can end up doing a huge number of GL commands in a single task.

This change adds logic to ping the watchdog thread at regular intervals
(assuming that destruction is moving forward, and hasn't actually hung).
This should prevent the timeout hangs we're seeing.

In addition to reporting progress in ContextGroup, we also report during
shader, program, and texture deletion, as these are the three cases
where we've actually seen timeouts.

BUG=612219
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/8767b2b934ea9f992399430f0ac78da93c1c9f4b
Cr-Commit-Position: refs/heads/master@{#423751}
==========

[commit-bot: I haz the power, 2016-10-07 00:14:12]

Patchset 8 (id:??) landed as
https://crrev.com/8767b2b934ea9f992399430f0ac78da93c1c9f4b
Cr-Commit-Position: refs/heads/master@{#423751}