788022 - Add support for dalvik profiling

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 1

[Benoit Girard (:BenWa)]

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

Comment 2

[Benoit Girard (:BenWa)]

Attachment: patch

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.

Comment 3

[Benoit Girard (:BenWa)]

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()"
              }
            ]
          },
...

Comment 4

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 5

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 6

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 7

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 8

[Benoit Girard (:BenWa)]

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

Comment 9

[Benoit Girard (:BenWa)]

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

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

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

Comment 11

[Benoit Girard (:BenWa)]

(In reply to James Willcox (:snorp) ([e-mail]) 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.

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Benoit Girard (:BenWa) from comment #11)
> (In reply to James Willcox (:snorp) ([e-mail]) 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.

Comment 13

[Benoit Girard (:BenWa)]

Attachment: patch

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.

Comment 14

[Benoit Girard (:BenWa)]

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.

Comment 15

[Benoit Girard (:BenWa)]

Attachment: patch rebased r=snorp

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

Comment 16

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=f91d748329ca

Comment 17

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 18

[Benoit Girard (:BenWa)]

Attachment: patch v2 r=snorp,kats

Comment 19

[Benoit Girard (:BenWa)]

Attachment: patch v2 r=snorp,kats

Comment 20

[Benoit Girard (:BenWa)]

Attachment: patch v2 rebased r=snorp,kats

Rebased again. This patch queue is really tied together.

Comment 21

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/26ab26b299a5

Comment 22

[Ed Morley [:emorley]]

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

Comment 23

[Benoit Girard (:BenWa)]

Attachment: patch v2 rebased r=snorp,kats

I keep forgetting that ANDROID also implies b2g :(

Comment 24

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=cd8ceefc2683

Comment 25

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=44aae84a0c0a

Comment 26

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=0cc538e7a31a

Comment 27

[Benoit Girard (:BenWa)]

Attachment: patch v3 r=snorp,kats

JNI symbol name fix

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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

Comment 29

[William Lachance (:wlach)]

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

Comment 30

[Benoit Girard (:BenWa)]

(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?

Comment 31

[William Lachance (:wlach)]

(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).