Make Chrome Trace work with Android ATrace.

Make Chrome Trace work with Android ATrace.

It uses "view" tag in the ATrace to enable tracing Chrome events.

Steps to grab a trace:

1. Enable trace of "view" in the Developer options.

2. adb shell stop; adb shell start
  or if the system itself supports changing trace settings without restart:
   adb shell am force-stop com.android.chrome (for stable version)
   adb shell am force-stop com.google.android.apps.chrome (for beta/dev version)

3. Start Chrome

4. // grab a 5-seconds trace, the result is in trace.html
   third_party/android_tools/sdk/tools/systrace/systrace.py -t 5 -f -w

Note: the intent triggered Chrome trace still works with this patch.

BUG=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=165507

[epenner, 2012-10-29 23:58:54]

Awesome, I just tried this and it's working well. Only issue is the large amount
of trace names due to adding the args to the trace names. Should we maybe add
that part later or put it behind a flag to be enabled only when needed?

https://chromiumcodereview.appspot.com/11345019/diff/1/base/debug/trace_event...
File base/debug/trace_event_android.cc (right):

https://chromiumcodereview.appspot.com/11345019/diff/1/base/debug/trace_event...
base/debug/trace_event_android.cc:54: for (int i = 0; i < num_args; ++i) {
I just tried this patch and I think this causes some "information overload" in
the traces. The args are definitely good to have, but I'm not sure it's worth it
without formal support from atrace.

[Xianzhu, 2012-10-30 02:21:31]

https://chromiumcodereview.appspot.com/11345019/diff/1/base/debug/trace_event...
File base/debug/trace_event_android.cc (right):

https://chromiumcodereview.appspot.com/11345019/diff/1/base/debug/trace_event...
base/debug/trace_event_android.cc:54: for (int i = 0; i < num_args; ++i) {
On 2012/10/29 23:58:55, epenner wrote:
> I just tried this patch and I think this causes some "information overload" in
> the traces. The args are definitely good to have, but I'm not sure it's worth
it
> without formal support from atrace.

In the last patch set instead of appending the arguments to the name, I append
the arguments separated by '|'s so that they won't be displayed in the name.
Will modify the atrace script to show the arguments.

[brettw, 2012-10-30 04:53:55]

owners lgtm rubberstamp. Please wait for review by jbates who seems to be the
one who's done the most work in this area.

https://chromiumcodereview.appspot.com/11345019/diff/4001/base/debug/trace_ev...
File base/debug/trace_event.h (right):

https://chromiumcodereview.appspot.com/11345019/diff/4001/base/debug/trace_ev...
base/debug/trace_event.h:602: #define TRACE_EVENT_API_IS_ATRACE_ENABLED() \
I don't see why this needs to be a #define. An inline function would be strictly
better. I'd put it in the base::internal namespace.

[Xianzhu, 2012-10-30 17:52:19]

https://chromiumcodereview.appspot.com/11345019/diff/4001/base/debug/trace_ev...
File base/debug/trace_event.h (right):

https://chromiumcodereview.appspot.com/11345019/diff/4001/base/debug/trace_ev...
base/debug/trace_event.h:602: #define TRACE_EVENT_API_IS_ATRACE_ENABLED() \
On 2012/10/30 04:53:56, brettw wrote:
> I don't see why this needs to be a #define. An inline function would be
strictly
> better. I'd put it in the base::internal namespace.

Noticed that all dependencies to trace_event_impl.h have been wrapped with
TRACE_EVENT_API_xxx macros, and I followed that. Seems a convention since
https://chromiumcodereview.appspot.com/9213013.

[jbates, 2012-10-30 20:22:14]

It looks like when InitATrace is called, tracing will permanently be turned on
until chrome is restarted. That what you want?

https://chromiumcodereview.appspot.com/11345019/diff/4001/base/debug/trace_ev...
File base/debug/trace_event_android.cc (right):

https://chromiumcodereview.appspot.com/11345019/diff/4001/base/debug/trace_ev...
base/debug/trace_event_android.cc:51: std::string out =
StringPrintf("B|%d|%s-%s", getpid(), category, name);
There's plenty of room for performance improvements here, fyi

https://chromiumcodereview.appspot.com/11345019/diff/4001/base/debug/trace_ev...
File base/debug/trace_event_impl.cc (right):

https://chromiumcodereview.appspot.com/11345019/diff/4001/base/debug/trace_ev...
base/debug/trace_event_impl.cc:611: if (IsATraceEnabled()) {
This if looks redundant

[Xianzhu, 2012-10-30 21:45:01]

> It looks like when InitATrace is called, tracing will permanently be turned on
> until chrome is restarted. That what you want?

Yes. We restart the system after changing the trace settings.

https://chromiumcodereview.appspot.com/11345019/diff/4001/base/debug/trace_ev...
File base/debug/trace_event_android.cc (right):

https://chromiumcodereview.appspot.com/11345019/diff/4001/base/debug/trace_ev...
base/debug/trace_event_android.cc:51: std::string out =
StringPrintf("B|%d|%s-%s", getpid(), category, name);
On 2012/10/30 20:22:14, jbates wrote:
> There's plenty of room for performance improvements here, fyi

Could you be more specific? Do you mean the "+=", not using StringPrintf, or sth
else?

https://chromiumcodereview.appspot.com/11345019/diff/4001/base/debug/trace_ev...
File base/debug/trace_event_impl.cc (right):

https://chromiumcodereview.appspot.com/11345019/diff/4001/base/debug/trace_ev...
base/debug/trace_event_impl.cc:611: if (IsATraceEnabled()) {
On 2012/10/30 20:22:14, jbates wrote:
> This if looks redundant

Done.

[jbates, 2012-10-31 22:26:01]

LGTM

https://chromiumcodereview.appspot.com/11345019/diff/4001/base/debug/trace_ev...
File base/debug/trace_event_android.cc (right):

https://chromiumcodereview.appspot.com/11345019/diff/4001/base/debug/trace_ev...
base/debug/trace_event_android.cc:51: std::string out =
StringPrintf("B|%d|%s-%s", getpid(), category, name);
On 2012/10/30 21:45:01, Xianzhu wrote:
> On 2012/10/30 20:22:14, jbates wrote:
> > There's plenty of room for performance improvements here, fyi
> 
> Could you be more specific? Do you mean the "+=", not using StringPrintf, or
sth
> else?

Yep, StringPrintf and std::string temporaries are inefficient. JSON conversion
below probably trumps it all though so I'm assuming you haven't ran into a
problem with performance. It's no worse than Java I guess ;)

[Tom Hudson, 2012-11-01 14:59:56]

On 2012/10/30 21:45:01, Xianzhu wrote:
> > It looks like when InitATrace is called, tracing will permanently be turned
on
> > until chrome is restarted. That what you want?
> 
> Yes. We restart the system after changing the trace settings.

Do you mean that you assume the user will restart the system after changing the
trace settings? As I understood it from talking to Grace, you can use Settings >
Developer options > Enable traces > View to change the trace settings, and that
doesn't require restarting the system to have an effect.

[Xianzhu, 2012-11-01 16:25:29]

On 2012/11/01 14:59:56, Tom Hudson wrote:
> 
> Do you mean that you assume the user will restart the system after changing
the
> trace settings? As I understood it from talking to Grace, you can use Settings
>
> Developer options > Enable traces > View to change the trace settings, and
that
> doesn't require restarting the system to have an effect.

I'm wondering if the trace settings in developer options could take effect in
the whole system without "adb shell stop; adb shell start". If this is the way
the newest Android system works, we can change to adopt it.

[Xianzhu, 2012-11-01 17:54:38]

Just verified that the Android system (at most) JOP01+ can respond to change of
trace settings without "adb shell stop; adb shell start". So I'm going to let
Chrome conform to it.

[Xianzhu, 2012-11-01 18:40:15]

We need to depend on another hidden API android.os.SystemProperties to achieve
real-time trace settings. To avoid that I'm keeping the current change which
still require restarting Chrome (or "adb shell stop; adb shell start") after
changing the trace options as stated in the Descriptions of this change.

[commit-bot: I haz the power, 2012-11-01 18:45:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/11345019/7004

[commit-bot: I haz the power, 2012-11-01 21:38:17]

Change committed as 165507

[pasko, 2015-10-26 15:12:53]

pasko@chromium.org changed reviewers:
+ pasko@chromium.org

[pasko, 2015-10-26 15:12:54]

https://chromiumcodereview.appspot.com/11345019/diff/7004/base/debug/trace_ev...
File base/debug/trace_event_android.cc (right):

https://chromiumcodereview.appspot.com/11345019/diff/7004/base/debug/trace_ev...
base/debug/trace_event_android.cc:64: write(g_atrace_fd, out.c_str(),
out.size());
I am wondering why there is no protection against short writes here in a form
of:
HANDLE_EINTR(write(...))

is it intentional?

[Xianzhu, 2015-10-26 16:04:24]

https://chromiumcodereview.appspot.com/11345019/diff/7004/base/debug/trace_ev...
File base/debug/trace_event_android.cc (right):

https://chromiumcodereview.appspot.com/11345019/diff/7004/base/debug/trace_ev...
base/debug/trace_event_android.cc:64: write(g_atrace_fd, out.c_str(),
out.size());
On 2015/10/26 15:12:54, pasko wrote:
> I am wondering why there is no protection against short writes here in a form
> of:
> HANDLE_EINTR(write(...))
> 
> is it intentional?

Not intentional.

[pasko, 2015-10-26 17:16:31]

https://chromiumcodereview.appspot.com/11345019/diff/7004/base/debug/trace_ev...
File base/debug/trace_event_android.cc (right):

https://chromiumcodereview.appspot.com/11345019/diff/7004/base/debug/trace_ev...
base/debug/trace_event_android.cc:64: write(g_atrace_fd, out.c_str(),
out.size());
On 2015/10/26 16:04:24, Xianzhu wrote:
> On 2015/10/26 15:12:54, pasko wrote:
> > I am wondering why there is no protection against short writes here in a
form
> > of:
> > HANDLE_EINTR(write(...))
> > 
> > is it intentional?
> 
> Not intentional.

whew, thanks, added for tracking:
https://code.google.com/p/chromium/issues/detail?id=547825