Add a |scope| argument to TRACE_EVENT_INSTANT* and require its presence.

Add a |scope| argument to TRACE_EVENT_INSTANT* and require its presence.

The scope indicates how long the vertical line should be in the tracing UI. It
can be global (full screen), process or thread (fill those tracks), or local
(the original style for INSTANT).


BUG=None


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

[James Simonsen, 2013-02-15 03:47:14]

Should this have been an optional parameter instead? Also, let me know if you
have a better name for "local" scope.

[nduca, 2013-02-15 19:30:56]

Can we get the scope in on the flags argument of AddTraceEvent instead of
tweaking kMaxArgs to 3? We have a flags_ field on the TraceEvent class that we
could store that on, since its really just a few bits, and then push out into
json as another toplevel parameter, e.g.:

{ph: "i", s: "l", name: unchanged, args: {unchanged}}

[nduca, 2013-02-15 19:31:22]

thread scope?

[James Simonsen, 2013-02-16 02:01:34]

On 2013/02/15 19:31:22, nduca wrote:
> thread scope?

I already have that. :(

There's:

1. Global: Spans the screen.
2. Process: Spans all trace rows in the process.
3. Thread: Spans all trace rows in the thread.
4. "Local": The little slice/tick mark we display now.

I'm operating under the assumption we'll want to maintain the existing behavior
for the existing users. If not, we can just eliminate #4.

[nduca, 2013-02-16 02:12:43]

Hmm, eliminate it. It never worked well anyway. :)

[James Simonsen, 2013-02-20 20:15:32]

On 2013/02/15 19:30:56, nduca wrote:
> Can we get the scope in on the flags argument of AddTraceEvent instead of
> tweaking kMaxArgs to 3? We have a flags_ field on the TraceEvent class that we
> could store that on, since its really just a few bits, and then push out into
> json as another toplevel parameter, e.g.:
> 
> {ph: "i", s: "l", name: unchanged, args: {unchanged}}

Done. PTAL at your leisure. This obviously isn't urgent.

[nduca, 2013-02-21 06:06:04]

I like this. LGTM.

https://codereview.chromium.org/12252058/diff/6001/base/debug/trace_event_int...
File base/debug/trace_event_internal.h (right):

https://codereview.chromium.org/12252058/diff/6001/base/debug/trace_event_int...
base/debug/trace_event_internal.h:717: #define TRACE_SCOPE_NAME_GLOBAL  ('g')
I think looking up at the phase we seem to do mostly TRACE_EVENT_  prefixen

[nduca, 2013-03-13 06:02:50]

Is this landable?

[James Simonsen, 2013-03-14 23:24:12]

On 2013/03/13 06:02:50, nduca wrote:
> Is this landable?

Yeah, I probably just need to rebase and find owners.

I kinda held off because there's nothing to view the new format: 'i' instead of
'I'. Do you want to wait for that or is it no big deal?

[nduca, 2013-03-19 00:06:58]

Probably safe. Worst case someone complains and we write a quick fix to the UI
to recognize the new format.

[James Simonsen, 2013-03-21 23:14:05]

Need a rubber-stamp lgtm from OWNERS for the Trace Event API change.

Suggested OWNERS:
    kbr@chromium.org
    darin@chromium.org

[Ken Russell (switch to Gerrit), 2013-03-22 01:02:21]

LGTM for the parts I own.

[darin (slow to review), 2013-03-23 14:30:04]

OK, LGTM

[commit-bot: I haz the power, 2013-03-25 17:36:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonjam@chromium.org/12252058/14001

[commit-bot: I haz the power, 2013-03-25 17:36:41]

Failed to apply patch for cc/animation/animation.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/animation/animation.cc
  Hunk #1 FAILED at 124.
  1 out of 1 hunk FAILED -- saving rejects to file cc/animation/animation.cc.rej

Patch:       cc/animation/animation.cc
Index: cc/animation/animation.cc
diff --git a/cc/animation/animation.cc b/cc/animation/animation.cc
index
d81713be06b43acb2e73398701775a25378f4b47..e6fab404cddbf2ade26da075e9987041eb356fa0
100644
--- a/cc/animation/animation.cc
+++ b/cc/animation/animation.cc
@@ -124,6 +124,7 @@ void Animation::SetRunState(RunState run_state, double
monotonic_time) {
 
   TRACE_EVENT_INSTANT2("cc",
                        "LayerAnimationController::setRunState",
+                       TRACE_EVENT_SCOPE_THREAD,
                        "Name",
                        TRACE_STR_COPY(nameBuffer),
                        "State",

[commit-bot: I haz the power, 2013-03-25 17:51:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonjam@chromium.org/12252058/22001

[commit-bot: I haz the power, 2013-03-25 17:51:16]

Failed to apply patch for cc/animation/animation.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/animation/animation.cc
  Hunk #1 FAILED at 124.
  1 out of 1 hunk FAILED -- saving rejects to file cc/animation/animation.cc.rej

Patch:       cc/animation/animation.cc
Index: cc/animation/animation.cc
diff --git a/cc/animation/animation.cc b/cc/animation/animation.cc
index
d81713be06b43acb2e73398701775a25378f4b47..e6fab404cddbf2ade26da075e9987041eb356fa0
100644
--- a/cc/animation/animation.cc
+++ b/cc/animation/animation.cc
@@ -124,6 +124,7 @@ void Animation::SetRunState(RunState run_state, double
monotonic_time) {
 
   TRACE_EVENT_INSTANT2("cc",
                        "LayerAnimationController::setRunState",
+                       TRACE_EVENT_SCOPE_THREAD,
                        "Name",
                        TRACE_STR_COPY(nameBuffer),
                        "State",

[commit-bot: I haz the power, 2013-03-25 18:18:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonjam@chromium.org/12252058/27001

[commit-bot: I haz the power, 2013-03-25 18:54:27]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-03-26 04:52:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonjam@chromium.org/12252058/56001

[commit-bot: I haz the power, 2013-03-26 06:49:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonjam@chromium.org/12252058/56001

[commit-bot: I haz the power, 2013-03-26 07:42:44]

Change committed as 190577

[caseq, 2013-10-21 16:38:43]

On 2013/03/19 00:06:58, nduca wrote:
> Probably safe. Worst case someone complains and we write a quick fix to the UI
> to recognize the new format.

Besides the UI, we also have a few copies of trace_event.h in different places,
so we now have this:

https://code.google.com/p/chromium/codesearch#search/&q=%22%23define%20TRACE_...
chromiumsrc/base/debug/trace_event.h
   912: #define TRACE_EVENT_PHASE_INSTANT  ('i')
chromiumsrc/third_party/WebKit/Source/platform/TraceEvent.h
   597: #define TRACE_EVENT_PHASE_INSTANT  ('I')
chromiumsrc/third_party/webrtc/system_wrappers/interface/trace_event.h
   636: #define TRACE_EVENT_PHASE_INSTANT  ('I')
chromiumsrc/third_party/angle_dx11/src/third_party/trace_event/trace_event.h
   551: #define TRACE_EVENT_PHASE_INSTANT  ('I')

In particular, we check event category in TimelineTraceEventProcessor (nothing
blew so far, as we used to only check instant events that were also produced in
WebKit land, but I stumbled over this the moment I was about to add processing
of some of chromium-side events as well).
I wonder what's the right thing to do now -- revert base/debug to using 'I',
change everywhere else or just handle both 'i' and 'I'?
I guess it would also be nice to have something similar in spirit to
Source/web/AssertMatchingEnums.cpp so this can be avoided in the future.

[James Simonsen, 2013-10-23 20:45:23]

On 2013/10/21 16:38:43, caseq wrote:
> On 2013/03/19 00:06:58, nduca wrote:
> > Probably safe. Worst case someone complains and we write a quick fix to the
UI
> > to recognize the new format.
> 
> Besides the UI, we also have a few copies of trace_event.h in different
places,
> so we now have this:
> 
>
https://code.google.com/p/chromium/codesearch#search/&q=%2522%2523define%2520...
> chromiumsrc/base/debug/trace_event.h
>    912: #define TRACE_EVENT_PHASE_INSTANT  ('i')
> chromiumsrc/third_party/WebKit/Source/platform/TraceEvent.h
>    597: #define TRACE_EVENT_PHASE_INSTANT  ('I')
> chromiumsrc/third_party/webrtc/system_wrappers/interface/trace_event.h
>    636: #define TRACE_EVENT_PHASE_INSTANT  ('I')
> chromiumsrc/third_party/angle_dx11/src/third_party/trace_event/trace_event.h
>    551: #define TRACE_EVENT_PHASE_INSTANT  ('I')
> 
> In particular, we check event category in TimelineTraceEventProcessor (nothing
> blew so far, as we used to only check instant events that were also produced
in
> WebKit land, but I stumbled over this the moment I was about to add processing
> of some of chromium-side events as well).
> I wonder what's the right thing to do now -- revert base/debug to using 'I',
> change everywhere else or just handle both 'i' and 'I'?
> I guess it would also be nice to have something similar in spirit to
> Source/web/AssertMatchingEnums.cpp so this can be avoided in the future.

We should support both. We want to be able to load old trace files that use the
old 'I' format.

[caseq, 2013-10-28 07:44:39]

On 2013/10/23 20:45:23, James Simonsen wrote:
 
> We should support both. We want to be able to load old trace files that use
the
> old 'I' format.

I agree we should support both on the trace-viewer side. The question is whether
we should _produce_ both, and whether the code that only handles trace events
currently being generated should support both. I suggest we revert to 'I' for
base/debug/trace_event.h -- are you ok with that?

[James Simonsen, 2013-10-28 19:26:32]

On 2013/10/28 07:44:39, caseq wrote:
> On 2013/10/23 20:45:23, James Simonsen wrote:
>  
> > We should support both. We want to be able to load old trace files that use
> the
> > old 'I' format.
> 
> I agree we should support both on the trace-viewer side. The question is
whether
> we should _produce_ both, and whether the code that only handles trace events
> currently being generated should support both. I suggest we revert to 'I' for
> base/debug/trace_event.h -- are you ok with that?

I thought the difference is that 'i' has the newly required scope arg and 'I'
doesn't.