Currently, the memory consumption graph we have uses sizeOfTab, which as a target's memory increases, so does the cost to measure it, making the overhead of recording memory over time way too large for a performance tool. Allocation sampling has had some crashing issues, but may be more stable now -- we should see if this is the case, and there's not significant overhead for enabling this, and then separate these two features and have a "Record Allocations" and "Record Memory" options, with the allocations not being behind an "experimental" flag if things are solid.

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #0) > Allocation sampling has had some crashing issues, but may be more stable now Unless you've been hiding bugs from me, shu and I fixed all the known crashes ;) > -- we should see if this is the case, and there's not significant overhead > for enabling this, and then separate these two features and have a "Record > Allocations" and "Record Memory" options, with the allocations not being > behind an "experimental" flag if things are solid. +1 Personally would s/Record Memory/Record Heap Size/

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #1) > (In reply to Jordan Santell [:jsantell] [@jsantell] from comment #0) > > Allocation sampling has had some crashing issues, but may be more stable now > > Unless you've been hiding bugs from me, shu and I fixed all the known > crashes ;) Woo! No, I just haven't been following memory things for the last few weeks due to all the other issues > > > -- we should see if this is the case, and there's not significant overhead > > for enabling this, and then separate these two features and have a "Record > > Allocations" and "Record Memory" options, with the allocations not being > > behind an "experimental" flag if things are solid. > > +1 > > Personally would s/Record Memory/Record Heap Size/ sgtm -- we just have a lot of work done on memory things (as you obviously know), so it makes sense to get in what we can, rather than wait for it all to be solid (because I'm not even sure what a solution to sizeOfTab's slowness is)

Bug 1068988 adds size of allocation in our allocations data, which should be a requirement for supporting "allocation views", meaning we have enough stability, information, and time, to make these views usable, rather than backwards compat for dubious support in Gecko 36-40, when we knew much less about this tool.

Mostly test changes. Thoughts on the tooltip?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0d97adbd7fd

Comment on attachment 8616313 [details] [diff] [review] 1168566-allocation-pref.patch Review of attachment 8616313 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/locales/en-US/chrome/browser/devtools/profiler.dtd @@ +133,5 @@ > +<!-- LOCALIZATION NOTE (profilerUI.enableAllocations): This string > + - is displayed next to a checkbox determining whether or not allocation > + - measurements are enabled. --> > +<!ENTITY profilerUI.enableAllocations "Record Allocations"> > +<!ENTITY profilerUI.enableAllocations.tooltiptext "Record allocation locations and information while profiling."> How about "Record Object allocations while profiling." ? I think we shouldn't imply that we are recording all types of allocations, when we are only recording Object allocations (for now, at least). Not sure if it is important to mention that we aren't recording every Object allocation, but are doing statistical sampling.

Comment on attachment 8616313 [details] [diff] [review] 1168566-allocation-pref.patch Review of attachment 8616313 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/locales/en-US/chrome/browser/devtools/profiler.dtd @@ +133,5 @@ > +<!-- LOCALIZATION NOTE (profilerUI.enableAllocations): This string > + - is displayed next to a checkbox determining whether or not allocation > + - measurements are enabled. --> > +<!ENTITY profilerUI.enableAllocations "Record Allocations"> > +<!ENTITY profilerUI.enableAllocations.tooltiptext "Record allocation locations and information while profiling."> Recording types of allocations is determined by the host, so I don't think this should be terribly accurate in describing what is being recorded. We also don't mention the nature of recording information (statistical sampling) in any other view that uses this (so 4 of the 5 main views), so seems like something that should live in documentation land

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #7) > Comment on attachment 8616313 [details] [diff] [review] > 1168566-allocation-pref.patch > > Review of attachment 8616313 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/locales/en-US/chrome/browser/devtools/profiler.dtd > @@ +133,5 @@ > > +<!-- LOCALIZATION NOTE (profilerUI.enableAllocations): This string > > + - is displayed next to a checkbox determining whether or not allocation > > + - measurements are enabled. --> > > +<!ENTITY profilerUI.enableAllocations "Record Allocations"> > > +<!ENTITY profilerUI.enableAllocations.tooltiptext "Record allocation locations and information while profiling."> > > Recording types of allocations is determined by the host, so I don't think > this should be terribly accurate in describing what is being recorded. By "host" you mean the server? Yes that is true, but the only servers we talk to will only record object allocations. We can easily update it in the future when that changes.

Yeah that sounds good to me!