CC: Add 'cc-benchmark' traces for minimal tracing in benchmarks.

CC: Add 'cc-benchmark' traces for minimal tracing in benchmarks.

We use the swap-buffers for normalizing in benchmarks, so this is the only trace labelled for now.

TBR=nduca
BUG=342635
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251441

[epenner, 2014-02-11 07:15:46]

Ptal. It's basically a one-liner, but we need a "SwapBuffers" trace to normalize
by frames produced.

Nat, any ideas for how to label the "SwapBuffers" trace in CC? Should I just add
an extra one?

On a positive/negative note (depending how you look at it), tracing is adding
major overhead in heavily traced areas like CC.

[epenner, 2014-02-11 20:10:44]

https://codereview.chromium.org/148613014/diff/30001/cc/output/delegating_ren...
File cc/output/delegating_renderer.cc (right):

https://codereview.chromium.org/148613014/diff/30001/cc/output/delegating_ren...
cc/output/delegating_renderer.cc:125: TRACE_EVENT0("toplevel",
"DelegatingRenderer::SwapBuffers");
This is obviously a hack to indicate the problem. We need to know about Swaps in
the benchmark. I could rename it to 'benchmark' since that also needs to be
enabled, or I could have two traces... TBH, I'm not sure what the best solution
is.

[danakj, 2014-02-11 20:12:22]

https://codereview.chromium.org/148613014/diff/30001/cc/output/delegating_ren...
File cc/output/delegating_renderer.cc (right):

https://codereview.chromium.org/148613014/diff/30001/cc/output/delegating_ren...
cc/output/delegating_renderer.cc:125: TRACE_EVENT0("toplevel",
"DelegatingRenderer::SwapBuffers");
On 2014/02/11 20:10:44, epenner wrote:
> This is obviously a hack to indicate the problem. We need to know about Swaps
in
> the benchmark. I could rename it to 'benchmark' since that also needs to be
> enabled, or I could have two traces... TBH, I'm not sure what the best
solution
> is.

Why does changing the category help?

[epennerAtGoogle, 2014-02-11 20:20:21]

On 2014/02/11 20:12:22, danakj wrote:
>
https://codereview.chromium.org/148613014/diff/30001/cc/output/delegating_ren...
> File cc/output/delegating_renderer.cc (right):
> 
>
https://codereview.chromium.org/148613014/diff/30001/cc/output/delegating_ren...
> cc/output/delegating_renderer.cc:125: TRACE_EVENT0("toplevel",
> "DelegatingRenderer::SwapBuffers");
> On 2014/02/11 20:10:44, epenner wrote:
> > This is obviously a hack to indicate the problem. We need to know about
Swaps
> in
> > the benchmark. I could rename it to 'benchmark' since that also needs to be
> > enabled, or I could have two traces... TBH, I'm not sure what the best
> solution
> > is.
> 
> Why does changing the category help?

There are a couple benchmarks that are starting to use tracing as their data
collection method, since it provides such great data. In these benchmarks we
need to scrape certain traces, and in this particular benchmark we need to know
about how many swaps are occurring, such that we can calculate
CPU-time-per-swap. If all CC-traces are enabled we can use the CC-trace, but
CC-traces appear to be quite expensive right now and we don't want to enable
them. 

I'd like to spend some time to determine what the costly CC-traces are, since
they distort the picture we get from tracing quite significantly it turns out.
But for this benchmark we'll likely always want CC disabled for minimum noise.

[danakj, 2014-02-11 20:23:19]

https://codereview.chromium.org/148613014/diff/30001/cc/output/delegating_ren...
File cc/output/delegating_renderer.cc (right):

https://codereview.chromium.org/148613014/diff/30001/cc/output/delegating_ren...
cc/output/delegating_renderer.cc:125: TRACE_EVENT0("toplevel",
"DelegatingRenderer::SwapBuffers");
On 2014/02/11 20:12:23, danakj wrote:
> On 2014/02/11 20:10:44, epenner wrote:
> > This is obviously a hack to indicate the problem. We need to know about
Swaps
> in
> > the benchmark. I could rename it to 'benchmark' since that also needs to be
> > enabled, or I could have two traces... TBH, I'm not sure what the best
> solution
> > is.
> 
> Why does changing the category help?

Can you stick it in 2 categories then? cc,cc-benchmark or something? I think
that would work?

[epennerAtGoogle, 2014-02-11 20:25:16]

> > Why does changing the category help?
> 
> Can you stick it in 2 categories then? cc,cc-benchmark or something? I think
> that would work?

Yep, that's also fine by me. It's slightly more burden for someone to enable
'only CC', but probably not a big deal.

[danakj, 2014-02-11 20:26:41]

On Tue, Feb 11, 2014 at 3:25 PM, <epenner@google.com> wrote:

> > Why does changing the category help?
>>
>
>  Can you stick it in 2 categories then? cc,cc-benchmark or something? I
>> think
>> that would work?
>>
>
> Yep, that's also fine by me. It's slightly more burden for someone to
> enable
> 'only CC', but probably not a big deal.
>

It could be a disabled-by-default category?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[epennerAtGoogle, 2014-02-11 20:32:02]

> >  Can you stick it in 2 categories then? cc,cc-benchmark or something? I
> >> think
> >> that would work?
> >>
> >
> > Yep, that's also fine by me. It's slightly more burden for someone to
> > enable
> > 'only CC', but probably not a big deal.
> >
> 
> It could be a disabled-by-default category?
> 

That's also fine, but are you suggesting there be two traces or one? I see two
options:
- One cc trace and one cc-benchmark-disabled-by-default trace.
- Only the cc-benchmark-disabled-by-default trace.

It's really not a biggy to me, however we slice it.

[enne (OOO), 2014-02-11 20:32:14]

+1 to two categories

[danakj, 2014-02-11 20:45:38]

On Tue, Feb 11, 2014 at 3:32 PM, <epenner@google.com> wrote:

> >  Can you stick it in 2 categories then? cc,cc-benchmark or something? I
>> >> think
>> >> that would work?
>> >>
>> >
>> > Yep, that's also fine by me. It's slightly more burden for someone to
>> > enable
>> > 'only CC', but probably not a big deal.
>> >
>>
>
>  It could be a disabled-by-default category?
>>
>
>
> That's also fine, but are you suggesting there be two traces or one? I see
> two
> options:
> - One cc trace and one cc-benchmark-disabled-by-default trace.
> - Only the cc-benchmark-disabled-by-default trace.
>

Oh it's the trace thats disabled not the category? Then ya, a "cc" trace
and a "cc-benchmark-disabled-by-default" TRACE_INSTANT?


>
> It's really not a biggy to me, however we slice it.
>
>
> https://codereview.chromium.org/148613014/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[epennerAtGoogle, 2014-02-11 20:48:27]

OH! Can a single trace have two categories? Sorry I'm so slow to catch on :P
That would be perfect.

[epennerAtGoogle, 2014-02-11 20:52:20]

On 2014/02/11 20:48:27, epennerAtGoogle wrote:
> OH! Can a single trace have two categories? Sorry I'm so slow to catch on :P
> That would be perfect.

Maybe not. In that case two traces, one being instant/Disabled-by-default SGTM!

[epennerAtGoogle, 2014-02-11 20:58:25]

Found one! Need to brush off my regex skills but looks like we can just use two
categories in one trace. FTW!

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[epenner, 2014-02-12 02:02:15]

Ptal.

https://codereview.chromium.org/148613014/diff/210001/base/message_loop/messa...
File base/message_loop/message_loop.cc (right):

https://codereview.chromium.org/148613014/diff/210001/base/message_loop/messa...
base/message_loop/message_loop.cc:417: // semi-regular tasks (for sufficient
measurement granularity).
This is the primary tracing change. The rest just renames the other 'toplevel'
traces to be 'ipc' and 'task' again.

[nduca, 2014-02-12 03:39:57]

i've lost track of this patch tbh. um, can we split off the bit about cc stuff
from the ipc stuff?

Also, I still think toplevel is the right call, why are we moving away from it?

[epennerAtGoogle, 2014-02-12 04:22:20]

On 2014/02/12 03:39:57, nduca wrote:
> i've lost track of this patch tbh. um, can we split off the bit about cc stuff
> from the ipc stuff?
> 
> Also, I still think toplevel is the right call, why are we moving away from
it?

SG, I'll split out the patch into two. 

Executive Summary: This isn't removing 'toplevel', it's implementing 'toplevel'
more efficiently. Now it captures _all_ cpu-time ('task'/'ipc' were missing
quite a bit), and we never mislabel a non-toplevel trace as 'toplevel'.

Simplified patch will be more clear.

[epenner, 2014-02-13 05:58:39]

ptal

Back to basics with the three liner to report only SwapBuffers traces.

[epenner, 2014-02-14 03:27:58]

On 2014/02/13 05:58:39, epenner wrote:
> ptal
> 
> Back to basics with the three liner to report only SwapBuffers traces.

I've reduced this patch to just the recommended parts. 3+ props for this in the
comments so I'm TBR'ing if that's okay.

[epenner, 2014-02-14 03:28:03]

The CQ bit was checked by epenner@chromium.org

[epenner, 2014-02-14 03:28:36]

The CQ bit was unchecked by epenner@chromium.org

[epenner, 2014-02-14 03:28:43]

The CQ bit was checked by epenner@chromium.org

[commit-bot: I haz the power, 2014-02-14 03:30:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/epenner@chromium.org/148613014/270001

[commit-bot: I haz the power, 2014-02-14 06:32:10]

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

[commit-bot: I haz the power, 2014-02-14 06:32:10]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[ernstm, 2014-02-14 22:15:06]

We already have a 'benchmark' category that was added for the same reason. It is
used by the smoothness and rasterize_and_record benchmarks. Could you use that
same category, or would the overhead of the few other events in the category
cause problems for your benchmark? If so, we might want to use more specific
names (e.g. benchmark.fastpath), because both 'benchmark' and the new
'cc-benchmark' would be used for compositor related benchmarks.

[epennerAtGoogle, 2014-02-14 22:24:30]

On 2014/02/14 22:15:06, ernstm wrote:
> We already have a 'benchmark' category that was added for the same reason. It
is
> used by the smoothness and rasterize_and_record benchmarks. Could you use that
> same category, or would the overhead of the few other events in the category
> cause problems for your benchmark? If so, we might want to use more specific
> names (e.g. benchmark.fastpath), because both 'benchmark' and the new
> 'cc-benchmark' would be used for compositor related benchmarks.

I do want to minimize times in this case but I'll look at benchmark and see if
it is significant. I think I might need to enable 'benchmark' anyway, so no
reason to not merge it at the moment.

[epennerAtGoogle, 2014-02-14 22:25:24]

On 2014/02/14 22:24:30, epennerAtGoogle wrote:
> On 2014/02/14 22:15:06, ernstm wrote:
> > We already have a 'benchmark' category that was added for the same reason.
It
> is
> > used by the smoothness and rasterize_and_record benchmarks. Could you use
that
> > same category, or would the overhead of the few other events in the category
> > cause problems for your benchmark? If so, we might want to use more specific
> > names (e.g. benchmark.fastpath), because both 'benchmark' and the new
> > 'cc-benchmark' would be used for compositor related benchmarks.
> 
> I do want to minimize times in this case but I'll look at benchmark and see if
> it is significant. I think I might need to enable 'benchmark' anyway, so no
> reason to not merge it at the moment.

Oh good it didn't land yet, so I can just change it. I'll change it and revisit
if we need less traces.

[danakj, 2014-02-14 22:27:26]

On Fri, Feb 14, 2014 at 5:24 PM, <epenner@google.com> wrote:

> On 2014/02/14 22:15:06, ernstm wrote:
>
>> We already have a 'benchmark' category that was added for the same
>> reason. It
>>
> is
>
>> used by the smoothness and rasterize_and_record benchmarks. Could you use
>> that
>> same category, or would the overhead of the few other events in the
>> category
>> cause problems for your benchmark? If so, we might want to use more
>> specific
>> names (e.g. benchmark.fastpath), because both 'benchmark' and the new
>> 'cc-benchmark' would be used for compositor related benchmarks.
>>
>
> I do want to minimize times in this case but I'll look at benchmark and
> see if
> it is significant. I think I might need to enable 'benchmark' anyway, so no
> reason to not merge it at the moment.
>

Cool I didn't know about that category when I suggested the name. SGTM


>
> https://codereview.chromium.org/148613014/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[epennerAtGoogle, 2014-02-14 22:31:47]

> > I do want to minimize times in this case but I'll look at benchmark and
> > see if
> > it is significant. I think I might need to enable 'benchmark' anyway, so no
> > reason to not merge it at the moment.
> >
> 
> Cool I didn't know about that category when I suggested the name. SGTM
> 

Okay changed the name. Thanks Ernst. Self LGTM ;)

[epenner, 2014-02-14 22:32:32]

The CQ bit was checked by epenner@chromium.org

[commit-bot: I haz the power, 2014-02-14 22:34:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/epenner@chromium.org/148613014/630001

[commit-bot: I haz the power, 2014-02-14 23:56:24]

Change committed as 251441