Record task durations on Renderer Main & Compositor threads.

Record task durations on Renderer Main & Compositor threads.

This change piggybacks on the chrome://profiler machinery. Normally,
this machinery is only enabled on some platforms. This patch forces us
to measure durations of tasks on the renderer main and compositor
threads.

This may negatively impact Android performance, but based on previous
experimentation of this sort, I strongly expect we won't see any
regressions here.

BUG=736449

[tdresser, 2017-07-04 19:40:36]

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

[commit-bot: I haz the power, 2017-07-04 19:40:45]

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, 2017-07-04 21:47:26]

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

[commit-bot: I haz the power, 2017-07-04 21:47:27]

Dry run: This issue passed the CQ dry run.

[tdresser1, 2017-07-06 20:36:14]

tdresser@google.com changed reviewers:
+ altimin@chromium.org, tdresser@google.com

[tdresser1, 2017-07-06 20:36:15]

+altimin, can you see if this seems sane to you?

I'm a bit worried about the potential performance implications here, but the
perf try job results have enough noise that it isn't really conclusive.

(See the perf tryjob results on the most recent patch set)

[altimin, 2017-07-07 14:36:33]

On 2017/07/06 20:36:15, tdresser1 wrote:
> +altimin, can you see if this seems sane to you?
> 
> I'm a bit worried about the potential performance implications here, but the
> perf try job results have enough noise that it isn't really conclusive.
> 
> (See the perf tryjob results on the most recent patch set)

The general idea seems fine, but I have some reservations about layering. I
believe that the general principle is that base/ shouldn't now about particular
threads, so should we do the plumbing as appropriate?

[tdresser1, 2017-07-07 15:02:45]

tdresser@google.com changed reviewers:
+ skyostil@chromium.org

[tdresser1, 2017-07-07 15:02:46]

Yeah, that's a good point.

Any thoughts on where this should live / how it should be plumbed?

[altimin, 2017-07-07 16:53:37]

On 2017/07/07 15:02:46, tdresser1 wrote:
> Yeah, that's a good point.
> 
> Any thoughts on where this should live / how it should be plumbed?

I was thinking about having ThreadData::EnableTaskLengthRecording(const char*
histogram_name) and calling it during thread initialization
(content/renderer/renderer_main.cc etc).

[tdresser, 2017-07-26 18:14:28]

How's this?

[Sami, 2017-07-26 20:36:38]

https://codereview.chromium.org/2973543002/diff/40001/base/debug/task_annotat...
File base/debug/task_annotator.cc (right):

https://codereview.chromium.org/2973543002/diff/40001/base/debug/task_annotat...
base/debug/task_annotator.cc:26: if
(!thread_data->task_length_recording_enabled_for_uma())
Can we swap this before the previous 'if' since this is more likely to early
out?

https://codereview.chromium.org/2973543002/diff/40001/base/debug/task_annotat...
base/debug/task_annotator.cc:29: // TODO - this is slow.
TODO(tdresser)

In any case we probably can't land this as is given Bruce's recent findings
about debug string prints during video playback.

https://codereview.chromium.org/2973543002/diff/40001/base/tracked_objects.h
File base/tracked_objects.h (right):

https://codereview.chromium.org/2973543002/diff/40001/base/tracked_objects.h#...
base/tracked_objects.h:655: // If the provided thread is enabled for task
duration tracking, returns the
nit: "UMA task duration tracking" (I had to re-read this a couple of times to
figure out the difference to Now())

[tdresser, 2017-07-27 19:18:06]

https://codereview.chromium.org/2973543002/diff/40001/base/debug/task_annotat...
File base/debug/task_annotator.cc (right):

https://codereview.chromium.org/2973543002/diff/40001/base/debug/task_annotat...
base/debug/task_annotator.cc:29: // TODO - this is slow.
On 2017/07/26 20:36:38, Sami wrote:
> TODO(tdresser)
> 
> In any case we probably can't land this as is given Bruce's recent findings
> about debug string prints during video playback.

Oops, yeah, I meant to call this out as something I was wondering about.

I can imagine a solution in which we use some kind of integer thread type
identifier, and a STATIC_HISTOGRAM_POINTER_GROUP, which would make this much
faster, but most approaches to this would result in some awkward layering
violations.

We could have the "max thread id" get passed to the task annotator at some
point, have a big enum listing possible thread types, and use those here instead
of string manipulation. 

We'd then need to populate a vector where the index indicates the histogram
name, and use a STATIC_HISTOGRAM_POINTER_GROUP.

Pretty messy, but could work.

Thoughts?

[Sami, 2017-08-04 15:46:33]

https://codereview.chromium.org/2973543002/diff/40001/base/debug/task_annotat...
File base/debug/task_annotator.cc (right):

https://codereview.chromium.org/2973543002/diff/40001/base/debug/task_annotat...
base/debug/task_annotator.cc:29: // TODO - this is slow.
On 2017/07/27 19:18:05, tdresser wrote:
> On 2017/07/26 20:36:38, Sami wrote:
> > TODO(tdresser)
> > 
> > In any case we probably can't land this as is given Bruce's recent findings
> > about debug string prints during video playback.
> 
> Oops, yeah, I meant to call this out as something I was wondering about.
> 
> I can imagine a solution in which we use some kind of integer thread type
> identifier, and a STATIC_HISTOGRAM_POINTER_GROUP, which would make this much
> faster, but most approaches to this would result in some awkward layering
> violations.
> 
> We could have the "max thread id" get passed to the task annotator at some
> point, have a big enum listing possible thread types, and use those here
instead
> of string manipulation. 
> 
> We'd then need to populate a vector where the index indicates the histogram
> name, and use a STATIC_HISTOGRAM_POINTER_GROUP.
> 
> Pretty messy, but could work.
> 
> Thoughts?

Yeah, this is pretty tough from a layering perspective. A parallel case that
comes to mind is the tracing thread sorting index:

https://cs.chromium.org/chromium/src/base/trace_event/trace_log.h?l=320&gs=cp...

Maybe we could merge and generalize that with InitializeThreadContext()? We'd
still need to have a registry of ids somewhere for that to work however.

Alternatively we could compute a small hash of the thread name (possibly at
compile time) and use that as the id (and DCHECK that none of the names
collide).

[tdresser, 2017-08-04 15:56:57]

On 2017/08/04 15:46:33, Sami wrote:
>
https://codereview.chromium.org/2973543002/diff/40001/base/debug/task_annotat...
> File base/debug/task_annotator.cc (right):
> 
>
https://codereview.chromium.org/2973543002/diff/40001/base/debug/task_annotat...
> base/debug/task_annotator.cc:29: // TODO - this is slow.
> On 2017/07/27 19:18:05, tdresser wrote:
> > On 2017/07/26 20:36:38, Sami wrote:
> > > TODO(tdresser)
> > > 
> > > In any case we probably can't land this as is given Bruce's recent
findings
> > > about debug string prints during video playback.
> > 
> > Oops, yeah, I meant to call this out as something I was wondering about.
> > 
> > I can imagine a solution in which we use some kind of integer thread type
> > identifier, and a STATIC_HISTOGRAM_POINTER_GROUP, which would make this much
> > faster, but most approaches to this would result in some awkward layering
> > violations.
> > 
> > We could have the "max thread id" get passed to the task annotator at some
> > point, have a big enum listing possible thread types, and use those here
> instead
> > of string manipulation. 
> > 
> > We'd then need to populate a vector where the index indicates the histogram
> > name, and use a STATIC_HISTOGRAM_POINTER_GROUP.
> > 
> > Pretty messy, but could work.
> > 
> > Thoughts?
> 
> Yeah, this is pretty tough from a layering perspective. A parallel case that
> comes to mind is the tracing thread sorting index:
> 
>
https://cs.chromium.org/chromium/src/base/trace_event/trace_log.h?l=320&gs=cp...
> 
> Maybe we could merge and generalize that with InitializeThreadContext()? We'd
> still need to have a registry of ids somewhere for that to work however.
> 
> Alternatively we could compute a small hash of the thread name (possibly at
> compile time) and use that as the id (and DCHECK that none of the names
> collide).

Interesting thought.

For STATIC_HISTOGRAM_POINTER_GROUP, we'd still need to know the number of unique
thread names. We could pass that in though.

I'll take a stab at something along these lines when I get the chance.