Support JellyBean MR2 style of systrace (issue 12221047)

Showing 1-15 of 15 messages
Support JellyBean MR2 style of systrace (issue 12221047) wangx...@chromium.org 2/6/13 11:24 AM
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);


Re: Support JellyBean MR2 style of systrace (issue 12221047) klo...@chromium.org 2/7/13 6:31 PM
"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/
Re: Support JellyBean MR2 style of systrace (issue 12221047) wangx...@chromium.org 2/7/13 6:41 PM
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/
Re: Support JellyBean MR2 style of systrace (issue 12221047) wangx...@chromium.org 2/12/13 1:58 PM
Jay & Grace, does this patch lgty?

https://codereview.chromium.org/12221047/
Re: Support JellyBean MR2 style of systrace (issue 12221047) jciv...@chromium.org 2/13/13 10:24 AM

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/
Re: Support JellyBean MR2 style of systrace (issue 12221047) wangx...@chromium.org 2/13/13 1:50 PM
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/
Re: Support JellyBean MR2 style of systrace (issue 12221047) wangx...@chromium.org 2/14/13 9:26 AM
Re: Support JellyBean MR2 style of systrace (issue 12221047) Jim Roskind 2/14/13 9:39 AM
Re: Support JellyBean MR2 style of systrace (issue 12221047) jciv...@chromium.org 2/14/13 9:56 AM
Re: Support JellyBean MR2 style of systrace (issue 12221047) wangx...@chromium.org 2/14/13 10:00 AM
Re: Support JellyBean MR2 style of systrace (issue 12221047) commi...@chromium.org 2/14/13 10:02 AM
Re: Support JellyBean MR2 style of systrace (issue 12221047) commi...@chromium.org 2/14/13 2:19 PM
Re: Support JellyBean MR2 style of systrace (issue 12221047) commi...@chromium.org 2/15/13 3:28 AM
Re: Support JellyBean MR2 style of systrace (issue 12221047) commi...@chromium.org 2/15/13 9:05 AM
Re: Support JellyBean MR2 style of systrace (issue 12221047) commi...@chromium.org 2/15/13 9:24 AM