Separate the create and start phases in SchedulerSingleThreadTaskRunnerManager.

Separate the create and start phases in SchedulerSingleThreadTaskRunnerManager.

With this CL, single-thread TaskRunners created from a newly
instantiated SchedulerSingleThreadTaskRunnerManager aren't backed
by a thread (and hence their tasks can't run right away). Thread
creation is delayed until
SchedulerSingleThreadTaskRunnerManager::Start().

BUG=690706
Review-Url: https://codereview.chromium.org/2806413002
Cr-Commit-Position: refs/heads/master@{#465439}
Committed: https://chromium.googlesource.com/chromium/src/+/b64df4b2b6feeebc0ae622fb2fc012cfa6b58c1a

[fdoray, 2017-04-10 17:38:51]

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

[commit-bot: I haz the power, 2017-04-10 17:39:12]

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

[fdoray, 2017-04-10 17:40:07]

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

[fdoray, 2017-04-10 17:40:08]

PTAL

[gab, 2017-04-10 18:04:56]

Nice

https://codereview.chromium.org/2806413002/diff/20001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_single_thread_task_runner_manager.h (right):

https://codereview.chromium.org/2806413002/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_single_thread_task_runner_manager.h:38: class
BASE_EXPORT SchedulerSingleThreadTaskRunnerManager final {
This class is missing a meta-comment, please work with robliao@ to add one +
mention its thread-safety properties (APIs below are thread-safe but this isn't
the default for classes in chrome)

https://codereview.chromium.org/2806413002/diff/20001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc
(right):

https://codereview.chromium.org/2806413002/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc:224:
started = true;
Need to use AtomicFlag or this a data race (no such thing as a "benign data
race":
https://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-coul...)

https://codereview.chromium.org/2806413002/diff/20001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker.cc (right):

https://codereview.chromium.org/2806413002/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker.cc:320: void
SchedulerWorker::CreateThread() {
Last caller of this is now CreateThreadAssertSynchronized(), merge into this one
and always assert lock acquired?

(even more so since |started_| isn't thread-safe)

https://codereview.chromium.org/2806413002/diff/20001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker.h (right):

https://codereview.chromium.org/2806413002/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker.h:116: // immediately. Otherwise, creation
is delayed until the next WakeUp().
"immediately (in a wait state pending a WakeUp() call)"

I was confused in the last CL before I recalled that ALIVE merely meant "create
the thread but still need to wake it", clarifying here seems worthwhile.

https://codereview.chromium.org/2806413002/diff/20001/base/task_scheduler/tas...
File base/task_scheduler/task_scheduler_impl.cc (right):

https://codereview.chromium.org/2806413002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_scheduler_impl.cc:243:
single_thread_task_runner_manager_->Start();
Just checking: does anything break down if we forget this line :)? i.e. do we
have a safe check against completely breaking integration?

[commit-bot: I haz the power, 2017-04-10 18:10:24]

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

[commit-bot: I haz the power, 2017-04-10 18:10:25]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[fdoray, 2017-04-10 19:01:02]

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

[commit-bot: I haz the power, 2017-04-10 19:02:08]

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

[fdoray, 2017-04-10 19:03:52]

The CQ bit was unchecked by fdoray@chromium.org

[fdoray, 2017-04-10 19:04:36]

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

[fdoray, 2017-04-10 19:05:01]

PTAnL

https://codereview.chromium.org/2806413002/diff/20001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_single_thread_task_runner_manager.h (right):

https://codereview.chromium.org/2806413002/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_single_thread_task_runner_manager.h:38: class
BASE_EXPORT SchedulerSingleThreadTaskRunnerManager final {
On 2017/04/10 18:04:56, gab wrote:
> This class is missing a meta-comment, please work with robliao@ to add one +
> mention its thread-safety properties (APIs below are thread-safe but this
isn't
> the default for classes in chrome)

Done.

https://codereview.chromium.org/2806413002/diff/20001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc
(right):

https://codereview.chromium.org/2806413002/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc:224:
started = true;
On 2017/04/10 18:04:56, gab wrote:
> Need to use AtomicFlag or this a data race (no such thing as a "benign data
> race":
>
https://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-coul...)

Done.

https://codereview.chromium.org/2806413002/diff/20001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker.cc (right):

https://codereview.chromium.org/2806413002/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker.cc:320: void
SchedulerWorker::CreateThread() {
On 2017/04/10 18:04:56, gab wrote:
> Last caller of this is now CreateThreadAssertSynchronized(), merge into this
one
> and always assert lock acquired?
> 
> (even more so since |started_| isn't thread-safe)

Done.

https://codereview.chromium.org/2806413002/diff/20001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker.h (right):

https://codereview.chromium.org/2806413002/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker.h:116: // immediately. Otherwise, creation
is delayed until the next WakeUp().
On 2017/04/10 18:04:56, gab wrote:
> "immediately (in a wait state pending a WakeUp() call)"
> 
> I was confused in the last CL before I recalled that ALIVE merely meant
"create
> the thread but still need to wake it", clarifying here seems worthwhile.

Done.

https://codereview.chromium.org/2806413002/diff/20001/base/task_scheduler/tas...
File base/task_scheduler/task_scheduler_impl.cc (right):

https://codereview.chromium.org/2806413002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_scheduler_impl.cc:243:
single_thread_task_runner_manager_->Start();
On 2017/04/10 18:04:56, gab wrote:
> Just checking: does anything break down if we forget this line :)? i.e. do we
> have a safe check against completely breaking integration?

TaskSchedulerImplTest.MultipleTraitsExecutionModePairs verifies that
single-threaded tasks are scheduled, so we can't miss this (I verified that the
test fails when we remove this line).

[commit-bot: I haz the power, 2017-04-10 19:05:16]

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

[gab, 2017-04-10 19:10:56]

lgtm

[commit-bot: I haz the power, 2017-04-10 19:35:47]

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

[commit-bot: I haz the power, 2017-04-10 19:35:48]

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_...)

[robliao, 2017-04-10 20:52:48]

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

https://codereview.chromium.org/2806413002/diff/60001/base/task_scheduler/sch...
base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc:117:
single_thread_task_runner_manager_->Start();
Given the majority of the tests start the task runner manager as the first
thing, this may make more sense in the helper class and then PostTaskBeforeStart
should be special cased.

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

https://codereview.chromium.org/2806413002/diff/60001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker.h:118: bool Start(InitialState
initial_state = InitialState::ALIVE);
Initial state seems better placed at the constructor. Start just allows this
object to start doing work. Callers then don't have to remember the initial
state after they've created the SchedulerWorker.

https://codereview.chromium.org/2806413002/diff/60001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker.h:120: /// Wakes up this SchedulerWorker if
it wasn't already awake. After this is
Remove extra leading slash.

[fdoray, 2017-04-11 15:27:13]

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

[fdoray, 2017-04-11 15:27:24]

PTAnL

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

https://codereview.chromium.org/2806413002/diff/60001/base/task_scheduler/sch...
base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc:117:
single_thread_task_runner_manager_->Start();
On 2017/04/10 20:52:48, robliao wrote:
> Given the majority of the tests start the task runner manager as the first
> thing, this may make more sense in the helper class and then
PostTaskBeforeStart
> should be special cased.

Done.

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

https://codereview.chromium.org/2806413002/diff/60001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker.h:118: bool Start(InitialState
initial_state = InitialState::ALIVE);
On 2017/04/10 20:52:48, robliao wrote:
> Initial state seems better placed at the constructor. Start just allows this
> object to start doing work. Callers then don't have to remember the initial
> state after they've created the SchedulerWorker.

Did you have in mind a solution that doesn't involve adding a |initial_state_|
member to SchedulerWorker?

https://codereview.chromium.org/2806413002/diff/60001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker.h:120: /// Wakes up this SchedulerWorker if
it wasn't already awake. After this is
On 2017/04/10 20:52:48, robliao wrote:
> Remove extra leading slash.

Done.

[commit-bot: I haz the power, 2017-04-11 15:27: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, 2017-04-11 16:03:36]

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

[commit-bot: I haz the power, 2017-04-11 16:03:37]

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

[fdoray, 2017-04-11 17:18:04]

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

[commit-bot: I haz the power, 2017-04-11 17:18: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, 2017-04-11 18:27:32]

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

[commit-bot: I haz the power, 2017-04-11 18:27:32]

Dry run: This issue passed the CQ dry run.

[robliao, 2017-04-13 16:42:30]

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

https://codereview.chromium.org/2806413002/diff/60001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker.h:118: bool Start(InitialState
initial_state = InitialState::ALIVE);
On 2017/04/11 15:27:24, fdoray wrote:
> On 2017/04/10 20:52:48, robliao wrote:
> > Initial state seems better placed at the constructor. Start just allows this
> > object to start doing work. Callers then don't have to remember the initial
> > state after they've created the SchedulerWorker.
> 
> Did you have in mind a solution that doesn't involve adding a |initial_state_|
> member to SchedulerWorker?

In this CL, we create the SchedulerWorker and then immediately start it in both
SchedulerWorkerPoolImpl and SchedulerSingleThreadTaskRunnerManager. Why can't
initial_state_ be in the constructor?

[fdoray, 2017-04-13 19:53:15]

ptanl

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

https://codereview.chromium.org/2806413002/diff/60001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker.h:118: bool Start(InitialState
initial_state = InitialState::ALIVE);
On 2017/04/13 16:42:30, robliao wrote:
> On 2017/04/11 15:27:24, fdoray wrote:
> > On 2017/04/10 20:52:48, robliao wrote:
> > > Initial state seems better placed at the constructor. Start just allows
this
> > > object to start doing work. Callers then don't have to remember the
initial
> > > state after they've created the SchedulerWorker.
> > 
> > Did you have in mind a solution that doesn't involve adding a
|initial_state_|
> > member to SchedulerWorker?
> 
> In this CL, we create the SchedulerWorker and then immediately start it in
both
> SchedulerWorkerPoolImpl and SchedulerSingleThreadTaskRunnerManager. Why can't
> initial_state_ be in the constructor?

No. SchedulerWorkers created by SchedulerSingleThreadTaskRunnerManager before
SchedulerSingleThreadTaskRunnerManager::Start() are started here
https://codereview.chromium.org/2806413002/diff/100001/base/task_scheduler/sc...
(not in the method from which they were instantiated).

[fdoray, 2017-04-18 12:02:40]

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

[commit-bot: I haz the power, 2017-04-18 12:02:59]

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, 2017-04-18 12:10:45]

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

[commit-bot: I haz the power, 2017-04-18 12:10:46]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[fdoray, 2017-04-18 12:28:53]

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

[commit-bot: I haz the power, 2017-04-18 12:29:15]

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

[fdoray, 2017-04-18 12:41:28]

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

[commit-bot: I haz the power, 2017-04-18 12:41:45]

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, 2017-04-18 13:50:11]

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

[commit-bot: I haz the power, 2017-04-18 13:50:12]

Dry run: This issue passed the CQ dry run.

[fdoray, 2017-04-18 14:58:03]

robliao: PTAnL

[fdoray, 2017-04-18 14:58:46]

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

[commit-bot: I haz the power, 2017-04-18 14:59: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, 2017-04-18 16:20:11]

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

[commit-bot: I haz the power, 2017-04-18 16:20:12]

Dry run: This issue passed the CQ dry run.

[robliao, 2017-04-18 21:08:02]

lgtm + nit. Thanks!

https://codereview.chromium.org/2806413002/diff/180001/base/task_scheduler/sc...
File base/task_scheduler/scheduler_worker.h (right):

https://codereview.chromium.org/2806413002/diff/180001/base/task_scheduler/sc...
base/task_scheduler/scheduler_worker.h:113: InitialState initial_state =
InitialState::ALIVE);
Nit: Add initial_state back into the docs.

[fdoray, 2017-04-18 23:03:11]

https://codereview.chromium.org/2806413002/diff/180001/base/task_scheduler/sc...
File base/task_scheduler/scheduler_worker.h (right):

https://codereview.chromium.org/2806413002/diff/180001/base/task_scheduler/sc...
base/task_scheduler/scheduler_worker.h:113: InitialState initial_state =
InitialState::ALIVE);
On 2017/04/18 21:08:02, robliao wrote:
> Nit: Add initial_state back into the docs.

Done.

[fdoray, 2017-04-18 23:04:52]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2017-04-18 23:04:53]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
robliao@chromium.org
Link to the patchset: https://codereview.chromium.org/2806413002/#ps200001
(title: "CR-robliao-45-initial-state-comment")

[commit-bot: I haz the power, 2017-04-18 23:05:32]

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, 2017-04-19 00:37:31]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1492556692926440,
"parent_rev": "4cb75df89a5fabfb4a83717cdc0f9289eb3b0596", "commit_rev":
"b64df4b2b6feeebc0ae622fb2fc012cfa6b58c1a"}

[commit-bot: I haz the power, 2017-04-19 00:37:40]

Description was changed from

==========
Separate the create and start phases in SchedulerSingleThreadTaskRunnerManager.

With this CL, single-thread TaskRunners created from a newly
instantiated SchedulerSingleThreadTaskRunnerManager aren't backed
by a thread (and hence their tasks can't run right away). Thread
creation is delayed until
SchedulerSingleThreadTaskRunnerManager::Start().

BUG=690706
==========

to

==========
Separate the create and start phases in SchedulerSingleThreadTaskRunnerManager.

With this CL, single-thread TaskRunners created from a newly
instantiated SchedulerSingleThreadTaskRunnerManager aren't backed
by a thread (and hence their tasks can't run right away). Thread
creation is delayed until
SchedulerSingleThreadTaskRunnerManager::Start().

BUG=690706

Review-Url: https://codereview.chromium.org/2806413002
Cr-Commit-Position: refs/heads/master@{#465439}
Committed:
https://chromium.googlesource.com/chromium/src/+/b64df4b2b6feeebc0ae622fb2fc0...
==========

[commit-bot: I haz the power, 2017-04-19 00:37:41]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/b64df4b2b6feeebc0ae622fb2fc0...