Closed Bug 1241298 Opened 6 years ago Closed 5 years ago

Enable memory panel by default

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(firefox50 verified)

VERIFIED FIXED
Firefox 50
Tracking Status
firefox50 --- verified

People

(Reporter: fitzgen, Assigned: fitzgen)

References

Details

(Whiteboard: [MemShrink:P?])

Attachments

(3 files, 2 obsolete files)

Even devs within mozilla who use our tools a lot and frequently report bugs in them don't know (a) that the memory panel exists, nor (b) how to enable the memory panel:

http://logs.glob.uno/?c=mozilla%23jsapi&s=13+Jan+2016&e=13+Jan+2016&h=memory#c683568
> 23:46	njn_	jimb, fitzgen: how do I invoke the memory profiler in devtools?
> 23:46	njn_	I thought there was a "memory" pane, but I don't see one
> 23:49	njn_	oh, I have to enable the memory tools in the settings

http://logs.glob.uno/?c=mozilla%23jsapi&s=18+Jan+2016&e=18+Jan+2016&h=memory#c684707
> 22:40	njn	jimb, fitzgen: can we show the memory panel in devtools by default? *nobody* knows that tool exists
> 22:40	njn	(I just had to tell Jukka about it)

FWIW, Chrome has their heap snapshots enabled by default (although under the "profiles" panel).

See also bug 1238982, to make non-enabled panels easier to discover, but we should still make the memory panel enabled by default.
Whiteboard: [MemShrink:P?]
Attachment #8710152 - Attachment is obsolete: true
Attachment #8710152 - Flags: review?(jsantell)
Attachment #8710190 - Attachment is obsolete: true
Attachment #8710190 - Flags: review?(jsantell)
There was a test for the addon debugging panels that was failing when this became a new default, however after some investigation the memory panel doesn't seem to work with addon debugging (bug 1241333) so I disabled it for those targets.

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d68dc7f4c79e
Comment on attachment 8710191 [details] [diff] [review]
Enable the devtools' memory panel by default

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

Don't know if this should be on by default, but not my call -- maybe just in dev edition, nightly? Either way, curious to see if there's more usage if this is available! Do we have any telemetry of real usage rather than just opening the tool? Like # of snapshots per user? Good chance we can see some good data from this change so let's take advantage of it
Attachment #8710191 - Flags: review?(jsantell) → review+
Helen should weigh in on this.
Flags: needinfo?(hholmes)
Flags: needinfo?(hholmes)
I think that part of the issue is discovery—people don't seem to be going into the Settings to add/remove panels. I'm hoping that Bug 1238982 will help fix that.

I'm inclined otherwise to agree with Jordan (perhaps it should be enabled in Dev Edition/Nightly, but not release), but it's also fairly subjective and difficult to test (are people not using the tool because it's not enabled by default so Telemetry metrics are off, or is it actually a lesser-used tool?). Since it looks like it's being turned on by default, I'd really like to make sure we learn from this with Telemetry data.
Enabling for dev edition but not beta and release doesn't make sense for devtools-related features: dev edition is the channel we market to developers, so in practice it is equivalent to release for devtools-related features. With that in mind, the only combinations that make sense for devtools-related features are:

(a) Off-by-default everywhere, 
(b) on-by-default in Nightly and off-by-default everywhere else, and 
(c) on-by-default everywhere.

Whatever we end up with, it should be of these options.

In my mind, (a) makes sense for niche panels that might not be useful to most developers (eg the web audio panel), (b) makes the most sense for "experimental", "wip", and "v0" features that we would like to eventually enable everywhere by default, and (c) is for tools that are broadly useful for most developers.

---------------------------------------------------------------------

Currently, we are using (a) for the memory panel, but I think it makes sense to transition to (c):

* Memory/GC is not a niche-ish web platform feature; it is a fundamental part the browser regardless what kind of web platform APIs you are using.
* Time in GC and memory footprint is often a performance bottleneck or source of excess slop.
* Minimizing the memory footprint is very important for mobile games/sites/apps to to avoid getting OOM killed.
* Both Chrome and IE have their heap snapshots / memory tools enabled by default.

---------------------------------------------------------------------

The try push looks good. Who has final say on whether we should land the patch or not?
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #9)
> Enabling for dev edition but not beta and release doesn't make sense for
> devtools-related features: dev edition is the channel we market to
> developers, so in practice it is equivalent to release for devtools-related
> features. With that in mind, the only combinations that make sense for
> devtools-related features are:
> 
> (a) Off-by-default everywhere, 
> (b) on-by-default in Nightly and off-by-default everywhere else, and 
> (c) on-by-default everywhere.
> 
> Whatever we end up with, it should be of these options.
> 
> In my mind, (a) makes sense for niche panels that might not be useful to
> most developers (eg the web audio panel), (b) makes the most sense for
> "experimental", "wip", and "v0" features that we would like to eventually
> enable everywhere by default, and (c) is for tools that are broadly useful
> for most developers.
> 
> ---------------------------------------------------------------------
> 

Agreed with these points

> Currently, we are using (a) for the memory panel, but I think it makes sense
> to transition to (c):
> 
> * Memory/GC is not a niche-ish web platform feature; it is a fundamental
> part the browser regardless what kind of web platform APIs you are using.
> * Time in GC and memory footprint is often a performance bottleneck or
> source of excess slop.
> * Minimizing the memory footprint is very important for mobile
> games/sites/apps to to avoid getting OOM killed.
> * Both Chrome and IE have their heap snapshots / memory tools enabled by
> default.
> 

With the current toolbox UI, adding a new tool by default is sort of a big deal.  We are already nearly overflowing on a clean profile at 1024 px width (do we have data on the most common px width in firefox?) and I think adding one more tool will push it over the edge.  At least it makes the tabs much more likely to overflow which makes the toolbox feel cluttered and makes other tools hard to find.  My 2 cents - I'd rather do Bug 1238982 (as in Comment 8) in the short term which would help *a lot* with the discoverability issue.  And in parallel or later on tackle UI changes needed to make it easier to accommodate more tools.

> ---------------------------------------------------------------------
> 
> The try push looks good. Who has final say on whether we should land the
> patch or not?

I'd think at least Helen and Bryan should be in the loop about this.
Flags: needinfo?(clarkbw)
Attached image clean-profile-1024.png
Example of what tools currently look like on a clean profile and ~ 1024 width
Also, the Storage Panel is disabled by default, mostly because of incompleteness.  Once that's fixed, that should also be surfaced somehow but we don't have real estate for it, especially not if mem panel is added..  That's why I prefer doing something like bug 1239859 since it helps for all non-default panels.
See Also: → 1239859
(In reply to Brian Grinstead [:bgrins] from comment #13)
> That's why I prefer doing something like bug 1239859 since it helps for all
> non-default panels.

That bug is definitely sorely needed, but is certainly not mutually exclusive with this bug.
(In reply to Brian Grinstead [:bgrins] from comment #10)
> We are already nearly overflowing on a clean profile at 1024 px width
> (do we have data on the most common px width in firefox?)

From Jeff's telemetry survey as of June 2015[1], most DevTools users have 1280px or larger screen width.  However, there is also a large "other" bucket that makes the data hard to reason about.

[1]: https://docs.google.com/spreadsheets/d/1pp4EO1o0wpOr2webx-TmzNhl-yMn75pXc_Nw-zmFPaQ/edit#gid=2119694026
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #15)
> (In reply to Brian Grinstead [:bgrins] from comment #10)
> > We are already nearly overflowing on a clean profile at 1024 px width
> > (do we have data on the most common px width in firefox?)
> 
> From Jeff's telemetry survey as of June 2015[1], most DevTools users have
> 1280px or larger screen width.  However, there is also a large "other"
> bucket that makes the data hard to reason about.
> 
> [1]:
> https://docs.google.com/spreadsheets/d/1pp4EO1o0wpOr2webx-TmzNhl-yMn75pXc_Nw-
> zmFPaQ/edit#gid=2119694026

Also, it'd be nice to know the width of the Firefox window since people don't always have it maximized
I don't think we want to use screen size as a gating factor, if we have too many tools to fit in the average screen then we have a separate problem to deal with.  We should focus on having the right set of default tools for most users and make sure the other non-default tools are easily discoverable.


(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #9)
> In my mind, (a) makes sense for niche panels that might not be useful to
> most developers (eg the web audio panel), (b) makes the most sense for
> "experimental", "wip", and "v0" features that we would like to eventually
> enable everywhere by default, and (c) is for tools that are broadly useful
> for most developers.

Agreed on this.

> Currently, we are using (a) for the memory panel, but I think it makes sense
> to transition to (c):
> 
> * Memory/GC is not a niche-ish web platform feature; it is a fundamental
> part the browser regardless what kind of web platform APIs you are using.

I think we should use metrics to determine this.  I'm not sure there is an easy way to make a call here, my gut says to try it as a default panel and if it does get non-niche usage we keep it as a default.  You'll need to decide what you think that means, it will get a boost just by being the default.  Should it be seeing similar numbers to the debugger?  What kind of numbers matter here?  Time spent or number of times opened?

I don't think trying it as a default panel for a release or two will create a negative impact on the overall product.

> * Time in GC and memory footprint is often a performance bottleneck or
> source of excess slop.
> * Minimizing the memory footprint is very important for mobile
> games/sites/apps to to avoid getting OOM killed.

This makes sense to me, but I think it also argues that this is more of a niche feature.

> * Both Chrome and IE have their heap snapshots / memory tools enabled by
> default.

Competitive pressure is a decent measure but more importantly are users expecting this solution to be available.  I think its a little hard to test this without trying it.
Flags: needinfo?(clarkbw)
Ok, I will make this bug depend on bug 1221619, determine the details of the telemetry probes we want in that bug, and once it lands proceed with this one.
Depends on: 1221619
The telemetry probes have landed, let's enable this tool by default for a release or so and see how things go!
Keywords: checkin-needed
Nevermind! We are going to wait for shortest paths and a UX review first!
Keywords: checkin-needed
Depends on: 1149385
Depends on: 1247696
We've hurdled all blockers, can we move forward with this?
Flags: needinfo?(jwalker)
Flags: needinfo?(clarkbw)
I would like to double-check 2 things: I think we already have telemetry for this, so we know how much the tool is used, right? And by turning it on, with commonly used screen sizes, and everything else at default settings, we're not making other tabs elide. i.e. We're not making things worse.

But, in principle, Yes, I'm OK with turning it on. Thanks for being patient.
Flags: needinfo?(jwalker)
Yep, lets turn it on.  Thanks!

Most screen resolutions are above 1280 which should give us enough room.
https://mzl.la/29Ocetu
Flags: needinfo?(clarkbw)
Thanks for the replies!

(In reply to Joe Walker [:jwalker] (needinfo me or ping on irc) from comment #22)
> I would like to double-check 2 things: I think we already have telemetry for
> this, so we know how much the tool is used, right?

Yes, we do: https://dxr.mozilla.org/mozilla-central/search?tree=mozilla-central&q=telemetry%20path%3Adevtools%2Fclient%2Fmemory

> And by turning it on,
> with commonly used screen sizes, and everything else at default settings,
> we're not making other tabs elide. i.e. We're not making things worse.

(In reply to Bryan Clark (DevTools PM) [@clarkbw] from comment #23)
> Most screen resolutions are above 1280 which should give us enough room.
> https://mzl.la/29Ocetu

Yep, and we have room @ 1024, as shown by bgrins's screenshots.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/b9d5aa8be125
Enable the devtools' memory panel by default. r=jsantell
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b9d5aa8be125
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
I've managed this issue on this bug in Nightly 46.0a1 (2016-01-20) ; (Build ID: 20160120030239) from Linux.

This Bug is now verified as fixed on Latest Firefox Nightly 50.0a1 (2016-07-25)

Build ID: 20160725030248
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0
OS: Linux 4.4.0-31-generic ; Ubuntu 16.04 (64 Bit)
QA Whiteboard: [bugday-20160727]
I have successfully reproduce this bug on firefox nightly 46.0a1 (2016-01-20)
with windows 7 (32 bit)
Mozilla/5.0 (Windows NT 6.1; rv:46.0) Gecko/20100101 Firefox/46.0

I found this fix on latest nightly 50.0a1 (2016-07-26)

Mozilla/5.0 (Windows NT 6.1; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID : 20160726080520

[bugday-20160727]
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.