919078 - Improve async profiler startup feature list

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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 1

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug919078-async-profile-start-features-hg0.diff

Comment 2

[Benoit Girard (:BenWa)]

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 3

[Benoit Girard (:BenWa)]

Comment on attachment 808053 [details] [diff] [review]
bug919078-async-profile-start-features-hg0.diff

Waiting for info to review the patch

Comment 4

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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

Comment 5

[Kevin Grandon :kgrandon]

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.

Comment 6

[Benoit Girard (:BenWa)]

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

Comment 7

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: add js to async profiler start options

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.

Comment 8

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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.

Comment 9

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(Mid-air collision with myself.  It's definitely Friday.)

Comment 10

[Benoit Girard (:BenWa)]

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.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/331fb081db00

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/331fb081db00

Comment 13

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/eaea6570492f