1167835 - Perf tools should be enabled by default only when e10s is an option

Status: RESOLVED WONTFIX

(DevTools :: Performance Tools (Profiler/Timeline), defect)

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

From Jeff: it should be disabled by default in release beta until they get e10s

Comment 2

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1167835-perf-pref.patch

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8612540 [details] [diff] [review]
1167835-perf-pref.patch

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: The performance tool will be uplifted and enabled by default in environments that cannot fully use the tool (non-e10s)
[Describe test coverage new/current, TreeHerder]: n/a
[Risks and why]: Risk users on non-e10s having a less than lovely experience when using this tool in non-e10s
[String/UUID change made/needed]: none

** This does not need to make it's way to the June 2nd release, but should land in Aurora sometime this cycle **

Comment 4

[Victor Porof [:vporof][:vp]]

Comment on attachment 8612540 [details] [diff] [review]
1167835-perf-pref.patch

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

Wait, so, if e10s is not an option they don't get anything?!

I thought we'd still ship on non-e10s browsers, this but disable live updates and remove the "turn on e10s" message.

Tentatively r- unless I'm horribly mistaken.

Comment 5

[Jordan Santell [:jsantell] [@jsantell]]

If e10s is not an option, they just don't get [performance] by default on their devtools tab, unless they enable it from the toolbox options.

Yes it'll still work and they do not get live updates

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8612540 [details] [diff] [review]
1167835-perf-pref.patch

^

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #5)
> If e10s is not an option, they just don't get [performance] by default on
> their devtools tab, unless they enable it from the toolbox options.
> 
> Yes it'll still work and they do not get live updates

What?!? Most users that aren't on Nightly aren't using e10s. Now they lose a profiler by default?? This sounds like a horrible idea.

Comment 8

[Victor Porof [:vporof][:vp]]

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #7)
> (In reply to Jordan Santell [:jsantell] [@jsantell] from comment #5)

Crisis averted on IRC.

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #7)
> (In reply to Jordan Santell [:jsantell] [@jsantell] from comment #5)
> > If e10s is not an option, they just don't get [performance] by default on
> > their devtools tab, unless they enable it from the toolbox options.
> > 
> > Yes it'll still work and they do not get live updates
> 
> What?!? Most users that aren't on Nightly aren't using e10s. Now they lose a
> profiler by default?? This sounds like a horrible idea.

So what's the plan?  Is this a WONTFIX?

Comment 10

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Brian Grinstead [:bgrins] from comment #9)
> (In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #7)
> > (In reply to Jordan Santell [:jsantell] [@jsantell] from comment #5)
> > > If e10s is not an option, they just don't get [performance] by default on
> > > their devtools tab, unless they enable it from the toolbox options.
> > > 
> > > Yes it'll still work and they do not get live updates
> > 
> > What?!? Most users that aren't on Nightly aren't using e10s. Now they lose a
> > profiler by default?? This sounds like a horrible idea.
> 
> So what's the plan?  Is this a WONTFIX?

Looks like it. I'd forgotten that perftools were on by default and gave Jordan bad advice.

To be clear about what we want:

* for dev edition 40.1, we want perf tools enabled by default
* for beta 40 next month, there is no e10s, so same but make sure whatever shouldn't be turned on isn't.
* when e10s goes to beta ( 41? 42? ), we ensure that perf tools are fully enabled.

AFAICT there is no emergency, we're happy with what's enabled etc in Dev Edition 40.1 for Tuesday already right?

Comment 11

[Axel Kratel]

Not to add too many cooks here, but I just want to clarify a few things: 

1) Perf tools should be on by default in all subsequent releases
2) Anything that gets clobbered by lack of e10s should be turned off by default as long as e10s is not available.
3) Without e10s there is no live view, no memory metric in timeline, no allocation chart and no allocation tree. 
4) beta 40 should really have dev edition 40.1 perf features. 

Now here are the things that aren't so clear to me: 

5) If someone tries to turn on any e10s dependent feature, do they get prompted to turn on e10s?
6) Do we get e10s in 41 turned on by default?
7) Is there a programmatic way to automatically turn on e10s if someone enables an e10s dependent feature?

Comment 12

[Jordan Santell [:jsantell] [@jsantell]]

5) The only e10s dependent feature is realtime rendering, which isn't something that's toggleable -- e10s is a hard requirement
6) This is what I've been hearing from the e10s team: https://wiki.mozilla.org/Electrolysis#Schedule
7) Right now, if e10s is possible on the platform, the message says to enable it in `about:preferences` -- we might be able to flip the pref for them, but then we'd have to prompt for a restart, and maybe there's more going on here, but the only dependent feature is the realtime graphs

Comment 13

[Brian Grinstead [:bgrins]]

I don't think we need to proceed with this bug (or uplift it to 40).  The tool will gracefully degrade without e10s and give a message about why.  Because of that, hiding the tool would just cause needless confusion.  I'm not going to track this for uplift.  Feel free to reopen the bug if you'd like.