Support JellyBean MR2 style of systrace (issue 12221047)

471 views
Skip to first unread message

wangx...@chromium.org

unread,
Feb 6, 2013, 2:24:03 PM2/6/13
to jciv...@chromium.org, klo...@chromium.org, chromium...@chromium.org, joi+watc...@chromium.org, dari...@chromium.org, erikwrig...@chromium.org, j...@chromium.org, klo...@chromium.org, nd...@chromium.org
Reviewers: Jay Civelli, klobag.chromium,

Description:
Support JellyBean MR2 style of systrace

JB MR2 systrace.py combined the steps of trace tags settings and
trace collection into one step, so we need to react to the change
of the change of trace tags settings to enable/disable atrace
in chrome.

The change also benefit pre-JB-MR2 systems that we don't need to
restart chrome after changing the trace tags settings in Developer
options. However, we still need "adb shell stop; adb shell start"
if we set the trace tags using pre-JB-MR2 systrace.py.

BUG=173954


Please review this at https://codereview.chromium.org/12221047/

SVN Base: https://chromium.googlesource.com/chromium/src.git@master

Affected files:
M base/debug/trace_event_android.cc
M base/debug/trace_event_impl.h
M content/common/android/trace_event_binding.cc
M
content/public/android/java/src/org/chromium/content/common/TraceEvent.java


Index: base/debug/trace_event_android.cc
diff --git a/base/debug/trace_event_android.cc
b/base/debug/trace_event_android.cc
index
29e684a659d6e17aa51f0087aae3860b16d4c206..2901a3427f22fdc815c0b5fa5c3d380a89c416ee
100644
--- a/base/debug/trace_event_android.cc
+++ b/base/debug/trace_event_android.cc
@@ -20,12 +20,21 @@ const char* kATraceMarkerFile
= "/sys/kernel/debug/tracing/trace_marker";
namespace base {
namespace debug {

-// static
-void TraceLog::InitATrace() {
- DCHECK(g_atrace_fd == -1);
- g_atrace_fd = open(kATraceMarkerFile, O_WRONLY | O_APPEND);
- if (g_atrace_fd == -1)
- LOG(WARNING) << "Couldn't open " << kATraceMarkerFile;
+void TraceLog::StartATrace() {
+ AutoLock lock(lock_);
+ if (g_atrace_fd == -1) {
+ g_atrace_fd = open(kATraceMarkerFile, O_WRONLY);
+ if (g_atrace_fd == -1)
+ LOG(WARNING) << "Couldn't open " << kATraceMarkerFile;
+ }
+}
+
+void TraceLog::StopATrace() {
+ AutoLock lock(lock_);
+ if (g_atrace_fd != -1) {
+ close(g_atrace_fd);
+ g_atrace_fd = -1;
+ }
}

void TraceLog::SendToATrace(char phase,
@@ -91,6 +100,8 @@ void TraceLog::SendToATrace(char phase,
void TraceLog::ApplyATraceEnabledFlag(unsigned char* category_enabled) {
if (g_atrace_fd != -1)
*category_enabled |= ATRACE_ENABLED;
+ else
+ *category_enabled &= ~ATRACE_ENABLED;
}

} // namespace debug
Index: base/debug/trace_event_impl.h
diff --git a/base/debug/trace_event_impl.h b/base/debug/trace_event_impl.h
index
58a44dabf0ef0c5665163bc27ab5436beb417c76..7e306e2a78f2532a1f4068242da1f7501e9eb4cd
100644
--- a/base/debug/trace_event_impl.h
+++ b/base/debug/trace_event_impl.h
@@ -209,7 +209,8 @@ class BASE_EXPORT TraceLog {
bool IsEnabled() { return !!enable_count_; }

#if defined(OS_ANDROID)
- static void InitATrace();
+ void StartATrace();
+ void StopATrace();
#endif

// Enabled state listeners give a callback when tracing is enabled or
Index: content/common/android/trace_event_binding.cc
diff --git a/content/common/android/trace_event_binding.cc
b/content/common/android/trace_event_binding.cc
index
6dd3847cdac99df42a37d1b7727c5dba88355f72..04607ed20318e648789da8e7b09e703e77f47803
100644
--- a/content/common/android/trace_event_binding.cc
+++ b/content/common/android/trace_event_binding.cc
@@ -76,8 +76,12 @@ static jboolean TraceEnabled(JNIEnv* env, jclass clazz) {
return base::debug::TraceLog::GetInstance()->IsEnabled();
}

-static void InitATrace(JNIEnv* env, jclass clazz) {
- base::debug::TraceLog::InitATrace();
+static void StartATrace(JNIEnv* env, jclass clazz) {
+ base::debug::TraceLog::GetInstance()->StartATrace();
+}
+
+static void StopATrace(JNIEnv* env, jclass clazz) {
+ base::debug::TraceLog::GetInstance()->StopATrace();
}

static void Instant(JNIEnv* env, jclass clazz,
Index:
content/public/android/java/src/org/chromium/content/common/TraceEvent.java
diff --git
a/content/public/android/java/src/org/chromium/content/common/TraceEvent.java
b/content/public/android/java/src/org/chromium/content/common/TraceEvent.java
index
67c5413093dbb453698e1106eb46b1f7f8fc97ee..835aa38e2c6d839ed5f4e897bd0db0190f51c6d6
100644
---
a/content/public/android/java/src/org/chromium/content/common/TraceEvent.java
+++
b/content/public/android/java/src/org/chromium/content/common/TraceEvent.java
@@ -37,6 +37,32 @@ public class TraceEvent {
}
}

+ static {
+ if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.JELLY_BEAN) {
+ try {
+ Class<?> systemPropertiesClass =
Class.forName("android.os.SystemProperties");
+ Method addChangeCallbackMethod =
systemPropertiesClass.getDeclaredMethod(
+ "addChangeCallback", Runnable.class);
+ addChangeCallbackMethod.invoke(null, new Runnable() {
+ @Override
+ public void run() {
+ setEnabledToMatchNative();
+ }
+ });
+ } catch (ClassNotFoundException e) {
+ Log.e("TraceEvent", "init", e);
+ } catch (NoSuchMethodException e) {
+ Log.e("TraceEvent", "init", e);
+ } catch (IllegalArgumentException e) {
+ Log.e("TraceEvent", "init", e);
+ } catch (IllegalAccessException e) {
+ Log.e("TraceEvent", "init", e);
+ } catch (InvocationTargetException e) {
+ Log.e("TraceEvent", "init", e);
+ }
+ }
+ }
+
/**
* Calling this will cause enabled() to be updated to match that set
on the native side.
* The native library must be loaded before calling this method.
@@ -47,11 +73,22 @@ public class TraceEvent {
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.JELLY_BEAN) {
try {
Class<?> traceClass = Class.forName("android.os.Trace");
- Method m = traceClass.getDeclaredMethod("isTagEnabled",
Long.TYPE);
- Field f = traceClass.getField("TRACE_TAG_VIEW");
- boolean atraceEnabled = (Boolean) m.invoke(traceClass,
f.getLong(null));
- if (atraceEnabled) nativeInitATrace();
- enabled = enabled || atraceEnabled;
+ long traceTagView =
traceClass.getField("TRACE_TAG_VIEW").getLong(null);
+ String propertyTraceTagEnableFlags =
(String)traceClass.getField(
+ "PROPERTY_TRACE_TAG_ENABLEFLAGS").get(null);
+
+ Class<?> systemPropertiesClass =
Class.forName("android.os.SystemProperties");
+ Method systemPropertiesGetIntMethod =
systemPropertiesClass.getDeclaredMethod(
+ "getInt", String.class, Integer.TYPE);
+ int enabledTags = (Integer)
systemPropertiesGetIntMethod.invoke(
+ null, propertyTraceTagEnableFlags, 0);
+ Log.d("TraceEvent", "New enabled tags: " + enabledTags);
+ if ((enabledTags & traceTagView) != 0) {
+ nativeStartATrace();
+ enabled = true;
+ } else {
+ nativeStopATrace();
+ }
} catch (ClassNotFoundException e) {
Log.e("TraceEvent", "setEnabledToMatchNative", e);
} catch (NoSuchMethodException e) {
@@ -236,7 +273,8 @@ public class TraceEvent {
}

private static native boolean nativeTraceEnabled();
- private static native void nativeInitATrace();
+ private static native void nativeStartATrace();
+ private static native void nativeStopATrace();
private static native void nativeInstant(String name, String arg);
private static native void nativeBegin(String name, String arg);
private static native void nativeEnd(String name, String arg);


klo...@chromium.org

unread,
Feb 7, 2013, 9:31:32 PM2/7/13
to wangx...@chromium.org, jciv...@chromium.org, chromium...@chromium.org, joi+watc...@chromium.org, dari...@chromium.org, erikwrig...@chromium.org, j...@chromium.org, nd...@chromium.org
"adb shell stop/start" will restart the android runtime. Can we avoid it?
e.g.
keep the old way for pre-jb-mr2.

https://codereview.chromium.org/12221047/

wangx...@chromium.org

unread,
Feb 7, 2013, 9:41:11 PM2/7/13
to jciv...@chromium.org, klo...@chromium.org, chromium...@chromium.org, joi+watc...@chromium.org, dari...@chromium.org, erikwrig...@chromium.org, j...@chromium.org, klo...@chromium.org, nd...@chromium.org
On 2013/02/08 02:31:32, klobag.chromium wrote:
> "adb shell stop/start" will restart the android runtime. Can we avoid it?
> e.g.
> keep the old way for pre-jb-mr2.

On pre-jb-mr2, "Adb shell stop/start" is only required if using systrace.py
command line to enable systrace tags. Won't need it if setting it using
Settings/Developer options UI.


https://codereview.chromium.org/12221047/

wangx...@chromium.org

unread,
Feb 12, 2013, 4:58:44 PM2/12/13
to jciv...@chromium.org, klo...@chromium.org, chromium...@chromium.org, joi+watc...@chromium.org, dari...@chromium.org, erikwrig...@chromium.org, j...@chromium.org, klo...@chromium.org, nd...@chromium.org
Jay & Grace, does this patch lgty?

https://codereview.chromium.org/12221047/

jciv...@chromium.org

unread,
Feb 13, 2013, 1:24:02 PM2/13/13
to wangx...@chromium.org, klo...@chromium.org, chromium...@chromium.org, joi+watc...@chromium.org, dari...@chromium.org, erikwrig...@chromium.org, j...@chromium.org, klo...@chromium.org, nd...@chromium.org

https://codereview.chromium.org/12221047/diff/1/content/public/android/java/src/org/chromium/content/common/TraceEvent.java
File
content/public/android/java/src/org/chromium/content/common/TraceEvent.java
(right):

https://codereview.chromium.org/12221047/diff/1/content/public/android/java/src/org/chromium/content/common/TraceEvent.java#newcode77
content/public/android/java/src/org/chromium/content/common/TraceEvent.java:77:
String propertyTraceTagEnableFlags = (String)traceClass.getField(
Nit: missing space after (String)

https://codereview.chromium.org/12221047/diff/1/content/public/android/java/src/org/chromium/content/common/TraceEvent.java#newcode83
content/public/android/java/src/org/chromium/content/common/TraceEvent.java:83:
int enabledTags = (Integer) systemPropertiesGetIntMethod.invoke(
Nit: should it be named enableFlags?

https://codereview.chromium.org/12221047/

wangx...@chromium.org

unread,
Feb 13, 2013, 4:50:17 PM2/13/13
to jciv...@chromium.org, klo...@chromium.org, j...@chromium.org, chromium...@chromium.org, joi+watc...@chromium.org, dari...@chromium.org, erikwrig...@chromium.org, j...@chromium.org, klo...@chromium.org, nd...@chromium.org
Add jar@ for base/debug/.


https://codereview.chromium.org/12221047/diff/1/content/public/android/java/src/org/chromium/content/common/TraceEvent.java
File
content/public/android/java/src/org/chromium/content/common/TraceEvent.java
(right):

https://codereview.chromium.org/12221047/diff/1/content/public/android/java/src/org/chromium/content/common/TraceEvent.java#newcode77
content/public/android/java/src/org/chromium/content/common/TraceEvent.java:77:
String propertyTraceTagEnableFlags = (String)traceClass.getField(
On 2013/02/13 18:24:02, Jay Civelli wrote:
> Nit: missing space after (String)

Done.

https://codereview.chromium.org/12221047/diff/1/content/public/android/java/src/org/chromium/content/common/TraceEvent.java#newcode83
content/public/android/java/src/org/chromium/content/common/TraceEvent.java:83:
int enabledTags = (Integer) systemPropertiesGetIntMethod.invoke(
On 2013/02/13 18:24:02, Jay Civelli wrote:
> Nit: should it be named enableFlags?

Done.

https://codereview.chromium.org/12221047/

wangx...@chromium.org

unread,
Feb 14, 2013, 12:26:58 PM2/14/13
to jciv...@chromium.org, klo...@chromium.org, j...@chromium.org, chromium...@chromium.org, joi+watc...@chromium.org, dari...@chromium.org, erikwrig...@chromium.org, j...@chromium.org, klo...@chromium.org, nd...@chromium.org

j...@chromium.org

unread,
Feb 14, 2013, 12:39:43 PM2/14/13
to wangx...@chromium.org, jciv...@chromium.org, klo...@chromium.org, chromium...@chromium.org, joi+watc...@chromium.org, dari...@chromium.org, erikwrig...@chromium.org, j...@chromium.org, klo...@chromium.org, nd...@chromium.org

jciv...@chromium.org

unread,
Feb 14, 2013, 12:56:28 PM2/14/13
to wangx...@chromium.org, klo...@chromium.org, j...@chromium.org, chromium...@chromium.org, joi+watc...@chromium.org, dari...@chromium.org, erikwrig...@chromium.org, j...@chromium.org, klo...@chromium.org, nd...@chromium.org
LGTM



https://chromiumcodereview.appspot.com/12221047/diff/7001/content/public/android/java/src/org/chromium/content/common/TraceEvent.java
File
content/public/android/java/src/org/chromium/content/common/TraceEvent.java
(right):

https://chromiumcodereview.appspot.com/12221047/diff/7001/content/public/android/java/src/org/chromium/content/common/TraceEvent.java#newcode81
content/public/android/java/src/org/chromium/content/common/TraceEvent.java:81:
Method systemPropertiesGetIntMethod =
systemPropertiesClass.getDeclaredMethod(
Nit: rename systemPropertiesGetLongMethod

https://chromiumcodereview.appspot.com/12221047/

wangx...@chromium.org

unread,
Feb 14, 2013, 1:00:55 PM2/14/13
to jciv...@chromium.org, klo...@chromium.org, j...@chromium.org, chromium...@chromium.org, joi+watc...@chromium.org, dari...@chromium.org, erikwrig...@chromium.org, j...@chromium.org, klo...@chromium.org, nd...@chromium.org

https://chromiumcodereview.appspot.com/12221047/diff/7001/content/public/android/java/src/org/chromium/content/common/TraceEvent.java
File
content/public/android/java/src/org/chromium/content/common/TraceEvent.java
(right):

https://chromiumcodereview.appspot.com/12221047/diff/7001/content/public/android/java/src/org/chromium/content/common/TraceEvent.java#newcode81
content/public/android/java/src/org/chromium/content/common/TraceEvent.java:81:
Method systemPropertiesGetIntMethod =
systemPropertiesClass.getDeclaredMethod(
On 2013/02/14 17:56:28, Jay Civelli wrote:
> Nit: rename systemPropertiesGetLongMethod

Done.

https://chromiumcodereview.appspot.com/12221047/

commi...@chromium.org

unread,
Feb 14, 2013, 1:02:54 PM2/14/13
to wangx...@chromium.org, jciv...@chromium.org, klo...@chromium.org, j...@chromium.org, chromium...@chromium.org, joi+watc...@chromium.org, dari...@chromium.org, erikwrig...@chromium.org, j...@chromium.org, klo...@chromium.org, nd...@chromium.org

commi...@chromium.org

unread,
Feb 14, 2013, 5:19:18 PM2/14/13
to wangx...@chromium.org, jciv...@chromium.org, klo...@chromium.org, j...@chromium.org, chromium...@chromium.org, joi+watc...@chromium.org, dari...@chromium.org, erikwrig...@chromium.org, j...@chromium.org, klo...@chromium.org, nd...@chromium.org

commi...@chromium.org

unread,
Feb 15, 2013, 6:28:12 AM2/15/13
to wangx...@chromium.org, jciv...@chromium.org, klo...@chromium.org, j...@chromium.org, chromium...@chromium.org, joi+watc...@chromium.org, dari...@chromium.org, erikwrig...@chromium.org, j...@chromium.org, klo...@chromium.org, nd...@chromium.org

commi...@chromium.org

unread,
Feb 15, 2013, 12:05:38 PM2/15/13
to wangx...@chromium.org, jciv...@chromium.org, klo...@chromium.org, j...@chromium.org, chromium...@chromium.org, joi+watc...@chromium.org, dari...@chromium.org, erikwrig...@chromium.org, j...@chromium.org, klo...@chromium.org, nd...@chromium.org

commi...@chromium.org

unread,
Feb 15, 2013, 12:24:45 PM2/15/13
to wangx...@chromium.org, jciv...@chromium.org, klo...@chromium.org, j...@chromium.org, chromium...@chromium.org, joi+watc...@chromium.org, dari...@chromium.org, erikwrig...@chromium.org, j...@chromium.org, klo...@chromium.org, nd...@chromium.org
Reply all
Reply to author
Forward
0 new messages