Chrome Task Scheduler Task Traits

Chrome Task Scheduler Task Traits

Adds Task Traits documented in
https://docs.google.com/a/chromium.org/document/d/1S2AAeoo1xa_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa5k/edit

This change is a subset of https://codereview.chromium.org/1698183005/

BUG=553459

[robliao, 2016-02-16 21:32:00]

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

[robliao, 2016-02-16 21:32:01]

FYI, since we're starting the branch fresh, double check the CC lines since
we're doing a round of internal review first.

[robliao, 2016-02-16 21:37:28]

And added the OWNERS file.

[fdoray, 2016-02-16 21:47:01]

lgtm with 2 comments/questions.

https://codereview.chromium.org/1706533002/diff/20001/base/task_scheduler/tas...
File base/task_scheduler/task_traits.h (right):

https://codereview.chromium.org/1706533002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_traits.h:114: // first under contention.
Maybe just "Can execute multiple tasks at a time."

All tasks posted to a given PARALLEL TaskRunner have the same priority, so "High
priority tasks will execute first under contention." doesn't really make sense.

https://codereview.chromium.org/1706533002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_traits.h:119: // priority.
Maybe just "Executes one task at a time. Tasks are guaranteed to run in posting
order."

The last sentence is only relevant for CreateChildTaskRunner, so maybe we put it
next to the declaration of that function?

[robliao, 2016-02-16 21:55:04]

https://codereview.chromium.org/1706533002/diff/20001/base/task_scheduler/tas...
File base/task_scheduler/task_traits.h (right):

https://codereview.chromium.org/1706533002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_traits.h:114: // first under contention.
On 2016/02/16 21:47:00, fdoray wrote:
> Maybe just "Can execute multiple tasks at a time."
> 
> All tasks posted to a given PARALLEL TaskRunner have the same priority, so
"High
> priority tasks will execute first under contention." doesn't really make
sense.

CreateChildTaskRunnerWithTraits makes it so that these rules are important since
they allow the different priorities to enter the picture. It doesn't change the
fact that these describe the general behavior of the task runner. It should do
okay here, especially if we introduce other ways to change priorities of tasks
in the future.

https://codereview.chromium.org/1706533002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_traits.h:119: // priority.
On 2016/02/16 21:47:00, fdoray wrote:
> Maybe just "Executes one task at a time. Tasks are guaranteed to run in
posting
> order."
> 
> The last sentence is only relevant for CreateChildTaskRunner, so maybe we put
it
> next to the declaration of that function?

See previous comment.

[gab, 2016-02-16 22:15:43]

lgtm w/ comment nits

https://codereview.chromium.org/1706533002/diff/20001/base/task_scheduler/OWNERS
File base/task_scheduler/OWNERS (right):

https://codereview.chromium.org/1706533002/diff/20001/base/task_scheduler/OWN...
base/task_scheduler/OWNERS:3: fdoray@chromium.org
alpha-sort is the norm I think for OWNERS (except for the odd directories where
owners explicitly decided to randomize the list to load-balance..)

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

https://codereview.chromium.org/1706533002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_traits.cc:9: TaskTraits::TaskTraits()
Add a comment like: 

// Do not rely on defaults hard-coded below beyond the guarantees described in
the header, anything else is subject to change. Tasks should explicitly requests
defaults they depend on beyond the specifications of the API.

https://codereview.chromium.org/1706533002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_traits.cc:14: TaskTraits::~TaskTraits() = default;
TaskTraits is only made of POD types so the destructor can be inlined (i.e.
remove it from here and header and use default one)

https://codereview.chromium.org/1706533002/diff/20001/base/task_scheduler/tas...
File base/task_scheduler/task_traits.h (right):

https://codereview.chromium.org/1706533002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_traits.h:22: // - Updating the UI to reflect that a
long task was completed.
"to reflect progress on a long task." (i.e. generically talking about download
progress) ?

(i.e. otherwise I find this clashes with BACKGROUND as there we say : "user
won't notice if it takes a long time" but then we say that the UI update to a
long task is USER_VISIBLE)

https://codereview.chromium.org/1706533002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_traits.h:71: // explosion of PostTaskWith*() call
sites.
I guess the comment about preventing combinatorial explosion was nice for the
doc to justify using this over crrent PostTaskWith*() paradigm but in the
codebase it feels out of place, something like this is sufficient IMO:

// TaskTraits holds metadata to be associated to a task or group of tasks.

https://codereview.chromium.org/1706533002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_traits.h:89: // Applies a priority to tasks with these
traits.
s/a priority/|priority|/

https://codereview.chromium.org/1706533002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_traits.h:92: // Applies a shutdown behavior to tasks
with these traits.
s/a shutdown behavior/|shutdown_behavior|/

https://codereview.chromium.org/1706533002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_traits.h:114: // first under contention.
On 2016/02/16 21:47:00, fdoray wrote:
> Maybe just "Can execute multiple tasks at a time."
> 
> All tasks posted to a given PARALLEL TaskRunner have the same priority, so
"High
> priority tasks will execute first under contention." doesn't really make
sense.

Agreed and executing first compared to others is inherited from the definition
of TaskPriority.

https://codereview.chromium.org/1706533002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_traits.h:119: // priority.
On 2016/02/16 21:47:00, fdoray wrote:
> Maybe just "Executes one task at a time. Tasks are guaranteed to run in
posting
> order."
> 
> The last sentence is only relevant for CreateChildTaskRunner, so maybe we put
it
> next to the declaration of that function?

+1

https://codereview.chromium.org/1706533002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_traits.h:131: SINGLE_THREADED_COM_STA,
Maybe keep this back for the CL that will implement COM all at once?

[robliao, 2016-02-16 22:58:31]

https://codereview.chromium.org/1706533002/diff/20001/base/task_scheduler/OWNERS
File base/task_scheduler/OWNERS (right):

https://codereview.chromium.org/1706533002/diff/20001/base/task_scheduler/OWN...
base/task_scheduler/OWNERS:3: fdoray@chromium.org
On 2016/02/16 22:15:43, gab wrote:
> alpha-sort is the norm I think for OWNERS (except for the odd directories
where
> owners explicitly decided to randomize the list to load-balance..)

Done.

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

https://codereview.chromium.org/1706533002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_traits.cc:9: TaskTraits::TaskTraits()
On 2016/02/16 22:15:43, gab wrote:
> Add a comment like: 
> 
> // Do not rely on defaults hard-coded below beyond the guarantees described in
> the header, anything else is subject to change. Tasks should explicitly
requests
> defaults they depend on beyond the specifications of the API.

Added a variant of the blurb.

https://codereview.chromium.org/1706533002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_traits.cc:14: TaskTraits::~TaskTraits() = default;
On 2016/02/16 22:15:43, gab wrote:
> TaskTraits is only made of POD types so the destructor can be inlined (i.e.
> remove it from here and header and use default one)

The current guidance is this:
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-i...

I've no strong opinion either way, but we've already got a implementation file,
so it doesn't change much either way.

https://codereview.chromium.org/1706533002/diff/20001/base/task_scheduler/tas...
File base/task_scheduler/task_traits.h (right):

https://codereview.chromium.org/1706533002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_traits.h:22: // - Updating the UI to reflect that a
long task was completed.
On 2016/02/16 22:15:43, gab wrote:
> "to reflect progress on a long task." (i.e. generically talking about download
> progress) ?
> 
> (i.e. otherwise I find this clashes with BACKGROUND as there we say : "user
> won't notice if it takes a long time" but then we say that the UI update to a
> long task is USER_VISIBLE)

sg. Done.

https://codereview.chromium.org/1706533002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_traits.h:71: // explosion of PostTaskWith*() call
sites.
On 2016/02/16 22:15:43, gab wrote:
> I guess the comment about preventing combinatorial explosion was nice for the
> doc to justify using this over crrent PostTaskWith*() paradigm but in the
> codebase it feels out of place, something like this is sufficient IMO:
> 
> // TaskTraits holds metadata to be associated to a task or group of tasks.

Updated. Done.

https://codereview.chromium.org/1706533002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_traits.h:89: // Applies a priority to tasks with these
traits.
On 2016/02/16 22:15:43, gab wrote:
> s/a priority/|priority|/

Done.

https://codereview.chromium.org/1706533002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_traits.h:92: // Applies a shutdown behavior to tasks
with these traits.
On 2016/02/16 22:15:43, gab wrote:
> s/a shutdown behavior/|shutdown_behavior|/

Done.

https://codereview.chromium.org/1706533002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_traits.h:114: // first under contention.
On 2016/02/16 22:15:43, gab wrote:
> On 2016/02/16 21:47:00, fdoray wrote:
> > Maybe just "Can execute multiple tasks at a time."
> > 
> > All tasks posted to a given PARALLEL TaskRunner have the same priority, so
> "High
> > priority tasks will execute first under contention." doesn't really make
> sense.
> 
> Agreed and executing first compared to others is inherited from the definition
> of TaskPriority.

Discussed offline.

https://codereview.chromium.org/1706533002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_traits.h:119: // priority.
On 2016/02/16 22:15:43, gab wrote:
> On 2016/02/16 21:47:00, fdoray wrote:
> > Maybe just "Executes one task at a time. Tasks are guaranteed to run in
> posting
> > order."
> > 
> > The last sentence is only relevant for CreateChildTaskRunner, so maybe we
put
> it
> > next to the declaration of that function?
> 
> +1

Discussed offline.

https://codereview.chromium.org/1706533002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_traits.h:131: SINGLE_THREADED_COM_STA,
On 2016/02/16 22:15:43, gab wrote:
> Maybe keep this back for the CL that will implement COM all at once?

Makes sense to me. Done.

[gab, 2016-02-16 23:03:26]

lgtm, thanks

[robliao, 2016-02-16 23:12:46]

Description was changed from

==========
Chrome Task Scheduler Task Traits

Adds Task Traits documented in
https://docs.google.com/a/chromium.org/document/d/1S2AAeoo1xa_vsLbDYBsDHCqhrk...

This change is a subset of https://codereview.chromium.org/1698183005/

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

to

==========
Chrome Task Scheduler Task Traits

Adds Task Traits documented in
https://docs.google.com/a/chromium.org/document/d/1S2AAeoo1xa_vsLbDYBsDHCqhrk...

This change is a subset of https://codereview.chromium.org/1698183005/

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

[robliao, 2016-02-16 23:14:16]

On 2016/02/16 23:03:26, gab wrote:
> lgtm, thanks

Landed at https://luckyluke-private.googlesource.com/src/+/s_0_task_traits/

[robliao, 2016-02-16 23:28:17]

Patchset #4 (id:60001) has been deleted