Add a task_scheduler tracing category which will record an extra event per task.

Add a task_scheduler tracing category which will record an extra event per task.

That event will contain ExecutionMode/SequenceToken and TaskPriority.

It would be better to tack an optional extra arg to the main task event but
tracing doesn't currently allow this (http://crbug.com/652692).

BUG=649084
Committed: https://crrev.com/b7555076c5684f2679c0f63be683cde91cf6e215
Cr-Commit-Position: refs/heads/master@{#423681}

[gab, 2016-10-04 20:27:44]

Patchset #2 (id:20001) has been deleted

[gab, 2016-10-04 20:27:47]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[gab, 2016-10-04 20:28:05]

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

[gab, 2016-10-04 20:28:05]

Francois PTAL, thanks!

[commit-bot: I haz the power, 2016-10-04 20:28:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2016-10-04 20:29:39]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-04 20:29:55]

Dry run: 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-04 22:32:57]

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

[commit-bot: I haz the power, 2016-10-04 22:32:58]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fdoray, 2016-10-05 12:39:40]

https://codereview.chromium.org/2392903002/diff/40001/base/sequence_token.h
File base/sequence_token.h (right):

https://codereview.chromium.org/2392903002/diff/40001/base/sequence_token.h#n...
base/sequence_token.h:34: // Returns a string with a number uniquely
representing this SequenceToken.
Nobody should use this for logic. Maybe add:

// Should only be used for tracing, profiling and debugging.

Similar to
https://cs.chromium.org/chromium/src/base/threading/sequenced_worker_pool.h?l...

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

https://codereview.chromium.org/2392903002/diff/40001/base/task_scheduler/tas...
base/task_scheduler/task_tracker.cc:53: switch (task_traits_.priority()) {
Add a char* ToString(TaskPriority priority) function in task_traits.h and use it
both here and in operator<<?

https://cs.chromium.org/chromium/src/base/task_scheduler/task_traits.h?l=134

https://codereview.chromium.org/2392903002/diff/40001/base/task_scheduler/tas...
base/task_scheduler/task_tracker.cc:67: switch (execution_mode_) {
Same as above

https://codereview.chromium.org/2392903002/diff/40001/base/task_scheduler/tas...
base/task_scheduler/task_tracker.cc:81: dict.SetString("SequenceToken",
sequence_token_.ToString());
why not
dict.SetInteger("SequenceToken", sequence_token_.ToIntervalValue());

ToIntervalValue returns an int

https://codereview.chromium.org/2392903002/diff/40001/base/task_scheduler/tas...
base/task_scheduler/task_tracker.cc:269: ExecutionMode execution_mode =
const ExecutionMode execution_mode =

[gab, 2016-10-05 20:12:43]

Thanks, done, PTAnL.

https://codereview.chromium.org/2392903002/diff/40001/base/sequence_token.h
File base/sequence_token.h (right):

https://codereview.chromium.org/2392903002/diff/40001/base/sequence_token.h#n...
base/sequence_token.h:34: // Returns a string with a number uniquely
representing this SequenceToken.
On 2016/10/05 12:39:40, fdoray wrote:
> Nobody should use this for logic. Maybe add:
> 
> // Should only be used for tracing, profiling and debugging.
> 
> Similar to
>
https://cs.chromium.org/chromium/src/base/threading/sequenced_worker_pool.h?l...

Done.

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

https://codereview.chromium.org/2392903002/diff/40001/base/task_scheduler/tas...
base/task_scheduler/task_tracker.cc:53: switch (task_traits_.priority()) {
On 2016/10/05 12:39:40, fdoray wrote:
> Add a char* ToString(TaskPriority priority) function in task_traits.h and use
it
> both here and in operator<<?
> 
> https://cs.chromium.org/chromium/src/base/task_scheduler/task_traits.h?l=134

I had debated that and weakly opted against it, but now that you bring it up too
I agree that's better :-).

Except I didn't use "ToString()" as it's in base:: and a bit too generic with
that name.

https://codereview.chromium.org/2392903002/diff/40001/base/task_scheduler/tas...
base/task_scheduler/task_tracker.cc:67: switch (execution_mode_) {
On 2016/10/05 12:39:40, fdoray wrote:
> Same as above

Done.

https://codereview.chromium.org/2392903002/diff/40001/base/task_scheduler/tas...
base/task_scheduler/task_tracker.cc:81: dict.SetString("SequenceToken",
sequence_token_.ToString());
On 2016/10/05 12:39:40, fdoray wrote:
> why not
> dict.SetInteger("SequenceToken", sequence_token_.ToIntervalValue());
> 
> ToIntervalValue returns an int

Great idea, done.

https://codereview.chromium.org/2392903002/diff/40001/base/task_scheduler/tas...
base/task_scheduler/task_tracker.cc:269: ExecutionMode execution_mode =
On 2016/10/05 12:39:40, fdoray wrote:
> const ExecutionMode execution_mode =

Done.

[gab, 2016-10-05 20:13:16]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-05 20:13:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fdoray, 2016-10-05 20:27:19]

lgtm

[gab, 2016-10-05 20:52:09]

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

[gab, 2016-10-05 20:52:10]

Thanks @danakj for OWNERS

[commit-bot: I haz the power, 2016-10-05 22:15:56]

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

[commit-bot: I haz the power, 2016-10-05 22:15:56]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[danakj, 2016-10-05 23:26:52]

LGTM

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

https://codereview.chromium.org/2392903002/diff/80001/base/task_scheduler/tas...
base/task_scheduler/task_tracker.cc:38: // Overridden from
trace_event::ConvertableToTraceFormat:
nit: normally written as "trace_event::ConvertableToTraceFormat implementation."
in code i read

https://codereview.chromium.org/2392903002/diff/80001/base/task_scheduler/tas...
base/task_scheduler/task_tracker.cc:52: dict.SetString("TaskPriority",
aren't tracing values usually like_this instead of LikeThis?

https://cs.chromium.org/chromium/src/ui/compositor/layer.cc?rcl=0&l=827
https://cs.chromium.org/chromium/src/gpu/command_buffer/service/gpu_state_tra...
https://cs.chromium.org/chromium/src/cc/layers/layer_impl.cc?rcl=0&l=942

[gab, 2016-10-06 19:13:53]

Thanks, done.

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

https://codereview.chromium.org/2392903002/diff/80001/base/task_scheduler/tas...
base/task_scheduler/task_tracker.cc:38: // Overridden from
trace_event::ConvertableToTraceFormat:
On 2016/10/05 23:26:52, danakj wrote:
> nit: normally written as "trace_event::ConvertableToTraceFormat
implementation."
> in code i read

Done.

https://codereview.chromium.org/2392903002/diff/80001/base/task_scheduler/tas...
base/task_scheduler/task_tracker.cc:52: dict.SetString("TaskPriority",
On 2016/10/05 23:26:52, danakj wrote:
> aren't tracing values usually like_this instead of LikeThis?
> 
> https://cs.chromium.org/chromium/src/ui/compositor/layer.cc?rcl=0&l=827
>
https://cs.chromium.org/chromium/src/gpu/command_buffer/service/gpu_state_tra...
> https://cs.chromium.org/chromium/src/cc/layers/layer_impl.cc?rcl=0&l=942

Done.

[gab, 2016-10-06 19:15:10]

The CQ bit was checked by gab@chromium.org

[gab, 2016-10-06 19:15:11]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
fdoray@chromium.org
Link to the patchset: https://codereview.chromium.org/2392903002/#ps100001
(title: "review:danakj#19")

[commit-bot: I haz the power, 2016-10-06 19:15:57]

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-06 21:16:52]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-10-06 21:20:10]

Description was changed from

==========
Add a task_scheduler tracing category which will record an extra event per task.

That event will contain ExecutionMode/SequenceToken and TaskPriority.

It would be better to tack an optional extra arg to the main task event but
tracing doesn't currently allow this (http://crbug.com/652692).

BUG=649084
==========

to

==========
Add a task_scheduler tracing category which will record an extra event per task.

That event will contain ExecutionMode/SequenceToken and TaskPriority.

It would be better to tack an optional extra arg to the main task event but
tracing doesn't currently allow this (http://crbug.com/652692).

BUG=649084

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

[commit-bot: I haz the power, 2016-10-06 21:20:11]

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