TaskScheduler: Pop a Task from its Sequence from SchedulerWorkerThread.

TaskScheduler: Pop a Task from its Sequence from SchedulerWorkerThread.

Previously, we planned to pop a Task from its Sequence inside the
RanTaskFromSequence() method of SchedulerWorkerThread::Delegate. This
method would also have pushed the Sequence to the appropriate
PriorityQueue if it wasn't empty after the pop.

We now believe that it is more natural to pop the Task from its Sequence
from SchedulerWorkerThread, right after it runs. If the Sequence isn't
empty after the pop, SchedulerWorkerThread will call
SchedulerWorkerThread::Delegate::EnqueueSequence(). It won't notify  its
delegate if the Sequence is empty after the pop.

BUG=553459
Committed: https://crrev.com/d314e56ae71bfb19fa9d1745055936cc870652e1
Cr-Commit-Position: refs/heads/master@{#386171}

[fdoray, 2016-04-07 21:20:52]

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

[fdoray, 2016-04-07 21:20:52]

robliao@/gab@: Can you review this CL? Thanks.

[gab, 2016-04-07 21:37:33]

mini-comment, will take a look at test changes tomorrow.

Cheers,
Gab

https://codereview.chromium.org/1862243005/diff/1/base/task_scheduler/schedul...
File base/task_scheduler/scheduler_worker_thread.cc (right):

https://codereview.chromium.org/1862243005/diff/1/base/task_scheduler/schedul...
base/task_scheduler/scheduler_worker_thread.cc:78: const bool
sequence_became_empty = sequence->PopTask();
In other places you have comments explaining why this atomic operation always
leads to the correct state (i.e. either we made it atomically empty and the next
Add() will be in charge of re-inserting or there was a task behind our pop and
we're in charge of re-inserting).
Think it's worth re-iterating the algorithmic guarantees relied on here?

[gab, 2016-04-08 15:32:17]

lgtm w/ two test nits (and previous comment)

Thanks!

https://codereview.chromium.org/1862243005/diff/1/base/task_scheduler/schedul...
File base/task_scheduler/scheduler_worker_thread_unittest.cc (right):

https://codereview.chromium.org/1862243005/diff/1/base/task_scheduler/schedul...
base/task_scheduler/scheduler_worker_thread_unittest.cc:149: void
EnqueueSequence(scoped_refptr<Sequence> sequence) override {
Add a comment above stating what this specific override does/tests (it's
otherwise quite unexpected to see EnqueueSequence() pop tasks.

https://codereview.chromium.org/1862243005/diff/1/base/task_scheduler/schedul...
base/task_scheduler/scheduler_worker_thread_unittest.cc:220: // nullptr.
"and wait until the GetWork() call that returns nullptr", it's not obvious from
the code below that it does that, since the nullptr thing is already explained
on line 214, maybe here something like this is sufficient here: "and wait until
GetWork() has been invoked the expected amount of times"

Same comment on line 251.

[fdoray, 2016-04-08 15:59:12]

https://codereview.chromium.org/1862243005/diff/1/base/task_scheduler/schedul...
File base/task_scheduler/scheduler_worker_thread.cc (right):

https://codereview.chromium.org/1862243005/diff/1/base/task_scheduler/schedul...
base/task_scheduler/scheduler_worker_thread.cc:78: const bool
sequence_became_empty = sequence->PopTask();
On 2016/04/07 21:37:33, gab wrote:
> In other places you have comments explaining why this atomic operation always
> leads to the correct state (i.e. either we made it atomically empty and the
next
> Add() will be in charge of re-inserting or there was a task behind our pop and
> we're in charge of re-inserting).
> Think it's worth re-iterating the algorithmic guarantees relied on here?

Done.

https://codereview.chromium.org/1862243005/diff/1/base/task_scheduler/schedul...
File base/task_scheduler/scheduler_worker_thread_unittest.cc (right):

https://codereview.chromium.org/1862243005/diff/1/base/task_scheduler/schedul...
base/task_scheduler/scheduler_worker_thread_unittest.cc:149: void
EnqueueSequence(scoped_refptr<Sequence> sequence) override {
On 2016/04/08 15:32:17, gab wrote:
> Add a comment above stating what this specific override does/tests (it's
> otherwise quite unexpected to see EnqueueSequence() pop tasks.

Done.

https://codereview.chromium.org/1862243005/diff/1/base/task_scheduler/schedul...
base/task_scheduler/scheduler_worker_thread_unittest.cc:220: // nullptr.
On 2016/04/08 15:32:17, gab wrote:
> "and wait until the GetWork() call that returns nullptr", it's not obvious
from
> the code below that it does that, since the nullptr thing is already explained
> on line 214, maybe here something like this is sufficient here: "and wait
until
> GetWork() has been invoked the expected amount of times"
> 
> Same comment on line 251.

Done.

[robliao, 2016-04-08 17:21:39]

Some nits

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

https://codereview.chromium.org/1862243005/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_thread.cc:83: // will be enqueued when a
Task is added to it.
Nit: Technically, for the empty case...
If it is empty and there are live references to it, it will be enqueued when a
Task is added to it. Otherwise, the sequence will be destroyed at the end of
this scope.

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

https://codereview.chromium.org/1862243005/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_thread_unittest.cc:65: void
SetMaxGetWork(size_t max_get_work) {
This should probably be
set_max_get_work

https://codereview.chromium.org/1862243005/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_thread_unittest.cc:76: size_t NumRunTasks()
{
Similarly
num_run_tasks

https://codereview.chromium.org/1862243005/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_thread_unittest.cc:150: // Tasks and adds
it to |enqueued_sequences_|. It doesn't put |sequence| in a
Instead of "It doesn't put |sequence| in a ..."
Maybe
Unlike a normal EnqueueSequence implementation, it doesn't reinsert |sequence|
into a queue for further execution.

https://codereview.chromium.org/1862243005/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_thread_unittest.cc:277:
::testing::Values(2));
Should we have a test case with some arbitrary large number, like 18 or 42?

[fdoray, 2016-04-08 17:49:13]

robliao@: Can you take another look? Thanks.

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

https://codereview.chromium.org/1862243005/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_thread.cc:83: // will be enqueued when a
Task is added to it.
On 2016/04/08 17:21:39, robliao wrote:
> Nit: Technically, for the empty case...
> If it is empty and there are live references to it, it will be enqueued when a
> Task is added to it. Otherwise, the sequence will be destroyed at the end of
> this scope.

Done.

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

https://codereview.chromium.org/1862243005/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_thread_unittest.cc:65: void
SetMaxGetWork(size_t max_get_work) {
On 2016/04/08 17:21:39, robliao wrote:
> This should probably be
> set_max_get_work

Dana asked me to use CamelCase when I acquire a lock
https://codereview.chromium.org/1704113002/diff/280001/base/task_scheduler/wo...

https://codereview.chromium.org/1862243005/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_thread_unittest.cc:76: size_t NumRunTasks()
{
On 2016/04/08 17:21:39, robliao wrote:
> Similarly
> num_run_tasks

ditto

https://codereview.chromium.org/1862243005/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_thread_unittest.cc:150: // Tasks and adds
it to |enqueued_sequences_|. It doesn't put |sequence| in a
On 2016/04/08 17:21:39, robliao wrote:
> Instead of "It doesn't put |sequence| in a ..."
> Maybe
> Unlike a normal EnqueueSequence implementation, it doesn't reinsert |sequence|
> into a queue for further execution.

Done.

https://codereview.chromium.org/1862243005/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_thread_unittest.cc:277:
::testing::Values(2));
On 2016/04/08 17:21:39, robliao wrote:
> Should we have a test case with some arbitrary large number, like 18 or 42?

The only reason difference between One/TwoTaskPerSequence is that
TwoTaskPerSequence calls EnqueueSequence while OneTaskPerSequence doesn't.
Adding a test case with a large number wouldn't test anything new.

[robliao, 2016-04-08 17:56:43]

lgtm

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

https://codereview.chromium.org/1862243005/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_thread_unittest.cc:65: void
SetMaxGetWork(size_t max_get_work) {
On 2016/04/08 17:49:13, fdoray wrote:
> On 2016/04/08 17:21:39, robliao wrote:
> > This should probably be
> > set_max_get_work
> 
> Dana asked me to use CamelCase when I acquire a lock
>
https://codereview.chromium.org/1704113002/diff/280001/base/task_scheduler/wo...

Ah, I missed that.

[fdoray, 2016-04-08 17:58:00]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2016-04-08 17:58:01]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org
Link to the patchset: https://codereview.chromium.org/1862243005/#ps40001
(title: "CR robliao #6 (improve comments)")

[commit-bot: I haz the power, 2016-04-08 17:58:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1862243005/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1862243005/40001

[commit-bot: I haz the power, 2016-04-08 19:05:34]

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

[commit-bot: I haz the power, 2016-04-08 19:05:35]

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

[fdoray, 2016-04-08 19:29:35]

The CQ bit was checked by fdoray@chromium.org

[commit-bot: I haz the power, 2016-04-08 19:29:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1862243005/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1862243005/40001

[commit-bot: I haz the power, 2016-04-08 19:58:00]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-08 20:01:51]

Description was changed from

==========
TaskScheduler: Pop a Task from its Sequence from SchedulerWorkerThread.

Previously, we planned to pop a Task from its Sequence inside the
RanTaskFromSequence() method of SchedulerWorkerThread::Delegate. This
method would also have pushed the Sequence to the appropriate
PriorityQueue if it wasn't empty after the pop.

We now believe that it is more natural to pop the Task from its Sequence
from SchedulerWorkerThread, right after it runs. If the Sequence isn't
empty after the pop, SchedulerWorkerThread will call
SchedulerWorkerThread::Delegate::EnqueueSequence(). It won't notify  its
delegate if the Sequence is empty after the pop.

BUG=553459
==========

to

==========
TaskScheduler: Pop a Task from its Sequence from SchedulerWorkerThread.

Previously, we planned to pop a Task from its Sequence inside the
RanTaskFromSequence() method of SchedulerWorkerThread::Delegate. This
method would also have pushed the Sequence to the appropriate
PriorityQueue if it wasn't empty after the pop.

We now believe that it is more natural to pop the Task from its Sequence
from SchedulerWorkerThread, right after it runs. If the Sequence isn't
empty after the pop, SchedulerWorkerThread will call
SchedulerWorkerThread::Delegate::EnqueueSequence(). It won't notify  its
delegate if the Sequence is empty after the pop.

BUG=553459

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

[commit-bot: I haz the power, 2016-04-08 20:01:52]

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