Add support for dalvik profiling

RESOLVED FIXED in mozilla23

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jrmuizel, Assigned: BenWa)

Tracking

unspecified
mozilla23
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 11 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
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
(Assignee)

Comment 2

6 years ago
Created attachment 709451 [details] [diff] [review]
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.
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
(Assignee)

Updated

6 years ago
Depends on: 837460
(Assignee)

Comment 3

6 years ago
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()"
              }
            ]
          },
...
(Assignee)

Comment 4

6 years ago
Created attachment 709716 [details] [diff] [review]
patch
Attachment #709451 - Attachment is obsolete: true
(Assignee)

Comment 5

6 years ago
Created attachment 709717 [details] [diff] [review]
patch
Attachment #709716 - Attachment is obsolete: true
OS: Mac OS X → Android
Hardware: x86 → All
(Assignee)

Comment 6

6 years ago
Created attachment 709911 [details] [diff] [review]
patch
Attachment #709717 - Attachment is obsolete: true
Attachment #709911 - Flags: review?(snorp)
(Assignee)

Comment 7

6 years ago
Created attachment 709912 [details] [diff] [review]
patch
Attachment #709911 - Attachment is obsolete: true
Attachment #709911 - Flags: review?(snorp)
Attachment #709912 - Flags: review?(snorp)
(Assignee)

Comment 8

6 years ago
Note that this can land before bug 837460 but we can't see the results in it lands.
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
Attachment #709912 - Flags: review?(snorp) → review+
(Assignee)

Comment 11

6 years ago
(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.
(Assignee)

Comment 13

6 years ago
Created attachment 710928 [details] [diff] [review]
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.
Attachment #709912 - Attachment is obsolete: true
Attachment #710928 - Flags: review?(snorp)
Attachment #710928 - Flags: review?(snorp) → review+
(Assignee)

Comment 14

6 years ago
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.
Depends on: 779291
(Assignee)

Comment 15

6 years ago
Created attachment 739403 [details] [diff] [review]
patch rebased r=snorp

Kats can you review the changes to the jni-stubs?
Attachment #710928 - Attachment is obsolete: true
Attachment #739403 - Flags: review?(bugmail.mozilla)
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.
Attachment #739403 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 18

6 years ago
Created attachment 739683 [details] [diff] [review]
patch v2 r=snorp,kats
Attachment #739403 - Attachment is obsolete: true
Attachment #739683 - Flags: review+
(Assignee)

Comment 19

6 years ago
Created attachment 739693 [details] [diff] [review]
patch v2 r=snorp,kats
Attachment #739683 - Attachment is obsolete: true
Attachment #739693 - Flags: review+
(Assignee)

Comment 20

6 years ago
Created attachment 739744 [details] [diff] [review]
patch v2 rebased r=snorp,kats

Rebased again. This patch queue is really tied together.
Attachment #739693 - Attachment is obsolete: true
Attachment #739744 - Flags: review+
(Assignee)

Comment 23

6 years ago
Created attachment 740365 [details] [diff] [review]
patch v2 rebased r=snorp,kats

I keep forgetting that ANDROID also implies b2g :(
Attachment #739744 - Attachment is obsolete: true
Attachment #740365 - Flags: review+
(Assignee)

Comment 27

6 years ago
Created attachment 740899 [details] [diff] [review]
patch v3 r=snorp,kats

JNI symbol name fix
Attachment #740365 - Attachment is obsolete: true
Attachment #740899 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/42f859a219d6
https://hg.mozilla.org/mozilla-central/rev/22b4a153b3c5
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
It looks like the commit here broke profiling on fennec, at least on my Galaxy Nexus.
(Assignee)

Comment 30

6 years ago
(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).
You need to log in before you can comment on or make changes to this bug.