Closed
Bug 888473
Opened 11 years ago
Closed 11 years ago
Startup profiling should start GeckoJavaSampler right away
Categories
(Core :: Gecko Profiler, defect)
Tracking
()
RESOLVED
FIXED
mozilla25
People
(Reporter: BenWa, Assigned: jchen)
Details
Attachments
(2 files, 1 obsolete file)
33.63 KB,
image/png
|
Details | |
8.82 KB,
patch
|
BenWa
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Updated•11 years ago
|
Assignee: nobody → nchen
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•11 years ago
|
||
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)
Assignee | ||
Comment 2•11 years ago
|
||
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.
Reporter | ||
Comment 3•11 years ago
|
||
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.
Reporter | ||
Updated•11 years ago
|
Attachment #772682 -
Flags: review?(bgirard) → review-
Assignee | ||
Comment 4•11 years ago
|
||
(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)
Reporter | ||
Comment 5•11 years ago
|
||
Comment on attachment 772796 [details] [diff] [review] Start GeckoJavaSampler early when profiling startup (v2) :)
Attachment #772796 -
Flags: review?(bgirard) → review+
Assignee | ||
Comment 6•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b50e33bfa0f0
Comment 7•11 years ago
|
||
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.
Description
•