Issue 2650383007: Move Task Scheduler Single Thread Task Runners to Dedicated Threads

robliao

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

3 years, 11 months ago (2017-01-27 01:48:09 UTC) #1

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-27 01:49:08 UTC) #2

robliao

robliao@chromium.org changed reviewers: + fdoray@chromium.org, gab@chromium.org

3 years, 11 months ago (2017-01-27 01:57:31 UTC) #3

robliao

Here's the idea we discussed for single threaded task runners with adjustments as I ran ...

3 years, 11 months ago (2017-01-27 01:57:32 UTC) #4

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-27 02:26:17 UTC) #5

commit-bot: I haz the power

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_linux/builds/299303)

3 years, 11 months ago (2017-01-27 02:26:18 UTC) #6

robliao

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

3 years, 10 months ago (2017-01-27 06:26:00 UTC) #7

commit-bot: I haz the power

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

3 years, 10 months ago (2017-01-27 06:26:22 UTC) #8

commit-bot: I haz the power

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

3 years, 10 months ago (2017-01-27 08:10:14 UTC) #10

commit-bot: I haz the power

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

3 years, 10 months ago (2017-01-27 08:10:15 UTC) #11

robliao

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

3 years, 10 months ago (2017-01-27 08:53:58 UTC) #12

commit-bot: I haz the power

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

3 years, 10 months ago (2017-01-27 08:54:13 UTC) #13

robliao

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

3 years, 10 months ago (2017-01-27 09:37:28 UTC) #15

commit-bot: I haz the power

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

3 years, 10 months ago (2017-01-27 09:37:38 UTC) #17

commit-bot: I haz the power

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

3 years, 10 months ago (2017-01-27 10:59:41 UTC) #18

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 10 months ago (2017-01-27 10:59:43 UTC) #19

fdoray

https://codereview.chromium.org/2650383007/diff/60001/base/task_scheduler/scheduler_single_thread_worker_pool_manager.cc File base/task_scheduler/scheduler_single_thread_worker_pool_manager.cc (right): https://codereview.chromium.org/2650383007/diff/60001/base/task_scheduler/scheduler_single_thread_worker_pool_manager.cc#newcode42 base/task_scheduler/scheduler_single_thread_worker_pool_manager.cc:42: void JoinForTesting() { Set a |join_for_testing_called_| flag and DCHECK ...

3 years, 10 months ago (2017-01-27 16:47:35 UTC) #20

robliao

Thanks for the comments! https://codereview.chromium.org/2650383007/diff/60001/base/task_scheduler/scheduler_single_thread_worker_pool_manager.cc File base/task_scheduler/scheduler_single_thread_worker_pool_manager.cc (right): https://codereview.chromium.org/2650383007/diff/60001/base/task_scheduler/scheduler_single_thread_worker_pool_manager.cc#newcode42 base/task_scheduler/scheduler_single_thread_worker_pool_manager.cc:42: void JoinForTesting() { On 2017/01/27 ...

3 years, 10 months ago (2017-01-27 21:25:41 UTC) #23

Thanks for the comments!

https://codereview.chromium.org/2650383007/diff/60001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_single_thread_worker_pool_manager.cc (right):

https://codereview.chromium.org/2650383007/diff/60001/base/task_scheduler/sch...
base/task_scheduler/scheduler_single_thread_worker_pool_manager.cc:42: void
JoinForTesting() {
On 2017/01/27 16:47:35, fdoray wrote:
> Set a |join_for_testing_called_| flag and DCHECK that this flag isn't set in
> CreateSingleThreadTaskRunnerWithTraits()? Same flag that you could use to
> address this comment
>
https://codereview.chromium.org/2650383007/diff/60001/base/task_scheduler/tas...

I'm going to make worker pool cleanup best-effort for the *ForTesting calls.
That should simplify things here.

https://codereview.chromium.org/2650383007/diff/60001/base/task_scheduler/sch...
base/task_scheduler/scheduler_single_thread_worker_pool_manager.cc:44: // move
|worker_pools_| locally, request the detachment and join, and then
On 2017/01/27 16:47:35, fdoray wrote:
> s/request the detachment/disallow detachment/

Done.

https://codereview.chromium.org/2650383007/diff/60001/base/task_scheduler/sch...
base/task_scheduler/scheduler_single_thread_worker_pool_manager.cc:86:
num_worker_pools = worker_pools_.size();
On 2017/01/27 16:47:34, fdoray wrote:
> A concurrent call to GetAvailableOrCreateSchedulerWorkerPool() may obtain the
> same |num_worker_pools|. You could maintain a separate counter to number
pools.

Nice catch! Indeed! Fixed.

https://codereview.chromium.org/2650383007/diff/60001/base/task_scheduler/sch...
base/task_scheduler/scheduler_single_thread_worker_pool_manager.cc:88: size_t
index = free_worker_pool_indexes_.back();
On 2017/01/27 16:47:35, fdoray wrote:
> Optional:
> 
> We create SingleThreadTaskRunners for browser threads. Those
> SingleThreadTaskRunners will remain alive for most of the lifetime of the
> browser process. Apart from that, we plan to discourage usage of
> CreateSingleThreadTaskRunnerWithTraits(). That means that
> |free_worker_pool_indexes_| will probably always be empty when this method is
> called.
> 
> Knowing that, maybe we shouldn't try to reuse single-threaded worker pools? It
> would simplify this CL. It would also make it very easy to add a |name|
argument
> to base::CreateSingleThreadTaskRunnerWithTraits (Whenever you call
> CreateSingleThreadTaskRunnerWithTraits, a thread is created with a name of
your
> choice. This thread is not reused for other purposes.)
> 
> WDYT?

While skipping the cleanup code would keep things simplier, I would like to
avoid a repeat of the TLS work retrofit I had to because TLS didn't implement
cleaning up (as freeing TLS is rare in production code but common in test code).

https://codereview.chromium.org/2650383007/diff/60001/base/task_scheduler/sch...
base/task_scheduler/scheduler_single_thread_worker_pool_manager.cc:102: //
|single_thread_worker_pools_lock_|.
On 2017/01/27 16:47:34, fdoray wrote:
> s/single_thread_worker_pools_lock_/lock_/

Done.

https://codereview.chromium.org/2650383007/diff/60001/base/task_scheduler/sch...
base/task_scheduler/scheduler_single_thread_worker_pool_manager.cc:168:
AutoSchedulerLock auto_lock(lock_);
On 2017/01/27 16:47:34, fdoray wrote:
> No lock required. |worker_pool_impl_pools_| is only modified in the
constructor,
> before any other thread can access it.

While it is an error to call JoinForTesting() or Wait*ForTesting() during
CreateSingleThreadTaskRunnerWithTraits we still need to lock here for
correctness as those *ForTesting() methods mutate worker_pool_impl_pool_.

https://codereview.chromium.org/2650383007/diff/60001/base/task_scheduler/sch...
base/task_scheduler/scheduler_single_thread_worker_pool_manager.cc:181:
AutoSchedulerLock auto_lock(lock_);
On 2017/01/27 16:47:34, fdoray wrote:
> No lock / moving to a local variable required. |worker_pool_impl_pools_| is
only
> modified in the constructor, before any other thread can access it.

The following line changes worker_pool_impl_pools_ and std::vector move doesn't
make any thread guarantees, does it?

https://codereview.chromium.org/2650383007/diff/60001/base/task_scheduler/sch...
base/task_scheduler/scheduler_single_thread_worker_pool_manager.cc:199:
AutoSchedulerLock auto_lock(lock_);
On 2017/01/27 16:47:35, fdoray wrote:
> No lock / moving to a local variable required. |worker_pool_impl_pools_| is
only
> modified in the constructor, before any other thread can access it.

See above.

https://codereview.chromium.org/2650383007/diff/60001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_single_thread_worker_pool_manager.h (right):

https://codereview.chromium.org/2650383007/diff/60001/base/task_scheduler/sch...
base/task_scheduler/scheduler_single_thread_worker_pool_manager.h:20: #include
"base/time/time.h"
On 2017/01/27 16:47:35, fdoray wrote:
> Not used

Done.

https://codereview.chromium.org/2650383007/diff/60001/base/task_scheduler/sch...
base/task_scheduler/scheduler_single_thread_worker_pool_manager.h:38: // to
create a single thread WorkerPool of the exact same threading environment.
On 2017/01/27 16:47:35, fdoray wrote:
> s/WorkerPool/SchedulerWorkerPoolImpl/

Done.

https://codereview.chromium.org/2650383007/diff/60001/base/task_scheduler/sch...
base/task_scheduler/scheduler_single_thread_worker_pool_manager.h:63: const
std::vector<SchedulerWorkerPoolParams> worker_pool_params_vector_;
On 2017/01/27 16:47:35, fdoray wrote:
> Not needed (only used in the constructor).

Done.

https://codereview.chromium.org/2650383007/diff/60001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker_pool_impl.cc (right):

https://codereview.chromium.org/2650383007/diff/60001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_pool_impl.cc:195: const
UnregisterSingleThreadWorkerPoolCallback
On 2017/01/27 16:47:35, fdoray wrote:
> Not needed. SchedulerSingleThreadTaskRunner can access
> worker_pool_->unregister_single_thread_worker_pool_callback_ directly.

Indeed! Done.

https://codereview.chromium.org/2650383007/diff/60001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_pool_impl.cc:301:
SchedulerWorkerPoolImpl::CreateSingleThreadWorkerPool(
On 2017/01/27 16:47:35, fdoray wrote:
> Forward Create() and CreateSingleThreadWorkerPool() to CreateInternal()?

I wasn't convinced that it would make things clearer since CreateInternal would
have to take the union of all arguments, but I've done it here as an experiment.
It does make one path for construction, which is nice, but at the same time,
construction is already simple.

https://codereview.chromium.org/2650383007/diff/60001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_pool_impl.cc:311: if
(worker_pool->Initialize(params, ReEnqueueSequenceCallback())) {
On 2017/01/27 16:47:35, fdoray wrote:
> no braces

Done.

https://codereview.chromium.org/2650383007/diff/60001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_pool_impl.cc:331:
DCHECK(!unregister_single_thread_worker_pool_callback_.is_null());
On 2017/01/27 16:47:35, fdoray wrote:
> Tip: You can write
> DCHECK(unregister_single_thread_worker_pool_callback_);
> https://cs.chromium.org/chromium/src/base/callback_internal.h?l=87

Nice! Done.

https://codereview.chromium.org/2650383007/diff/60001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_pool_impl.cc:643: outer_->workers_.size()
== 1) &&  // Single Thread Worker Pools
On 2017/01/27 16:47:35, fdoray wrote:
> Should be
> 
> ...
> // In a pool which runs sequenced and parallel tasks, always keep
> // on idle thread alive. In a pool which runs single-threaded tasks,
> // keep the thread alive only when there are active TaskRunners.
> (worker != outer_->PeekAtIdleWorkersStack() ||
>  (unregister_single_thread_worker_pool_callback_ &&
>   !subtle::NoBarrier_Load(&num_single_threaded_runners_))) &&
> outer_->CanWorkerDetachForTesting();
> 
> Note: (outer_->workers_.size() == 1) is true in pools with a single thread
that
> runs parallel/sequenced tasks
>
https://cs.chromium.org/chromium/src/content/renderer/render_process_impl.cc?...
> I think we want to keep the thread active in these pools.

Good point. Done.

https://codereview.chromium.org/2650383007/diff/60001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker_pool_impl.h (right):

https://codereview.chromium.org/2650383007/diff/60001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_pool_impl.h:129:
UnregisterSingleThreadWorkerPoolCallback
On 2017/01/27 16:47:35, fdoray wrote:
> Move after |params| to keep the same order as CreateSingleThreadWorkerPool().

The reason this is at the end is the previous constructor has the same set at
the beginning. It's a loose convention to keep the constructor argument order
consistent and add additional parameters at the end for overloading purposes.

Now that I look at this again, C++ default arguments can handle this case, so
I've gone with that.

https://codereview.chromium.org/2650383007/diff/60001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker_pool_impl_unittest.cc (right):

https://codereview.chromium.org/2650383007/diff/60001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:791: auto
other_task_runner =
On 2017/01/27 16:47:35, fdoray wrote:
> |other_task_runner| was used to change the order of workers on the idle stack
> and allow the worker associated with |task_runner| to detach. Now that a
worker
> in single-threaded pool is always allowed to detach even when it's on top of
the
> idle stack, you don't need |other_task_runner|. 

Removed!

https://codereview.chromium.org/2650383007/diff/60001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker_pool_params.h (right):

https://codereview.chromium.org/2650383007/diff/60001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_pool_params.h:10: #include "base/macros.h"
On 2017/01/27 16:47:35, fdoray wrote:
>  Not needed anymore.

Done.

https://codereview.chromium.org/2650383007/diff/60001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_pool_params.h:47:
SchedulerWorkerPoolParams(SchedulerWorkerPoolParams&& other);
On 2017/01/27 16:47:35, fdoray wrote:
> You can remove lines 47-48. These methods pretend to be a move constructor / a
> move assign operator but they just copy all members...

Done.

https://codereview.chromium.org/2650383007/diff/60001/base/task_scheduler/tas...
File base/task_scheduler/task_scheduler_impl_unittest.cc (right):

https://codereview.chromium.org/2650383007/diff/60001/base/task_scheduler/tas...
base/task_scheduler/task_scheduler_impl_unittest.cc:286:
scheduler_->FlushForTesting();
On 2017/01/27 16:47:35, fdoray wrote:
> Is the issue that this DCHECK can be hit
>
https://codereview.chromium.org/2650383007/diff/60001/base/task_scheduler/sch...
> if a SingleThreadTaskRunner is destroyed during
> SchedulerSingleThreadWorkerPoolManager::JoinForTesting()?
> 
> If so, could you instead add a |join_for_testing_called_| flag in
> SchedulerSingleThreadWorkerPoolManager and return immediately in
> UnregisterSingleThreadWorkerPool() when this flag is set? That way, we won't
> have to think about this issue every time we join TaskScheduler.

sgtm. Done. Added a reinforcing comment in task_scheduler.h to say that after
JoinForTesting() returns, that's the end for that task scheduler instance.

robliao

Description was changed from ========== Move Task Scheduler Single Thread Task Runners to Dedicated Threads ...

3 years, 10 months ago (2017-02-01 23:57:18 UTC) #24

robliao

3 years, 10 months ago (2017-02-01 23:57:58 UTC) #25

Message was sent while issue was closed.

Closing for now. We're going to go ahead and table the LA design and do this the
right way.

Issue 2650383007: Move Task Scheduler Single Thread Task Runners to Dedicated Threads (Closed)

Description

Patch Set 1 #

Patch Set 2 : CR Feedback #

Messages