Closed Bug 919078 Opened 7 years ago Closed 6 years ago

Improve async profiler startup feature list

Categories

(Core :: Gecko Profiler, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27
Tracking Status
firefox25 --- wontfix
firefox26 --- fixed
firefox27 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: jld, Assigned: jld)

References

Details

(Keywords: perf, Whiteboard: [c=profiling p= s= u=])

Attachments

(1 file, 1 obsolete file)

Starting the profiler asynchronously will be more useful if more features are enabled; in particular, "js" and "stackwalk" (or "leaf" on platforms where stack walking isn't available) are ones that I predict most users of the profiler will expect, and which the existing profiler-start-on-startup mechanism enables.
Comment on attachment 808053 [details] [diff] [review]
bug919078-async-profile-start-features-hg0.diff

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

::: tools/profiler/platform-linux.cc
@@ +447,5 @@
>    uint32_t featureCount = 0;
> +  features[featureCount++] = "js";
> +#if defined(SPS_ARCH_arm)
> +  // cf. the #if in mozilla_sampler_init
> +  features[featureCount++] = "stackwalk";

Does stackwalk work well enough for it to be on by default? Last I tried it wasn't that great but maybe I wasn't lucky. If it works well enough I guess it's fine.

I was expecting we'd modify profiler.options so that we can use profile.sh start <pid> <thread name> <options> or something like that.
Comment on attachment 808053 [details] [diff] [review]
bug919078-async-profile-start-features-hg0.diff

Waiting for info to review the patch
Attachment #808053 - Flags: review?(bgirard)
Flags: needinfo?(jld)
(In reply to Benoit Girard (:BenWa) from comment #2)
> Comment on attachment 808053 [details] [diff] [review]
> bug919078-async-profile-start-features-hg0.diff
> 
> Review of attachment 808053 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: tools/profiler/platform-linux.cc
> @@ +447,5 @@
> >    uint32_t featureCount = 0;
> > +  features[featureCount++] = "js";
> > +#if defined(SPS_ARCH_arm)
> > +  // cf. the #if in mozilla_sampler_init
> > +  features[featureCount++] = "stackwalk";
> 
> Does stackwalk work well enough for it to be on by default? Last I tried it
> wasn't that great but maybe I wasn't lucky. If it works well enough I guess
> it's fine.

It's been working pretty well on MOZ_PROFILING=1 builds of b2g.  (Non-profiling builds can't unwind Gecko, but they will unwind Gonk.)

But... is ARM the right conditional for this?  The unwinder from bug 810526 is currently enabled only for B2G, but I don't really want to duplicate those ifdefs more than they already are if I don't have to.
Flags: needinfo?(jld)
From our standup we discussed that fixing this would enable javascript in profiles when capturing a specific PID. We would *really* like to have this.
(In reply to Kevin Grandon :kgrandon from comment #5)
> From our standup we discussed that fixing this would enable javascript in
> profiles when capturing a specific PID. We would *really* like to have this.

Actually *please* just change the default for now and don't block on this:
http://mxr.mozilla.org/mozilla-central/source/tools/profiler/platform-linux.cc#471
extends features by one and hardcode "js" unconditionally. This is an omission. Such a patch will get my r+. I had this change locally a few weeks ago but I forgot to land it.
Add JS for now.  This bug should stay open until stack walk is turned on, and I think we want bug 916106 to land before that.
Attachment #816054 - Flags: review?(bgirard)
Attachment #816054 - Flags: review?(bgirard) → review+
Attachment #808053 - Attachment is obsolete: true
Checkin note: for b2g-inbound, and the other half of the original patch will be landable relatively soon (I hope), so either leave open or tell me I should clone the bug instead.
(Mid-air collision with myself.  It's definitely Friday.)
Keywords: checkin-needed
my general rule if the patch are independent and there's a chance one but not both to be backed out or cause a regression its easier having two bugs. When in doubt split the bugs.
https://hg.mozilla.org/mozilla-central/rev/331fb081db00
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Keywords: perf
Whiteboard: [c=profiling] → [c=profiling p= s=2013.10.18 u=]
Whiteboard: [c=profiling p= s=2013.10.18 u=] → [c=profiling p= s= u=]
Comment on attachment 816054 [details] [diff] [review]
add js to async profiler start options

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 914815
User impact if declined: Developers using async profiler startup won't see JS stack frames.  (No impact on end users.)  I'm in a meeting with developers complaining about not having this on b2g 1.2 as I type this, in fact.
Testing completed (on m-c, etc.): It's been on m-c for a few days, and I've successfully used it.
Risk to taking this patch (and alternatives if risky): Low risk for developers, and no risk for end users.
String or IDL/UUID changes made by this patch: None.
Attachment #816054 - Flags: approval-mozilla-aurora?
Attachment #816054 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.