Closed Bug 888473 Opened 11 years ago Closed 11 years ago

Startup profiling should start GeckoJavaSampler right away

Categories

(Core :: Gecko Profiler, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: BenWa, Assigned: jchen)

Details

Attachments

(2 files, 1 obsolete file)

When requesting startup profiling we use MOZ_PROFILER_STARTUP which the main gecko thread will check in XRE_Main and use JNI to start GeckoJavaSampler. We should start GeckoJavaSampler immediately in anticipation if MOZ_PROFILER_STARTUP is set. GeckoJavaSampler doesn't depend on anything from the profiler except 'getProfilerTime' so that might be the hardest part.
Assignee: nobody → nchen
Status: NEW → ASSIGNED
This patch checks for the MOZ_PROFILER_STARTUP env var early on and starts the Java profiler ASAP. Because getProfilerTime doesn't work yet at this point, this patch relies on SystemClock.elapsedRealtime() to keep track of time. When the sample time is read back later, we rebase the SystemClock time to the profiler time; this results in negative sample times, but Cleopatra seems to handle it fine.
Attachment #772682 - Flags: review?(bgirard)
Attached image Sample profile timeline
This is a sample profile timeline with the patch. You can see the negative sample times on the Java thread, and lots of activity being captured before the profiler starts on the Gecko side.
Comment on attachment 772682 [details] [diff] [review]
Start GeckoJavaSampler early when profiling startup (v1)

Review of attachment 772682 [details] [diff] [review]:
-----------------------------------------------------------------

Great idea for normalizing the time.

::: mobile/android/base/GeckoApp.java
@@ +1151,5 @@
> +     **/
> +    protected void earlyStartJavaSampler(Intent intent)
> +    {
> +        for (int c = 0;; c++) {
> +            String env = intent.getStringExtra("env" + c);

I'm not a fan of duplicating the environment reading code and also doing string building on startup. Is it worth doing all the environment related work here. Or maybe at least have an early return if env0 isn't set:
if (intent.getStringExtra("env0") == null)
  return;

Should speed up the general case.

::: mobile/android/base/GeckoJavaSampler.java
@@ +29,5 @@
>          public Sample(StackTraceElement[] aStack) {
>              mFrames = new Frame[aStack.length];
> +            try {
> +                mTime = getProfilerTime();
> +            } catch (UnsupportedOperationException e) {

I don't like relying on exception for expected behavior. We should track if we expect gecko to be up instead.

@@ +33,5 @@
> +            } catch (UnsupportedOperationException e) {
> +            }
> +            if (mTime == 0.0d) {
> +                // getProfilerTime is not available yet
> +                mJavaTime = SystemClock.elapsedRealtime();

Good idea.
Attachment #772682 - Flags: review?(bgirard) → review-
(In reply to Benoit Girard (:BenWa) from comment #3)
> Comment on attachment 772682 [details] [diff] [review]
> Start GeckoJavaSampler early when profiling startup (v1)
> 
> ::: mobile/android/base/GeckoApp.java
> @@ +1151,5 @@
> > +     **/
> > +    protected void earlyStartJavaSampler(Intent intent)
> > +    {
> > +        for (int c = 0;; c++) {
> > +            String env = intent.getStringExtra("env" + c);
> 
> I'm not a fan of duplicating the environment reading code and also doing
> string building on startup. Is it worth doing all the environment related
> work here. Or maybe at least have an early return if env0 isn't set:
> if (intent.getStringExtra("env0") == null)
>   return;
> 
> Should speed up the general case.

Added the env0 check. The general env var handling code runs pretty late in the startup process, after we spawn the Gecko thread [1]. That code is also tied to a bunch of other code that sets up the environment for Gecko [2], so I don't really want to move the env var code out of that block.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoThread.java#103
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/mozglue/GeckoLoader.java.in#122

> ::: mobile/android/base/GeckoJavaSampler.java
> @@ +29,5 @@
> >          public Sample(StackTraceElement[] aStack) {
> >              mFrames = new Frame[aStack.length];
> > +            try {
> > +                mTime = getProfilerTime();
> > +            } catch (UnsupportedOperationException e) {
> 
> I don't like relying on exception for expected behavior. We should track if
> we expect gecko to be up instead.

Added a flag to check whether libxul has been loaded or not.
Attachment #772682 - Attachment is obsolete: true
Attachment #772796 - Flags: review?(bgirard)
Comment on attachment 772796 [details] [diff] [review]
Start GeckoJavaSampler early when profiling startup (v2)

:)
Attachment #772796 - Flags: review?(bgirard) → review+
https://hg.mozilla.org/mozilla-central/rev/b50e33bfa0f0
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: