TaskScheduler: Add internal::GetTaskPriorityForCurrentThread().

TaskScheduler: Add internal::GetTaskPriorityForCurrentThread().

This function returns the TaskPriority of the currently running
task. It will used to initialize TaskTraits with the priority of
the currently running task.

BUG=553459
TBR=danakj@chromium.org

Committed: https://crrev.com/0458ffb3117d62ea4922b578effb625ed118d647
Cr-Commit-Position: refs/heads/master@{#435728}

[fdoray, 2016-12-01 17:14:53]

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

[fdoray, 2016-12-01 17:14:54]

PTAL

[gab, 2016-12-01 18:04:39]

Meta-comment:

I'm not convinced we want every trait to be inherited.

In fact I think we specifically only want TaskPriority to be inherited.

It's the only trait that is contextual at the moment, every other trait is about
what the scheduled task *will do* (which is independent from what its context is
doing -- i.e. file I/O, ShutdownBehavior).

https://codereview.chromium.org/2546773002/diff/40001/base/task_scheduler/tas...
File base/task_scheduler/task_traits_for_current_thread.cc (right):

https://codereview.chromium.org/2546773002/diff/40001/base/task_scheduler/tas...
base/task_scheduler/task_traits_for_current_thread.cc:33: DCHECK_EQ(traits_,
GetTaskTraitsForCurrentThread());
Is this necessary? As long as we DCHECK == null in the constructor and DCHECK !=
null in destructor then it can't be set to anything else?

[robliao, 2016-12-01 18:12:01]

https://codereview.chromium.org/2546773002/diff/40001/base/task_scheduler/tas...
File base/task_scheduler/task_traits_for_current_thread.cc (right):

https://codereview.chromium.org/2546773002/diff/40001/base/task_scheduler/tas...
base/task_scheduler/task_traits_for_current_thread.cc:21: const TaskTraits*
traits)
Crazy thought. Can this simply be a const TaskTraits& and we store the address
of that?

[fdoray, 2016-12-01 18:51:53]

On 2016/12/01 18:04:39, gab wrote:
> Meta-comment:
> 
> I'm not convinced we want every trait to be inherited.
> 
> In fact I think we specifically only want TaskPriority to be inherited.
> 
> It's the only trait that is contextual at the moment, every other trait is
about
> what the scheduled task *will do* (which is independent from what its context
is
> doing -- i.e. file I/O, ShutdownBehavior).
> 

I didn't plan to inherit other traits. I decided to store TaskTraits* instead of
TaskPriority* because I thought it could be useful in the future to have access
to other traits (e.g. to implement DCHECK_HAS_MUTUAL_EXCLUSION_TOKEN(token);).
Since these are speculative needs, I will change this CL to store only
TaskPriority* in TLS.

[fdoray, 2016-12-01 19:08:06]

Description was changed from

==========
TaskScheduler: Add internal::GetTaskTraitsForCurrentThread().

This function returns the TaskTraits of the currently running task.
It will used to initialize TaskTraits with the priority of the
currently running task.

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

to

==========
TaskScheduler: Add internal::GetTaskPriorityForCurrentThread().

This function returns the TaskPriority of the currently running
task. It will used to initialize TaskTraits with the priority of
the currently running task.

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

[fdoray, 2016-12-01 19:16:02]

PTAnL

On 2016/12/01 18:04:39, gab wrote:
> Meta-comment:
> 
> I'm not convinced we want every trait to be inherited.
> 
> In fact I think we specifically only want TaskPriority to be inherited.
> 
> It's the only trait that is contextual at the moment, every other trait is
about
> what the scheduled task *will do* (which is independent from what its context
is
> doing -- i.e. file I/O, ShutdownBehavior).
> 

I didn't plan to inherit other traits. I decided to store TaskTraits* instead of
TaskPriority* because I thought it could be useful in the future to have access
to other traits (e.g. to implement DCHECK_HAS_MUTUAL_EXCLUSION_TOKEN(token);).
Since these are speculative needs, I will change this CL to store only
TaskPriority* in TLS.

https://codereview.chromium.org/2546773002/diff/40001/base/task_scheduler/tas...
File base/task_scheduler/task_traits_for_current_thread.cc (right):

https://codereview.chromium.org/2546773002/diff/40001/base/task_scheduler/tas...
base/task_scheduler/task_traits_for_current_thread.cc:21: const TaskTraits*
traits)
On 2016/12/01 18:12:01, robliao wrote:
> Crazy thought. Can this simply be a const TaskTraits& and we store the address
> of that?

n/a with TaskPriority

https://codereview.chromium.org/2546773002/diff/40001/base/task_scheduler/tas...
base/task_scheduler/task_traits_for_current_thread.cc:33: DCHECK_EQ(traits_,
GetTaskTraitsForCurrentThread());
On 2016/12/01 18:04:39, gab wrote:
> Is this necessary? As long as we DCHECK == null in the constructor and DCHECK
!=
> null in destructor then it can't be set to anything else?

Not necessary. Do you still want me to remove it with the latest patch set?

[robliao, 2016-12-01 19:27:27]

https://codereview.chromium.org/2546773002/diff/80001/base/task_scheduler/tas...
File base/task_scheduler/task_priority_for_current_thread.cc (right):

https://codereview.chromium.org/2546773002/diff/80001/base/task_scheduler/tas...
base/task_scheduler/task_priority_for_current_thread.cc:16:
LazyInstance<ThreadLocalPointer<const TaskPriority>>::Leaky
Would ThreadLocalStorage::StaticSlot tls... = TLS_INITIALIZER work better here?

https://codereview.chromium.org/2546773002/diff/80001/base/task_scheduler/tas...
File base/task_scheduler/task_priority_for_current_thread.h (right):

https://codereview.chromium.org/2546773002/diff/80001/base/task_scheduler/tas...
base/task_scheduler/task_priority_for_current_thread.h:15: class BASE_EXPORT
ScopedSetTaskPriorityForCurrentThread {
The file name should match the class.
scoped_set_task_priority_for_current_thread.[cc|h]
That's a mouthful!

[fdoray, 2016-12-01 19:41:07]

https://codereview.chromium.org/2546773002/diff/80001/base/task_scheduler/tas...
File base/task_scheduler/task_priority_for_current_thread.cc (right):

https://codereview.chromium.org/2546773002/diff/80001/base/task_scheduler/tas...
base/task_scheduler/task_priority_for_current_thread.cc:16:
LazyInstance<ThreadLocalPointer<const TaskPriority>>::Leaky
On 2016/12/01 19:27:27, robliao wrote:
> Would ThreadLocalStorage::StaticSlot tls... = TLS_INITIALIZER work better
here?

I doubt it.

1. I don't need more direct control of the slot
https://cs.chromium.org/chromium/src/base/threading/thread_local_storage.h?sq...
2. I would need to initialize the slot myself
https://cs.chromium.org/chromium/src/base/threading/thread_local_storage.h?sq...
. LazyInstance+ThreadLocalPointer handles that for me.

https://codereview.chromium.org/2546773002/diff/80001/base/task_scheduler/tas...
File base/task_scheduler/task_priority_for_current_thread.h (right):

https://codereview.chromium.org/2546773002/diff/80001/base/task_scheduler/tas...
base/task_scheduler/task_priority_for_current_thread.h:15: class BASE_EXPORT
ScopedSetTaskPriorityForCurrentThread {
On 2016/12/01 19:27:27, robliao wrote:
> The file name should match the class.
> scoped_set_task_priority_for_current_thread.[cc|h]
> That's a mouthful!

Even if this file also declares a GetTaskPriorityForCurrentThread() function?

[robliao, 2016-12-01 20:08:57]

https://codereview.chromium.org/2546773002/diff/80001/base/task_scheduler/tas...
File base/task_scheduler/task_priority_for_current_thread.cc (right):

https://codereview.chromium.org/2546773002/diff/80001/base/task_scheduler/tas...
base/task_scheduler/task_priority_for_current_thread.cc:16:
LazyInstance<ThreadLocalPointer<const TaskPriority>>::Leaky
On 2016/12/01 19:41:07, fdoray wrote:
> On 2016/12/01 19:27:27, robliao wrote:
> > Would ThreadLocalStorage::StaticSlot tls... = TLS_INITIALIZER work better
> here?
> 
> I doubt it.
> 
> 1. I don't need more direct control of the slot
>
https://cs.chromium.org/chromium/src/base/threading/thread_local_storage.h?sq...
> 2. I would need to initialize the slot myself
>
https://cs.chromium.org/chromium/src/base/threading/thread_local_storage.h?sq...
> . LazyInstance+ThreadLocalPointer handles that for me.

sgtm.

https://codereview.chromium.org/2546773002/diff/80001/base/task_scheduler/tas...
File base/task_scheduler/task_priority_for_current_thread.h (right):

https://codereview.chromium.org/2546773002/diff/80001/base/task_scheduler/tas...
base/task_scheduler/task_priority_for_current_thread.h:15: class BASE_EXPORT
ScopedSetTaskPriorityForCurrentThread {
On 2016/12/01 19:41:07, fdoray wrote:
> On 2016/12/01 19:27:27, robliao wrote:
> > The file name should match the class.
> > scoped_set_task_priority_for_current_thread.[cc|h]
> > That's a mouthful!
> 
> Even if this file also declares a GetTaskPriorityForCurrentThread() function? 

That's the convention we've used so far. It goes with the predominant class. 
https://codereview.chromium.org/1908633002/diff/40001/base/test/command_line_...

https://google.github.io/styleguide/cppguide.html#File_Names (on convention)

https://cs.chromium.org/chromium/src/base/sequence_token.h defines SequenceToken
as the predominant class. TaskToken and ScopedSetSequenceTokenForCurrentThread
simply jump along for the ride. Arguably they should be in different files.

[fdoray, 2016-12-01 20:23:37]

PTAnL

https://codereview.chromium.org/2546773002/diff/80001/base/task_scheduler/tas...
File base/task_scheduler/task_priority_for_current_thread.h (right):

https://codereview.chromium.org/2546773002/diff/80001/base/task_scheduler/tas...
base/task_scheduler/task_priority_for_current_thread.h:15: class BASE_EXPORT
ScopedSetTaskPriorityForCurrentThread {
On 2016/12/01 20:08:57, robliao wrote:
> On 2016/12/01 19:41:07, fdoray wrote:
> > On 2016/12/01 19:27:27, robliao wrote:
> > > The file name should match the class.
> > > scoped_set_task_priority_for_current_thread.[cc|h]
> > > That's a mouthful!
> > 
> > Even if this file also declares a GetTaskPriorityForCurrentThread()
function? 
> 
> That's the convention we've used so far. It goes with the predominant class. 
>
https://codereview.chromium.org/1908633002/diff/40001/base/test/command_line_...
> 
> https://google.github.io/styleguide/cppguide.html#File_Names (on convention)
> 
> https://cs.chromium.org/chromium/src/base/sequence_token.h defines
SequenceToken
> as the predominant class. TaskToken and ScopedSetSequenceTokenForCurrentThread
> simply jump along for the ride. Arguably they should be in different files.
> 

Done.

[robliao, 2016-12-01 20:25:24]

lgtm

[fdoray, 2016-12-01 20:26:59]

Description was changed from

==========
TaskScheduler: Add internal::GetTaskPriorityForCurrentThread().

This function returns the TaskPriority of the currently running
task. It will used to initialize TaskTraits with the priority of
the currently running task.

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

to

==========
TaskScheduler: Add internal::GetTaskPriorityForCurrentThread().

This function returns the TaskPriority of the currently running
task. It will used to initialize TaskTraits with the priority of
the currently running task.

BUG=553459
TBR=danakj@chromium.org
==========

[fdoray, 2016-12-01 20:27:16]

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

[fdoray, 2016-12-01 20:27:17]

TBR danakj@chromium.org for base/BUILD.gn

[fdoray, 2016-12-01 20:27:21]

The CQ bit was checked by fdoray@chromium.org

[commit-bot: I haz the power, 2016-12-01 20:27:55]

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-12-01 21:49:50]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1480624041737360,
"parent_rev": "190414a5f3ea896fc05864ef7d5e45d7deed9912", "commit_rev":
"a7c3a2d65a05ea9882454d149c7d7454bffbd170"}

[commit-bot: I haz the power, 2016-12-01 21:50:14]

Description was changed from

==========
TaskScheduler: Add internal::GetTaskPriorityForCurrentThread().

This function returns the TaskPriority of the currently running
task. It will used to initialize TaskTraits with the priority of
the currently running task.

BUG=553459
TBR=danakj@chromium.org
==========

to

==========
TaskScheduler: Add internal::GetTaskPriorityForCurrentThread().

This function returns the TaskPriority of the currently running
task. It will used to initialize TaskTraits with the priority of
the currently running task.

BUG=553459
TBR=danakj@chromium.org
==========

[commit-bot: I haz the power, 2016-12-01 21:50:15]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-12-01 21:52:53]

Description was changed from

==========
TaskScheduler: Add internal::GetTaskPriorityForCurrentThread().

This function returns the TaskPriority of the currently running
task. It will used to initialize TaskTraits with the priority of
the currently running task.

BUG=553459
TBR=danakj@chromium.org
==========

to

==========
TaskScheduler: Add internal::GetTaskPriorityForCurrentThread().

This function returns the TaskPriority of the currently running
task. It will used to initialize TaskTraits with the priority of
the currently running task.

BUG=553459
TBR=danakj@chromium.org

Committed: https://crrev.com/0458ffb3117d62ea4922b578effb625ed118d647
Cr-Commit-Position: refs/heads/master@{#435728}
==========

[commit-bot: I haz the power, 2016-12-01 21:52:53]

Patchset 6 (id:??) landed as
https://crrev.com/0458ffb3117d62ea4922b578effb625ed118d647
Cr-Commit-Position: refs/heads/master@{#435728}