1168566 - Separate memory consumption from allocations

Status: RESOLVED FIXED

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

Description

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

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.

Comment 1

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

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

Comment 2

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

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

Comment 3

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

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.

Comment 4

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

Attachment: 1168566-allocation-pref.patch

Mostly test changes. Thoughts on the tooltip?

Comment 5

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

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

Comment 6

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

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 7

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

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

Comment 8

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

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

Comment 9

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

Yeah that sounds good to me!

Comment 10

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

Attachment: 1168566-allocation-pref.patch

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ba4ba4458feb

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/fx-team/rev/ba4ba4458feb

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ba4ba4458feb