TaskScheduler: Always remove SchedulerWorker from idle stack on wake up.

TaskScheduler: Always remove SchedulerWorker from idle stack on wake up.

With this CL, posting a SINGLE_THREADED task removes the target
SchedulerWorker from the idle stack.

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

[fdoray, 2016-10-13 14:22:55]

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

[fdoray, 2016-10-13 14:22:55]

PTAL

[robliao, 2016-10-13 17:38:41]

Thanks for the small fixes coming through!

https://codereview.chromium.org/2414063002/diff/1/base/task_scheduler/schedul...
File base/task_scheduler/scheduler_worker_pool_impl.h (right):

https://codereview.chromium.org/2414063002/diff/1/base/task_scheduler/schedul...
base/task_scheduler/scheduler_worker_pool_impl.h:117: // Wakes up |worker| and
removes it from the idle stack.
Leave both of these comments like they were before. The idle stack is an
implementation detail.

Instead, I would discuss the invariant that the idle stack only contains idle
workers below.

[fdoray, 2016-10-13 18:46:55]

PTAnL

https://codereview.chromium.org/2414063002/diff/1/base/task_scheduler/schedul...
File base/task_scheduler/scheduler_worker_pool_impl.h (right):

https://codereview.chromium.org/2414063002/diff/1/base/task_scheduler/schedul...
base/task_scheduler/scheduler_worker_pool_impl.h:117: // Wakes up |worker| and
removes it from the idle stack.
On 2016/10/13 17:38:40, robliao wrote:
> Leave both of these comments like they were before. The idle stack is an
> implementation detail.
> 
> Instead, I would discuss the invariant that the idle stack only contains idle
> workers below.

Done.

[robliao, 2016-10-13 19:02:20]

lgtm

[fdoray, 2016-10-13 19:02:58]

The CQ bit was checked by fdoray@chromium.org

[commit-bot: I haz the power, 2016-10-13 19:03: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-13 19:52:12]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-13 19:53:55]

Description was changed from

==========
TaskScheduler: Always remove SchedulerWorker from idle stack on wake up.

With this CL, posting a SINGLE_THREADED task removes the target
SchedulerWorker from the idle stack.

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

to

==========
TaskScheduler: Always remove SchedulerWorker from idle stack on wake up.

With this CL, posting a SINGLE_THREADED task removes the target
SchedulerWorker from the idle stack.

BUG=553459

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

[commit-bot: I haz the power, 2016-10-13 19:53:56]

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

[gab, 2016-10-17 16:53:21]

gab@chromium.org changed reviewers:
+ gab@chromium.org

[gab, 2016-10-17 16:53:21]

lgtm because it's better than before but there's still a race where the same
worker could be posted SINGLE_THREADED work in parallel with being chosen to
process a SEQUENCED/PARALLEL task (the stack is locked during the operation
itself so there is no data corruption but the thread is still assigned more than
one task while other threads are sleeping).

It might be overkill to try and fix this but one solution is something like:
return a bool from RemoveFromIdleWorkersStack() to know whether the worker was
currently idle and then to see if the SINGLE_THREADED task is next in priority
with pending work behind it (if so, wake up another thread).

[fdoray, 2016-10-17 17:23:13]

On 2016/10/17 16:53:21, gab (OOO) wrote:
> lgtm because it's better than before but there's still a race where the same
> worker could be posted SINGLE_THREADED work in parallel with being chosen to
> process a SEQUENCED/PARALLEL task (the stack is locked during the operation
> itself so there is no data corruption but the thread is still assigned more
than
> one task while other threads are sleeping).
> 
> It might be overkill to try and fix this but one solution is something like:
> return a bool from RemoveFromIdleWorkersStack() to know whether the worker was
> currently idle and then to see if the SINGLE_THREADED task is next in priority
> with pending work behind it (if so, wake up another thread).

It makes sense to wake up another worker when a SINGLE_THREADED task is posted
to a worker that has been selected to run a SEQUENCED/PARALLEL task but hasn't
started to run it yet (after that it's too late). I would be curious to know how
often that occurs (shouldn't happen often if time from wake up to Run() is
short).

[gab, 2016-10-17 18:33:43]

On 2016/10/17 17:23:13, fdoray wrote:
> On 2016/10/17 16:53:21, gab (OOO) wrote:
> > lgtm because it's better than before but there's still a race where the same
> > worker could be posted SINGLE_THREADED work in parallel with being chosen to
> > process a SEQUENCED/PARALLEL task (the stack is locked during the operation
> > itself so there is no data corruption but the thread is still assigned more
> than
> > one task while other threads are sleeping).
> > 
> > It might be overkill to try and fix this but one solution is something like:
> > return a bool from RemoveFromIdleWorkersStack() to know whether the worker
was
> > currently idle and then to see if the SINGLE_THREADED task is next in
priority
> > with pending work behind it (if so, wake up another thread).
> 
> It makes sense to wake up another worker when a SINGLE_THREADED task is posted
> to a worker that has been selected to run a SEQUENCED/PARALLEL task but hasn't
> started to run it yet (after that it's too late). I would be curious to know
how
> often that occurs (shouldn't happen often if time from wake up to Run() is
> short).

I agree that it probably won't happen often. How do we infer if it does? Our
current latency histograms might be helpful but not until we have
SINGLE_THREADED work sent to the scheduler.

In a world where we eventually decide to "assign" multiple tasks to a single
worker (e.g. if it turns out that NumTasksBetweenWaits is too low) it would
still be nice to correctly account for the number of tasks "assigned" to a given
worker instead of just saying "meh +/- 1".

It might actually happen often in the initial redirection world where over half
of the work will be SINGLE_THREADED.