Provide TaskRunner to ContextCacheController

Provide TaskRunner to ContextCacheController

Both ContextCacheController and the CommandBuffer/GpuControl need
access to a TaskRunner. CommandBuffer/GpuControl currently creates this
TaskRunner internally in most cases. This change moves TaskRunner
creation to the ContextProvider, allowing the same TaskRunner to be
passed to both objects.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/d989e6a0815a6f77d3a6fbab24afe9ff6abbf25e
Cr-Commit-Position: refs/heads/master@{#419553}

[ericrk, 2016-08-26 21:01:38]

Description was changed from

==========
Provide TaskRunner to ContextCacheController

Both ContextCacheController and the CommandBuffer/GpuControl need
access to a TaskRunner. CommandBuffer/GpuControl currently creates this
TaskRunner internally in most cases. This change moves TaskRunner
creation to the ContextProvider, allowing the same TaskRunner to be
passed to both objects.
==========

to

==========
Provide TaskRunner to ContextCacheController

Both ContextCacheController and the CommandBuffer/GpuControl need
access to a TaskRunner. CommandBuffer/GpuControl currently creates this
TaskRunner internally in most cases. This change moves TaskRunner
creation to the ContextProvider, allowing the same TaskRunner to be
passed to both objects.
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-08-26 21:01:39]

Description was changed from

==========
Provide TaskRunner to ContextCacheController

Both ContextCacheController and the CommandBuffer/GpuControl need
access to a TaskRunner. CommandBuffer/GpuControl currently creates this
TaskRunner internally in most cases. This change moves TaskRunner
creation to the ContextProvider, allowing the same TaskRunner to be
passed to both objects.
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

==========
Provide TaskRunner to ContextCacheController

Both ContextCacheController and the CommandBuffer/GpuControl need
access to a TaskRunner. CommandBuffer/GpuControl currently creates this
TaskRunner internally in most cases. This change moves TaskRunner
creation to the ContextProvider, allowing the same TaskRunner to be
passed to both objects.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[ericrk, 2016-08-26 21:07:10]

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

[commit-bot: I haz the power, 2016-08-26 21:07:49]

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-08-26 21:26:45]

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

[commit-bot: I haz the power, 2016-08-26 21:26:47]

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

[ericrk, 2016-08-30 18:39:11]

Patchset #1 (id:1) has been deleted

[ericrk, 2016-08-30 20:54:44]

Patchset #1 (id:20001) has been deleted

[ericrk, 2016-08-30 20:54:51]

Patchset #1 (id:40001) has been deleted

[ericrk, 2016-08-30 20:54:57]

Patchset #1 (id:60001) has been deleted

[ericrk, 2016-08-30 20:55:04]

Patchset #1 (id:80001) has been deleted

[ericrk, 2016-08-30 21:28:14]

Patchset #2 (id:120001) has been deleted

[ericrk, 2016-08-30 21:33:25]

Patchset #2 (id:140001) has been deleted

[ericrk, 2016-08-30 21:36:11]

ericrk@chromium.org changed reviewers:
+ sievers@chromium.org

[ericrk, 2016-08-30 21:36:13]

https://codereview.chromium.org/2286873003/diff/160001/cc/output/context_cach...
File cc/output/context_cache_controller.h (right):

https://codereview.chromium.org/2286873003/diff/160001/cc/output/context_cach...
cc/output/context_cache_controller.h:74:
scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
This will be used in a follow-up CL.

[ericrk, 2016-08-31 20:53:04]

Patchset #1 (id:100001) has been deleted

[ericrk, 2016-08-31 20:53:54]

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

[ericrk, 2016-09-01 19:15:58]

friendly ping :D - sievers@, can you take a look?

[no sievers, 2016-09-02 19:47:33]

content + gpu lgtm

[ericrk, 2016-09-06 21:57:08]

ericrk@chromium.org changed reviewers:
+ boliu@chromium.org, msw@chromium.org, wez@chromium.org

[ericrk, 2016-09-06 21:57:09]

Adding additional owners for ContextProvider changes.

+wez for Blimp/
+msw for services/
+boliu for aw/

Per wez@'s comments on the previous CL I looked at whether we could refactor
ContextProvider into a Base impl + derived impl to reduce code duplication. We
*may* be able to, but there are a lot of small differences between these impls
and I'm not sure how clean it would be. I'll continue looking into this, but
didn't want to block this CL on that work.

[Wez, 2016-09-06 23:29:02]

https://codereview.chromium.org/2286873003/diff/180001/blimp/client/support/c...
File blimp/client/support/compositor/blimp_context_provider.cc (right):

https://codereview.chromium.org/2286873003/diff/180001/blimp/client/support/c...
blimp/client/support/compositor/blimp_context_provider.cc:46: auto task_runner =
base::ThreadTaskRunnerHandle::Get();
It's non-obvious what the type of |task_runner| will be here; I'd recommend
using scoped_refptr<TaskRunner> or similar, explicitly, or just using
base::ThreadTaskRunnerHandle::Get() directly as a parameter below.

https://codereview.chromium.org/2286873003/diff/180001/blimp/client/support/c...
blimp/client/support/compositor/blimp_context_provider.cc:55:
context_->GetImplementation(), std::move(task_runner)));
It looks strange to have one object passed |task_runner| via copy and the other
via std::move(), and all you really gain is one less ref-count inc/dec, so I'd
recommend just passing by-copy here.

[danakj, 2016-09-06 23:42:39]

https://codereview.chromium.org/2286873003/diff/180001/blimp/client/support/c...
File blimp/client/support/compositor/blimp_context_provider.cc (right):

https://codereview.chromium.org/2286873003/diff/180001/blimp/client/support/c...
blimp/client/support/compositor/blimp_context_provider.cc:55:
context_->GetImplementation(), std::move(task_runner)));
On 2016/09/06 23:29:02, Wez wrote:
> It looks strange to have one object passed |task_runner| via copy and the
other
> via std::move(), and all you really gain is one less ref-count inc/dec, so I'd
> recommend just passing by-copy here.

I have to disagree here, that's entirely the point of move() with a refptr,
you're giving up a ref you don't need anymore. Above you need to keep your ref,
but here you don't. What's strange?

[boliu, 2016-09-07 16:29:27]

lgtm

[ericrk, 2016-09-13 16:45:38]

Patchset #3 (id:200001) has been deleted

[ericrk, 2016-09-13 17:04:27]

https://codereview.chromium.org/2286873003/diff/180001/blimp/client/support/c...
File blimp/client/support/compositor/blimp_context_provider.cc (right):

https://codereview.chromium.org/2286873003/diff/180001/blimp/client/support/c...
blimp/client/support/compositor/blimp_context_provider.cc:46: auto task_runner =
base::ThreadTaskRunnerHandle::Get();
On 2016/09/06 23:29:02, Wez wrote:
> It's non-obvious what the type of |task_runner| will be here; I'd recommend
> using scoped_refptr<TaskRunner> or similar, explicitly, or just using
> base::ThreadTaskRunnerHandle::Get() directly as a parameter below.

I guess factoring this out wasn't getting us a huge amount... I'll just pass
::Get to the functions below.

https://codereview.chromium.org/2286873003/diff/180001/blimp/client/support/c...
blimp/client/support/compositor/blimp_context_provider.cc:55:
context_->GetImplementation(), std::move(task_runner)));
On 2016/09/06 23:42:38, danakj wrote:
> On 2016/09/06 23:29:02, Wez wrote:
> > It looks strange to have one object passed |task_runner| via copy and the
> other
> > via std::move(), and all you really gain is one less ref-count inc/dec, so
I'd
> > recommend just passing by-copy here.
> 
> I have to disagree here, that's entirely the point of move() with a refptr,
> you're giving up a ref you don't need anymore. Above you need to keep your
ref,
> but here you don't. What's strange?

I tend to agree w/ Dana, although I do see that this adds a bit of extra
complexity (having to worry about the lifetime of |task_runner|.)

That said, I took Wez's previous suggestion of just using
base::ThreadTaskRunnerHandle::Get directly (avoiding the temp), which sidesteps
this issue.

[Wez, 2016-09-13 20:04:47]

On 2016/09/13 17:04:27, ericrk - OOO Until 9-19 wrote:
[snip]
> That said, I took Wez's previous suggestion of just using
> base::ThreadTaskRunnerHandle::Get directly (avoiding the temp), which
sidesteps
> this issue.

Is that acceptable to Dana, given that it's going to be higher overhead than
just AddRef'ing or move()ing a scoped_refptr?

[Wez, 2016-09-13 20:05:21]

blimp/ LGTM

[danakj, 2016-09-13 21:37:08]

On Tue, Sep 13, 2016 at 1:04 PM, <wez@chromium.org> wrote:

> On 2016/09/13 17:04:27, ericrk - OOO Until 9-19 wrote:
> [snip]
> > That said, I took Wez's previous suggestion of just using
> > base::ThreadTaskRunnerHandle::Get directly (avoiding the temp), which
> sidesteps
> > this issue.
>
> Is that acceptable to Dana, given that it's going to be higher overhead
> than
> just AddRef'ing or move()ing a scoped_refptr?
>

It's ok this isn't particularly hot code or anything. I don't know that
it's generally best practice to call the same function twice from the same
place instead of cacheing locally but I just wanted to argue against being
anti-std::move not to bikeshed into oblivion.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ericrk, 2016-09-14 06:55:57]

msw@, friendly ping for review. Thanks!

[msw, 2016-09-14 17:07:05]

services/* rubber stamp lgtm.
+CC Robert, you'd be a better owner there than myself.

[ericrk, 2016-09-15 08:11:46]

The CQ bit was checked by ericrk@chromium.org

[ericrk, 2016-09-15 08:11:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
sievers@chromium.org, boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2286873003/#ps220001
(title: "feedback")

[commit-bot: I haz the power, 2016-09-15 08:12:00]

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-15 08:21:47]

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

[commit-bot: I haz the power, 2016-09-15 08:21:49]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[ericrk, 2016-09-18 13:00:43]

The CQ bit was checked by ericrk@chromium.org

[ericrk, 2016-09-18 13:00:44]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
sievers@chromium.org, wez@chromium.org, boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2286873003/#ps240001
(title: "rebase")

[commit-bot: I haz the power, 2016-09-18 13:00:52]

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-18 13:13:00]

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

[commit-bot: I haz the power, 2016-09-18 13:13:02]

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[ericrk, 2016-09-18 16:58:06]

The CQ bit was checked by ericrk@chromium.org

[ericrk, 2016-09-18 16:58:07]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
sievers@chromium.org, wez@chromium.org, boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2286873003/#ps260001
(title: "update param in gl_tests")

[commit-bot: I haz the power, 2016-09-18 16:58:16]

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-18 17:08:33]

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

[commit-bot: I haz the power, 2016-09-18 17:08:35]

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

[ericrk, 2016-09-18 22:17:07]

Patchset #5 (id:260001) has been deleted

[ericrk, 2016-09-18 22:20:13]

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

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

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-18 22:34:12]

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

[commit-bot: I haz the power, 2016-09-18 22:34:16]

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

[ericrk, 2016-09-18 22:46:35]

Patchset #5 (id:250020) has been deleted

[ericrk, 2016-09-18 22:48:40]

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

[commit-bot: I haz the power, 2016-09-18 22:48:53]

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-18 23:01:37]

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

[commit-bot: I haz the power, 2016-09-18 23:01:39]

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

[ericrk, 2016-09-18 23:20:26]

Patchset #5 (id:290001) has been deleted

[ericrk, 2016-09-18 23:20:33]

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

[commit-bot: I haz the power, 2016-09-18 23:20:43]

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-19 15:32:32]

Patchset #5 (id:310001) has been deleted

[ericrk, 2016-09-19 15:32:42]

Patchset #5 (id:330001) has been deleted

[ericrk, 2016-09-19 15:32:54]

The CQ bit was checked by ericrk@chromium.org

[ericrk, 2016-09-19 15:32:54]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
sievers@chromium.org, wez@chromium.org, boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2286873003/#ps350001
(title: "fix unittests")

[ericrk, 2016-09-19 15:32:55]

The CQ bit was unchecked by ericrk@chromium.org

[ericrk, 2016-09-19 15:32:58]

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

[commit-bot: I haz the power, 2016-09-19 15:33: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-19 16:32:12]

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

[commit-bot: I haz the power, 2016-09-19 16:32:13]

Dry run: This issue passed the CQ dry run.

[ericrk, 2016-09-19 17:37:33]

Had to add a few more files to get the build to pass - very minor changes,
largely adding "nullptr" to tests - danakj@, can you take a peak at the
remaining files?

[danakj, 2016-09-19 18:59:17]

https://codereview.chromium.org/2286873003/diff/350001/cc/output/context_cach...
File cc/output/context_cache_controller.h (right):

https://codereview.chromium.org/2286873003/diff/350001/cc/output/context_cach...
cc/output/context_cache_controller.h:74:
scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
Remind me why this is here? It's not used.

https://codereview.chromium.org/2286873003/diff/350001/ui/compositor/test/in_...
File ui/compositor/test/in_process_context_provider.cc (right):

https://codereview.chromium.org/2286873003/diff/350001/ui/compositor/test/in_...
ui/compositor/test/in_process_context_provider.cc:93: image_factory_, nullptr /*
task_runner */));
I think you should pass non-null cuz otherwise if a test does lose context in
future it'll just crash. Null was meant to be for the |surface| isn't null case
right?

[ericrk, 2016-09-19 20:00:10]

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

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

https://codereview.chromium.org/2286873003/diff/350001/cc/output/context_cach...
File cc/output/context_cache_controller.h (right):

https://codereview.chromium.org/2286873003/diff/350001/cc/output/context_cach...
cc/output/context_cache_controller.h:74:
scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
On 2016/09/19 18:59:17, danakj wrote:
> Remind me why this is here? It's not used.

I'm going to use it in a follow-up CL I'll send after this.

https://codereview.chromium.org/2286873003/diff/350001/ui/compositor/test/in_...
File ui/compositor/test/in_process_context_provider.cc (right):

https://codereview.chromium.org/2286873003/diff/350001/ui/compositor/test/in_...
ui/compositor/test/in_process_context_provider.cc:93: image_factory_, nullptr /*
task_runner */));
On 2016/09/19 18:59:17, danakj wrote:
> I think you should pass non-null cuz otherwise if a test does lose context in
> future it'll just crash. Null was meant to be for the |surface| isn't null
case
> right?

Yeah - fair enough - I was thinking this was fine as long as tests don't use it,
but you're right, it's quite brittle.

[commit-bot: I haz the power, 2016-09-19 20:01:01]

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

[danakj, 2016-09-19 20:03:25]

LGTM

https://codereview.chromium.org/2286873003/diff/370001/gpu/command_buffer/cli...
File gpu/command_buffer/client/gl_in_process_context.cc (right):

https://codereview.chromium.org/2286873003/diff/370001/gpu/command_buffer/cli...
gpu/command_buffer/client/gl_in_process_context.cc:193: if (surface.get()) {
no .get here either while u here?

https://codereview.chromium.org/2286873003/diff/370001/gpu/command_buffer/cli...
gpu/command_buffer/client/gl_in_process_context.cc:199:
DCHECK_EQ(!!surface.get(), !task_runner);
no .get()

https://codereview.chromium.org/2286873003/diff/370001/gpu/command_buffer/cli...
gpu/command_buffer/client/gl_in_process_context.cc:199:
DCHECK_EQ(!!surface.get(), !task_runner);
also nit: normally i like DCHECKs at the top of the function, as like invariants
on the arguments.

[ericrk, 2016-09-19 20:08:03]

https://codereview.chromium.org/2286873003/diff/370001/gpu/command_buffer/cli...
File gpu/command_buffer/client/gl_in_process_context.cc (right):

https://codereview.chromium.org/2286873003/diff/370001/gpu/command_buffer/cli...
gpu/command_buffer/client/gl_in_process_context.cc:193: if (surface.get()) {
On 2016/09/19 20:03:25, danakj wrote:
> no .get here either while u here?

Done.

https://codereview.chromium.org/2286873003/diff/370001/gpu/command_buffer/cli...
gpu/command_buffer/client/gl_in_process_context.cc:199:
DCHECK_EQ(!!surface.get(), !task_runner);
On 2016/09/19 20:03:25, danakj wrote:
> no .get()

Done.

[ericrk, 2016-09-19 20:08:16]

The CQ bit was checked by ericrk@chromium.org

[ericrk, 2016-09-19 20:08:17]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
sievers@chromium.org, wez@chromium.org, boliu@chromium.org, danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2286873003/#ps390001
(title: "feedback")

[commit-bot: I haz the power, 2016-09-19 20:09:01]

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-19 21:17:24]

Committed patchset #7 (id:390001)

[commit-bot: I haz the power, 2016-09-19 21:20:44]

Description was changed from

==========
Provide TaskRunner to ContextCacheController

Both ContextCacheController and the CommandBuffer/GpuControl need
access to a TaskRunner. CommandBuffer/GpuControl currently creates this
TaskRunner internally in most cases. This change moves TaskRunner
creation to the ContextProvider, allowing the same TaskRunner to be
passed to both objects.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Provide TaskRunner to ContextCacheController

Both ContextCacheController and the CommandBuffer/GpuControl need
access to a TaskRunner. CommandBuffer/GpuControl currently creates this
TaskRunner internally in most cases. This change moves TaskRunner
creation to the ContextProvider, allowing the same TaskRunner to be
passed to both objects.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/d989e6a0815a6f77d3a6fbab24afe9ff6abbf25e
Cr-Commit-Position: refs/heads/master@{#419553}
==========

[commit-bot: I haz the power, 2016-09-19 21:20:47]

Patchset 7 (id:??) landed as
https://crrev.com/d989e6a0815a6f77d3a6fbab24afe9ff6abbf25e
Cr-Commit-Position: refs/heads/master@{#419553}