TaskScheduler: No BACKGROUND threads when unsupported.

TaskScheduler: No BACKGROUND threads when unsupported.

This CL ensures that no BACKGROUND threads are created when priority
inheritance is not available or when the priority of a thread can't be
increased after it has been created.

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

[fdoray, 2016-08-02 21:01:02]

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

[fdoray, 2016-08-02 21:01:02]

PTAL

[gab, 2016-08-03 13:35:24]

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

https://codereview.chromium.org/2208493002/diff/1/base/task_scheduler/schedul...
base/task_scheduler/scheduler_worker.cc:133: // SchedulerWorker's preferred
priority, current shutdown state and platform
oxford comma FTW (", and") :-)

https://codereview.chromium.org/2208493002/diff/1/base/task_scheduler/schedul...
base/task_scheduler/scheduler_worker.cc:147: #if defined(OS_POSIX)
Add a comment here why it's fine on Windows? This isn't captured in lock.h or
anywhere else AFAIK. Ideally this would be mentioned at a higher level than
here... I'd prefer for the call made here to not be OS_POSIX, maybe some kind of
Lock::IsThreadPrioritySafe()? which returns true on Windows (with reasoning) and
returns LockImpl::PriorityInheritanceAvailable() on POSIX)?

https://codereview.chromium.org/2208493002/diff/1/base/task_scheduler/schedul...
base/task_scheduler/scheduler_worker.cc:157: if (worker_priority_below_normal &&
So "worker priority" is just the priority hint right? Not the actual priority? I
guess I find this statement confusing because of that.. renaming it to
|priority_hint_| as suggested above will make that clearer IMO (and this bool to
something like |priority_hint_below_normal|).

https://codereview.chromium.org/2208493002/diff/1/base/task_scheduler/schedul...
base/task_scheduler/scheduler_worker.cc:162: return outer_->thread_priority_;
So basically the point of |worker_priority_below_normal| is to avoid returning
NORMAL when the hint is > NORMAL< right?

Would it be cleaner to instead start with

if (static_cast<int>(outer_->priority_hint_) >=
    static_cast<int>(ThreadPriority::NORMAL)) {
  return outer_->priority_hint_;
}

and then doing the rest of the logic assuming that the hint is < NORMAL?

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

https://codereview.chromium.org/2208493002/diff/1/base/task_scheduler/schedul...
base/task_scheduler/scheduler_worker.h:88: ThreadPriority thread_priority,
s/thread_priority/priority_hint/ or something?

[fdoray, 2016-08-03 18:42:49]

PTAnL

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

https://codereview.chromium.org/2208493002/diff/1/base/task_scheduler/schedul...
base/task_scheduler/scheduler_worker.cc:133: // SchedulerWorker's preferred
priority, current shutdown state and platform
On 2016/08/03 13:35:24, gab wrote:
> oxford comma FTW (", and") :-)

Done.

https://codereview.chromium.org/2208493002/diff/1/base/task_scheduler/schedul...
base/task_scheduler/scheduler_worker.cc:147: #if defined(OS_POSIX)
On 2016/08/03 13:35:24, gab wrote:
> Add a comment here why it's fine on Windows? This isn't captured in lock.h or
> anywhere else AFAIK. Ideally this would be mentioned at a higher level than
> here... I'd prefer for the call made here to not be OS_POSIX, maybe some kind
of
> Lock::IsThreadPrioritySafe()? which returns true on Windows (with reasoning)
and
> returns LockImpl::PriorityInheritanceAvailable() on POSIX)?

n/a now that we have HandlesMultipleThreadPriorities()

https://codereview.chromium.org/2208493002/diff/1/base/task_scheduler/schedul...
base/task_scheduler/scheduler_worker.cc:157: if (worker_priority_below_normal &&
On 2016/08/03 13:35:24, gab wrote:
> So "worker priority" is just the priority hint right? Not the actual priority?
I
> guess I find this statement confusing because of that.. renaming it to
> |priority_hint_| as suggested above will make that clearer IMO (and this bool
to
> something like |priority_hint_below_normal|).
Done

https://codereview.chromium.org/2208493002/diff/1/base/task_scheduler/schedul...
base/task_scheduler/scheduler_worker.cc:162: return outer_->thread_priority_;
On 2016/08/03 13:35:24, gab wrote:
> So basically the point of |worker_priority_below_normal| is to avoid returning
> NORMAL when the hint is > NORMAL< right?
> 
> Would it be cleaner to instead start with
> 
> if (static_cast<int>(outer_->priority_hint_) >=
>     static_cast<int>(ThreadPriority::NORMAL)) {
>   return outer_->priority_hint_;
> }
> 
> and then doing the rest of the logic assuming that the hint is < NORMAL?


|worker_priority_below_normal| is gone

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

https://codereview.chromium.org/2208493002/diff/1/base/task_scheduler/schedul...
base/task_scheduler/scheduler_worker.h:88: ThreadPriority thread_priority,
On 2016/08/03 13:35:24, gab wrote:
> s/thread_priority/priority_hint/ or something?

Done.

[gab, 2016-08-03 18:50:53]

lgtm

[robliao, 2016-08-03 23:53:21]

lgtm + nit

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

https://codereview.chromium.org/2208493002/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker.cc:132: // Returns the priority to which
the thread should be set based on the
Nit: Returns the priority for which the thread

[fdoray, 2016-08-04 20:43:14]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2016-08-04 20:43:20]

The CQ bit was unchecked by fdoray@chromium.org

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

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

https://codereview.chromium.org/2208493002/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker.cc:132: // Returns the priority to which
the thread should be set based on the
On 2016/08/03 23:53:20, robliao wrote:
> Nit: Returns the priority for which the thread

Done.

[fdoray, 2016-08-04 20:58:24]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2016-08-04 20:58:24]

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

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

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-04 22:56:20]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-04 22:58:16]

Description was changed from

==========
TaskScheduler: No BACKGROUND threads when unsupported.

This CL ensures that no BACKGROUND threads are created when priority
inheritance is not available or when the priority of a thread can't be
increased after it has been created.

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

to

==========
TaskScheduler: No BACKGROUND threads when unsupported.

This CL ensures that no BACKGROUND threads are created when priority
inheritance is not available or when the priority of a thread can't be
increased after it has been created.

BUG=553459

Committed: https://crrev.com/3ac29c0a1f10f882a5876fdf7a09da482f0743bb
Cr-Commit-Position: refs/heads/master@{#409923}
==========

[commit-bot: I haz the power, 2016-08-04 22:58:18]

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