I think we should be able to dvmDumpThreadStack safely from a signal handler. This should give us the information we need. We may need some dirtyness to hook this up though. Dalvik has a hacky dvmDumpRunningThreadStack that suggests that this is remotely possible. It may also be worth investigating what the debugger does to get stacks.

I've discussed this a bit with snorp and blassey. There's a few options but we're not sure at this point if they are signal safe. We could always spawn a JVM sampling thread and use: http://docs.oracle.com/javase/1.5.0/docs/api/java/lang/Thread.html#getAllStackTraces%28%29

This patch appears to be working. It's missing (1) stopping, (2) ifdefs, (3) controlling interval & sample size. This patch will also need cleopatra changes to handle the new thread in the profile file.

Here's an example of what I have so far. Of course we can't easily visualize it until bug 837460 is fixed: { "name": "Java Main Thread", "samples": [ { "frames": [ { "location": "android.os.MessageQueue.nativePollOnce()" }, { "location": "android.os.MessageQueue.next()" }, { "location": "android.os.Looper.loop()" }, { "location": "android.app.ActivityThread.main()" }, { "location": "java.lang.reflect.Method.invokeNative()" }, { "location": "java.lang.reflect.Method.invoke()" }, { "location": "com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run()" }, { "location": "com.android.internal.os.ZygoteInit.main()" }, { "location": "dalvik.system.NativeStart.main()" } ] }, ...

Note that this can land before bug 837460 but we can't see the results in it lands.

Now that bug 837460 is landed I can post links to sample profile: http://people.mozilla.com/~bgirard/cleopatra/#report=2c44df22ccb94a5edf0645171ede974b134b0193

Comment on attachment 709912 [details] [diff] [review] patch Review of attachment 709912 [details] [diff] [review]: ----------------------------------------------------------------- Looks good modulo the nits ::: mobile/android/base/GeckoJavaSampler.java @@ +16,5 @@ > + private static Thread sSamplingThread = null; > + private static boolean sPauseSampler = false; > + private static boolean sStopSampler = false; > + > + private static Sample[][] sSamples; So this is supposed to be sSamples[threadId][sampleIndex]? Maybe use a Map or something instead? Map<int, Sample[]> @@ +17,5 @@ > + private static boolean sPauseSampler = false; > + private static boolean sStopSampler = false; > + > + private static Sample[][] sSamples; > + private static int sSamplePos; I don't think any of this should be static except maybe sMainThread and LOGTAG. @@ +23,5 @@ > + > + private static class Sample { > + public Frame[] mFrames; > + public double mTime; > + public Sample(StackTraceElement[] pBT) { What is pBT? Seems like a weird name. @@ +36,5 @@ > + } > + } > + } > + private static class Frame { > + public String mFileName; I think if you're going to use it as a bare struct, just drop the mFoo stuff. mFileName -> fileName, etc. @@ +97,5 @@ > + } > + return null; > + } > + > + private synchronized static Sample getSample(int aThreadId, int aSampleId) { These methods don't need to be static once the members are fixed

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #10) > @@ +17,5 @@ > > + private static boolean sPauseSampler = false; > > + private static boolean sStopSampler = false; > > + > > + private static Sample[][] sSamples; > > + private static int sSamplePos; > > I don't think any of this should be static except maybe sMainThread and > LOGTAG. > I don't have any instances of GeckoJavaSampler. Are you suggesting that I create a singleton? I could move some of this into SamplingThread but not sSamplingThread. I synchronize all the access on 'GeckoJavaSampler.class' so this should be safe.

(In reply to Benoit Girard (:BenWa) from comment #11) > (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #10) > > @@ +17,5 @@ > > > + private static boolean sPauseSampler = false; > > > + private static boolean sStopSampler = false; > > > + > > > + private static Sample[][] sSamples; > > > + private static int sSamplePos; > > > > I don't think any of this should be static except maybe sMainThread and > > LOGTAG. > > > > I don't have any instances of GeckoJavaSampler. Are you suggesting that I > create a singleton? I could move some of this into SamplingThread but not > sSamplingThread. I synchronize all the access on 'GeckoJavaSampler.class' so > this should be safe. Oh, I actually misread the first time. Yeah, I think most of it should be in SamplingThread. Static state bugs me.

Re-requesting review for non trivial changes: -Cap the sampling interval to 10ms for the java thread because 1ms has too many gaps. -Lowered java samples to 1000. With 10ms interval we need less samples. This will give less OOM failures -I moved some variables into SamplingThread. This still leaves several static methods and I'm not convinced this was an improvement.

Blocking on bug 779291 to minimize conflicts. If there's a strong demand for this ASAP then I may revisit this decision. It should be easy to apply locally.

Kats can you review the changes to the jni-stubs?

Comment on attachment 739403 [details] [diff] [review] patch rebased r=snorp Review of attachment 739403 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine. If you want you can also put the native method in GeckoJavaSampler directly (and make it private), and add GeckoJavaSampler.java to the CLASSES_WITH_JNI variable in the Makefile. It'll be more encapsulated that way. I did the same thing in https://bugzilla.mozilla.org/attachment.cgi?id=739196&action=diff if you want an example.

Rebased again. This patch queue is really tied together.

Backed out for bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=22091352&tree=Mozilla-Inbound remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c2d4abcf30df

I keep forgetting that ANDROID also implies b2g :(

JNI symbol name fix

https://hg.mozilla.org/mozilla-central/rev/42f859a219d6 https://hg.mozilla.org/mozilla-central/rev/22b4a153b3c5

It looks like the commit here broke profiling on fennec, at least on my Galaxy Nexus.

(In reply to William Lachance (:wlach) from comment #29) > It looks like the commit here broke profiling on fennec, at least on my > Galaxy Nexus. wlach, this is referring to the bug where the awesome bar causes javascript to stop executing right?

(In reply to Benoit Girard (:BenWa) from comment #30) > (In reply to William Lachance (:wlach) from comment #29) > > It looks like the commit here broke profiling on fennec, at least on my > > Galaxy Nexus. > > wlach, this is referring to the bug where the awesome bar causes javascript > to stop executing right? No, I think this was bug 865915. I must have misdiagnosed the cause of problems: I'm not having any problems with profiling these days aside from it crashing when profiling sites that use canvas (which is actually not a profiler problem since they crash on their own too).