[e10s] Add measure(s) for overall (browser + content) memory usage

RESOLVED FIXED in Firefox 44, Firefox OS v2.5

Status

()

Toolkit
Telemetry
RESOLVED FIXED
2 years ago
5 months ago

People

(Reporter: jimm, Assigned: chutten)

Tracking

(Blocks: 1 bug)

Trunk
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox43 affected, firefox44 fixed, firefox45 fixed, firefox46 fixed, b2g-v2.5 fixed)

Details

Attachments

(2 attachments, 7 obsolete attachments)

(Reporter)

Description

2 years ago
Part of the e10s release criteria work.
(Reporter)

Updated

2 years ago
tracking-e10s: --- → +
Blocks: 1222849
gatherMemory() in TelemetrySession.jsm already collect quite a few measurements:

MEMORY_VSIZE - vsize
MEMORY_VSIZE_MAX_CONTIGUOUS - vsizeMaxContiguous
MEMORY_RESIDENT - residentFast
MEMORY_HEAP_ALLOCATED - heapAllocated
MEMORY_HEAP_COMMITTED_UNUSED_RATIO - heapOverheadRatio
MEMORY_JS_GC_HEAP - JSMainRuntimeGCHeap
MEMORY_JS_MAIN_RUNTIME_TEMPORARY_PEAK - JSMainRuntimeTemporaryPeak
MEMORY_JS_COMPARTMENTS_SYSTEM - JSMainRuntimeCompartmentsSystem
MEMORY_JS_COMPARTMENTS_USER - JSMainRuntimeCompartmentsUser
MEMORY_IMAGES_CONTENT_USED_UNCOMPRESSED - imagesContentUsedUncompressed
MEMORY_STORAGE_SQLITE - storageSQLite
MEMORY_EVENTS_VIRTUAL - lowMemoryEventsVirtual
MEMORY_EVENTS_PHYSICAL - lowMemoryEventsPhysical
GHOST_WINDOWS - ghostWindows
PAGE_FAULTS_HARD - pageFaultsHard

Roberto, can you include these in your A/B comparisons?

Note that the units in these histograms aren't all the same. See https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetrySession.jsm#1043
Flags: needinfo?(rvitillo)
On Thu, Nov 12, 2015 at 4:16 PM, Eric Rahm <erahm@mozilla.com> wrote:
> The main issue is trying to get a useful view of the overall memory 
> used by the main process and content processes. The naive implementation 
> of summing the RSS for all processes is going to make things look much 
> worse than they are. For the measurements I've been doing w/AWSY + e10s 
> I settled on:
> overall_memory = main_rss + sum(content_processes_uss)
>
> So at the very least you'll want to add 'residentUnique' to the probe. 
> Currently this only works on linux (I think), I'm about to land a patch 
> for OS X as well. If necessary we can prioritize adding it to Windows as
> well.

I'm not familiar with the telemetry probe, but you'd need to make sure it's reporting from all processes, not just the main process.
Flags: needinfo?(rvitillo)
(Assignee)

Updated

2 years ago
Assignee: nobody → chutten
Depends on: 1224685
Depends on: 1223927
(Assignee)

Comment 3

2 years ago
(In reply to Vladan Djeric (:vladan) -- please needinfo! PTO Nov 16/19/20 from comment #2)
> On Thu, Nov 12, 2015 at 4:16 PM, Eric Rahm <erahm@mozilla.com> wrote:
> > The main issue is trying to get a useful view of the overall memory 
> > used by the main process and content processes. The naive implementation 
> > of summing the RSS for all processes is going to make things look much 
> > worse than they are. For the measurements I've been doing w/AWSY + e10s 
> > I settled on:
> > overall_memory = main_rss + sum(content_processes_uss)
> >
> > So at the very least you'll want to add 'residentUnique' to the probe. 
> > Currently this only works on linux (I think), I'm about to land a patch 
> > for OS X as well. If necessary we can prioritize adding it to Windows as
> > well.
> 
> I'm not familiar with the telemetry probe, but you'd need to make sure it's
> reporting from all processes, not just the main process.

I think he was speaking in generalities. People generally calculate total memory footprint as sum_over_processes(resident set size (RSS)), which is incorrect as it includes memory allocated by shared libraries whose memory budgets are, well, shared.

Often far more useful is the Unique Set Size (USS) which is mostly-accurately described as the amount of memory the system reclaims when killing a process. It's a lower bound and a functionality-based definition, which makes it appealing to me.

So, adding a histogram (say, MEMORY_UNIQUE to go along with MEMORY_RESIDENT?) for the residentUnique value is the way to enable us to measure this accurately. It's a distinguished value, so it's already an attribute in the idl for when it was implemented for Firefox OS in June 2014. :erahm has just added it to OSX and seems to have a patch one step away from landing for Windows in bug 1224685.

All I need is confirmation that TelemetrySession's gatherMemory is called on all processes, and I can make the eight-line patch to add the new histogram.
Flags: needinfo?(vladan.bugzilla)
Yes, I agree with everything you wrote (and with Eric's approach). The line "I'm not familiar with the telemetry probe, but you'd need to make sure it's reporting from all processes, not just the main process" in comment 2 was actually written by Eric in his email, I just messed up quoting it in Bugzilla.

gatherMemory() gets called on process startup (both chrome & content) and during the "cycle-collector-begin" event (both chrome & content).

You can see the differences in chrome-process & content-process Telemetry initialization in the setupContentProcess() and setupChromeProcess() methods of TelemetrySession.jsm. You could also just check for memory histograms in the child payloads of about:telemetry now that you've added that functionality ;)
Flags: needinfo?(vladan.bugzilla)
(Assignee)

Comment 5

2 years ago
Created attachment 8689091 [details] [diff] [review]
0001-bug-1198209-Add-a-histogram-for-USS-r-vladan.patch

And here's the trypush: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd613260c9ee
Attachment #8689091 - Flags: review?(vladan.bugzilla)
Comment on attachment 8689091 [details] [diff] [review]
0001-bug-1198209-Add-a-histogram-for-USS-r-vladan.patch

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

::: toolkit/components/telemetry/Histograms.json
@@ +473,5 @@
> +    "expires_in_version": "never",
> +    "kind": "exponential",
> +    "low": "32 * 1024",
> +    "high": "16 * 1024 * 1024",
> +    "n_buckets": 200,

add a bug_numbers field

@@ +474,5 @@
> +    "kind": "exponential",
> +    "low": "32 * 1024",
> +    "high": "16 * 1024 * 1024",
> +    "n_buckets": 200,
> +    "extended_statistics_ok": true,

we don't need extended stats

::: toolkit/components/telemetry/bucket-whitelist.json
@@ +1,3 @@
>  [
>    "MEMORY_RESIDENT",
> +  "MEMORY_UNIQUE",

do we really need 200 buckets for these memory histograms? i'd rather not add anything new to this whitelist..

If we don't need 200 buckets for these memory histograms, let's convert the other memory histograms to be 100 buckets. You'll need to change their names too.
Attachment #8689091 - Flags: review?(vladan.bugzilla)
(Assignee)

Comment 7

2 years ago
Created attachment 8689537 [details] [diff] [review]
0001-bug-1198209-Add-a-histogram-for-USS-r-vladan.patch

Looking at the shapes of data that MEMORY_RESIDENT has provided in the past, I don't see the need for the high fidelity for either measure. I think we can cut it in half without worry. I've filed bug 1226196 to track this effort.

Meanwhile, here's an unwhitelisted version of the previous patch with corrections.
Attachment #8689091 - Attachment is obsolete: true
Attachment #8689537 - Flags: review?(dteller)
Comment on attachment 8689537 [details] [diff] [review]
0001-bug-1198209-Add-a-histogram-for-USS-r-vladan.patch

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

The patch looks good to me, but I suspect you wanted r?vladan, right?

Also, I don't see the corrections requested in comment 6.
Attachment #8689537 - Flags: review?(dteller) → feedback+
(Assignee)

Comment 9

2 years ago
Created attachment 8689574 [details] [diff] [review]
0001-bug-1198209-Add-a-histogram-for-USS.patch

Vladan's on PTO, so I thought you'd be willing to take a look.

...and apparently I reuploaded the old version instead of uploading the new version (which addressed the comments). Could I trouble you a second time?
Attachment #8689537 - Attachment is obsolete: true
Attachment #8689574 - Flags: review?(dteller)
Comment on attachment 8689574 [details] [diff] [review]
0001-bug-1198209-Add-a-histogram-for-USS.patch

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

Looks good to me.
In the future, you should really try MozReview :)
Attachment #8689574 - Flags: review?(dteller) → review+
(Assignee)

Comment 11

2 years ago
Created attachment 8689580 [details] [diff] [review]
0001-bug-1198209-Add-a-histogram-for-USS.patch
Attachment #8689574 - Attachment is obsolete: true
Attachment #8689580 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 12

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/c1bac7e467ed
Keywords: checkin-needed

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c1bac7e467ed
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Chris: can you look into getting this patch and its dependencies uplifted?
Status: RESOLVED → REOPENED
Flags: needinfo?(chutten)
Resolution: FIXED → ---
Blocks: 1229104
(Assignee)

Comment 15

2 years ago
Comment on attachment 8689580 [details] [diff] [review]
0001-bug-1198209-Add-a-histogram-for-USS.patch

Approval Request Comment
[Feature/regressing bug #]: Unique Set Size telemetry histogram
[User impact if declined]: Won't be able to measure memory on e10s Beta 44 experiment
[Describe test coverage new/current, TreeHerder]: Baked on central for a week and a half
[Risks and why]: None, this is using an existing API. Should be uplifted after bug 1219733 or else it won't build due to the presence of the bug_numbers field
[String/UUID change made/needed]: None
Flags: needinfo?(chutten)
Attachment #8689580 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 16

2 years ago
Uplifts on their way.

Having the USS histograms is useful, but we have no way to cross-reference and get this bug's desired "total memory" measurement. So I'd better start work on that, then.
(Assignee)

Comment 17

2 years ago
Created attachment 8694901 [details] [diff] [review]
0001-bug-1198209-Add-a-Total-Memory-Histogram-r-vladan.patch
Attachment #8694901 - Flags: review?(vladan.bugzilla)
Comment on attachment 8694901 [details] [diff] [review]
0001-bug-1198209-Add-a-Total-Memory-Histogram-r-vladan.patch

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

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +96,5 @@
>  
>  const TOPIC_CYCLE_COLLECTOR_BEGIN = "cycle-collector-begin";
>  
> +// How long to wait in millis for all the child memory reports to come in
> +const TOTAL_MEMORY_COLLECTOR_TIMEOUT = 40;

is 40ms enough time? the TELEMETRY_MEMORY_REPORTER_MS probe seems to indicate that it usually takes longer to run gatherMemory() http://mzl.la/1NrJNZD

@@ +106,1 @@
>  var gLastMemoryPoll = null;

Can we move all these into the Impl object?

@@ +1066,5 @@
>      cc("PAGE_FAULTS_HARD", "pageFaultsHard");
>  
> +    if (!gTotalMemoryTimeout) {
> +      try {
> +        gTotalMemory = mgr.residentFast; // total = parent RSS + sum(child USS)

nit: put this comment above the line

@@ +1068,5 @@
> +    if (!gTotalMemoryTimeout) {
> +      try {
> +        gTotalMemory = mgr.residentFast; // total = parent RSS + sum(child USS)
> +        if (ppmm.childCount > 1) {
> +          gChildrenToHearFrom = ppmm.childCount - 1;

Why is childCount == 1 in single-process Firefox?

@@ +1073,5 @@
> +          // Do not report If we time out waiting for the children to call
> +          gTotalMemoryTimeout = setTimeout(
> +            () => gTotalMemoryTimeout = undefined,
> +            TOTAL_MEMORY_COLLECTOR_TIMEOUT);
> +          ppmm.broadcastAsyncMessage(MESSAGE_TELEMETRY_GET_CHILD_USS, {});

doesn't gatherMemory() get called in the content-process too? should content processes be broadcasting these GET_CHILD_USS messages?

gatherMemory() is called during Telemetry service initialization in the chrome & child processes, it's called whenever we transmit a telemetry session ping, during cycle collection, and when loading about:telemetry. You should make sure the new code behaves properly in each of those circumstances

@@ +1495,5 @@
> +    case MESSAGE_TELEMETRY_USS:
> +    {
> +      if (gTotalMemoryTimeout) {
> +        gTotalMemory += message.data;
> +        if (--gChildrenToHearFrom == 0) {

It's unlikely, but new processes could get created while memory info is being gathered, so gChildrentToHearFrom could end up negative.
Attachment #8694901 - Flags: review?(vladan.bugzilla)
(Assignee)

Comment 19

2 years ago
(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #18)
> Comment on attachment 8694901 [details] [diff] [review]
> 0001-bug-1198209-Add-a-Total-Memory-Histogram-r-vladan.patch
> 
> Review of attachment 8694901 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/TelemetrySession.jsm
> @@ +96,5 @@
> >  
> >  const TOPIC_CYCLE_COLLECTOR_BEGIN = "cycle-collector-begin";
> >  
> > +// How long to wait in millis for all the child memory reports to come in
> > +const TOTAL_MEMORY_COLLECTOR_TIMEOUT = 40;
> 
> is 40ms enough time? the TELEMETRY_MEMORY_REPORTER_MS probe seems to
> indicate that it usually takes longer to run gatherMemory()
> http://mzl.la/1NrJNZD

40ms is for the IPC out, IPC back and a residentUnique call in between for each process. Not a full gatherMemory call. Unless the main threads are all under contention, it's probably loads of time. But I'll up it to... 200? I expect it to be hit only infrequently, so I don't mind a "long"-running timer in this case.
 
> @@ +106,1 @@
> >  var gLastMemoryPoll = null;
> 
> Can we move all these into the Impl object?

Ah, so that's where the magic happens. Saw other uses of gWhatever and went with the flow.

> @@ +1066,5 @@
> >      cc("PAGE_FAULTS_HARD", "pageFaultsHard");
> >  
> > +    if (!gTotalMemoryTimeout) {
> > +      try {
> > +        gTotalMemory = mgr.residentFast; // total = parent RSS + sum(child USS)
> 
> nit: put this comment above the line

Will do

> @@ +1068,5 @@
> > +    if (!gTotalMemoryTimeout) {
> > +      try {
> > +        gTotalMemory = mgr.residentFast; // total = parent RSS + sum(child USS)
> > +        if (ppmm.childCount > 1) {
> > +          gChildrenToHearFrom = ppmm.childCount - 1;
> 
> Why is childCount == 1 in single-process Firefox?

I have no idea. My guess is that it's more of a processCount than a childCount. E10s-disabled it is 1, E10s-enabled with only about:telemetry showing it is 1, E10s-enabled with about:telemetry and google.com loaded it is 2.

> @@ +1073,5 @@
> > +          // Do not report If we time out waiting for the children to call
> > +          gTotalMemoryTimeout = setTimeout(
> > +            () => gTotalMemoryTimeout = undefined,
> > +            TOTAL_MEMORY_COLLECTOR_TIMEOUT);
> > +          ppmm.broadcastAsyncMessage(MESSAGE_TELEMETRY_GET_CHILD_USS, {});
> 
> doesn't gatherMemory() get called in the content-process too? should content
> processes be broadcasting these GET_CHILD_USS messages?

Why yes, yes they do. Could've sworn I put a !Utils.isContentProcess around this...
 
> gatherMemory() is called during Telemetry service initialization in the
> chrome & child processes, it's called whenever we transmit a telemetry
> session ping, during cycle collection, and when loading about:telemetry. You
> should make sure the new code behaves properly in each of those circumstances

Well, loading about:telemetry is easy to test. I might need some assistance triggering those others.

> @@ +1495,5 @@
> > +    case MESSAGE_TELEMETRY_USS:
> > +    {
> > +      if (gTotalMemoryTimeout) {
> > +        gTotalMemory += message.data;
> > +        if (--gChildrenToHearFrom == 0) {
> 
> It's unlikely, but new processes could get created while memory info is
> being gathered, so gChildrentToHearFrom could end up negative.

This would be the same case as if one child were to lag significantly and report during the next timeout period. In either case, the information would be wrong. (though probably not by much) The important thing is that it only report once.

I could build on top of sendAsyncMessage a framework for reliable broadcast and response. (I'm thinking we could send an id with the request that just needs to be echoed back in the response. Keep a dict of outstanding ids.). I wasn't sure we wanted to go to the trouble. I also was a little surprised this facility wasn't provided by the message managers already.
(Assignee)

Comment 20

2 years ago
Created attachment 8695886 [details] [diff] [review]
0001-bug-1198209-Add-a-Total-Memory-Histogram-r-vladan.patch

Nits addressed. Ended up writing a reliable call and response so we're definitely getting a snapshot of the processes we think we're snapshotting.

Tested about:telemetry reload trigger, session end trigger, child process create, child process destroy.
Attachment #8694901 - Attachment is obsolete: true
Attachment #8695886 - Flags: review?(vladan.bugzilla)
Vladan, Chris, should we uplift both the patches here (when ready) to FF44 together? I noticed that the first patch landed and then the bug was re-opened and at the moment, that's the only patch nom'd for uplift to Aurora. Just want to make sure, we uplift the "entire fix" to 44 and not a partial fix. Please let me know.
Flags: needinfo?(vladan.bugzilla)
Flags: needinfo?(chutten)
(Assignee)

Comment 22

2 years ago
Yes, please, Ritu. As you've noticed, the second one's pending review, but it adds the histogram that really makes this measure useful.
Flags: needinfo?(chutten)
Comment on attachment 8695886 [details] [diff] [review]
0001-bug-1198209-Add-a-Total-Memory-Histogram-r-vladan.patch

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

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +1091,5 @@
> +            "MEMORY_TOTAL",
> +            Ci.nsIMemoryReporter.UNITS_BYTES,
> +            this._totalMemory);
> +        }
> +      } catch (e) {

these scare me, log something here

@@ +1504,5 @@
>        break;
>      }
> +    case MESSAGE_TELEMETRY_USS:
> +    {
> +      if (this._totalMemoryTimeout && this._childrenToHearFrom.delete(message.data.id)) {

this is going to be very rare, but you could end up in a situation where a child doesn't respond to a request until a second request comes in, so it ends up sending two responses. That could cause the first response to be used in the second sum.

I think it ought to be harmless since the memory use would be gathered around the same time for both responses. You could use monotonically increasing child IDs to make sure the parent is never using outdated responses.
Attachment #8695886 - Flags: review?(vladan.bugzilla)
(In reply to Ritu Kothari (:ritu) from comment #21)
> Vladan, Chris, should we uplift both the patches here (when ready) to FF44
> together? I noticed that the first patch landed and then the bug was
> re-opened and at the moment, that's the only patch nom'd for uplift to
> Aurora. Just want to make sure, we uplift the "entire fix" to 44 and not a
> partial fix. Please let me know.

Yes, we'll need both patches on 44
Flags: needinfo?(vladan.bugzilla)
Comment on attachment 8689580 [details] [diff] [review]
0001-bug-1198209-Add-a-histogram-for-USS.patch

Please re-nominate both once they are ready.
Attachment #8689580 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
(Assignee)

Comment 26

2 years ago
Created attachment 8698517 [details] [diff] [review]
0001-bug-1198209-Add-a-Total-Memory-Histogram-r-vladan.patch

Removed the try/catch as it wasn't needed. This is only going into builds where residentFast is and has been present for ages.
Attachment #8695886 - Attachment is obsolete: true
Attachment #8698517 - Flags: review?(vladan.bugzilla)
Comment on attachment 8698517 [details] [diff] [review]
0001-bug-1198209-Add-a-Total-Memory-Histogram-r-vladan.patch

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

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +1070,4 @@
>      c("GHOST_WINDOWS", "ghostWindows");
>      cc("PAGE_FAULTS_HARD", "pageFaultsHard");
>  
> +    if (!Utils.isContentProcess && !this._totalMemoryTimeout) {

what happens if gatherMemory gets called for a second time with child replies outstanding from the first request? do we just complete one request? does that work out?

@@ +1077,5 @@
> +      if (ppmm.childCount > 1) {
> +        // Do not report If we time out waiting for the children to call
> +        this._totalMemoryTimeout = setTimeout(
> +          () => {
> +            this._totalMemoryTimeout = undefined;

this is initialized to null but you're resetting it to undefined

@@ +1508,5 @@
> +      if (this._totalMemoryTimeout && this._childrenToHearFrom.delete(message.data.id)) {
> +        this._totalMemory += message.data.bytes;
> +        if (this._childrenToHearFrom.size == 0) {
> +          clearTimeout(this._totalMemoryTimeout);
> +          this._totalMemoryTimeout = undefined;

ditto on null vs undefined
Attachment #8698517 - Flags: review?(vladan.bugzilla) → review+
(Assignee)

Comment 28

2 years ago
(In reply to Vladan Djeric (:vladan) -- please needinfo | PTO Dec 14-18. Email or IRC if urgent from comment #27)
> Comment on attachment 8698517 [details] [diff] [review]
> 0001-bug-1198209-Add-a-Total-Memory-Histogram-r-vladan.patch
> 
> Review of attachment 8698517 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/TelemetrySession.jsm
> @@ +1070,4 @@
> >      c("GHOST_WINDOWS", "ghostWindows");
> >      cc("PAGE_FAULTS_HARD", "pageFaultsHard");
> >  
> > +    if (!Utils.isContentProcess && !this._totalMemoryTimeout) {
> 
> what happens if gatherMemory gets called for a second time with child
> replies outstanding from the first request? do we just complete one request?
> does that work out?

Precisely that. The outstanding first calls come back dutifully, but we ignore them as we clear _childrenToHearFrom before sending out the second batch of calls. So we only collect and report the full cohort from the second call.

> @@ +1077,5 @@
> > +      if (ppmm.childCount > 1) {
> > +        // Do not report If we time out waiting for the children to call
> > +        this._totalMemoryTimeout = setTimeout(
> > +          () => {
> > +            this._totalMemoryTimeout = undefined;
> 
> this is initialized to null but you're resetting it to undefined
>
> @@ +1508,5 @@
> > +      if (this._totalMemoryTimeout && this._childrenToHearFrom.delete(message.data.id)) {
> > +        this._totalMemory += message.data.bytes;
> > +        if (this._childrenToHearFrom.size == 0) {
> > +          clearTimeout(this._totalMemoryTimeout);
> > +          this._totalMemoryTimeout = undefined;
> 
> ditto on null vs undefined

I'll initialize to undefined.
(Assignee)

Comment 29

2 years ago
Created attachment 8700026 [details] [diff] [review]
0001-bug-1198209-Add-a-Total-Memory-Histogram-r-vladan.patch
Attachment #8698517 - Attachment is obsolete: true
Attachment #8700026 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 30

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/d19ad724c64c
Keywords: checkin-needed
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=6276261&repo=fx-team
Flags: needinfo?(chutten)

Comment 32

2 years ago
Backout:
https://hg.mozilla.org/integration/fx-team/rev/12019f25b951
(Assignee)

Comment 33

2 years ago
Created attachment 8700771 [details] [diff] [review]
0001-bug-1198209-Add-a-Total-Memory-Histogram-r-vladan.patch

Forgot some `this.`

Passed it through try to ensure I didn't miss anything else: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6741998a66e7
Attachment #8700026 - Attachment is obsolete: true
Flags: needinfo?(chutten)
Attachment #8700771 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 34

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/034c6fa9ec86
Keywords: checkin-needed

Comment 35

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/034c6fa9ec86
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
(Assignee)

Comment 36

2 years ago
Comment on attachment 8689580 [details] [diff] [review]
0001-bug-1198209-Add-a-histogram-for-USS.patch

Approval Request Comment
[Feature/regressing bug #]: Unique Set Size telemetry histogram
[User impact if declined]: Won't be able to measure memory on e10s Beta 44 experiment
[Describe test coverage new/current, TreeHerder]: Baked on central for a week and a half
[Risks and why]: None, this is just using an existing API. Has been in 45 for weeks. Should be uplifted after bug 1219733 or else it won't build due to the presence of the bug_numbers field.
[String/UUID change made/needed]: None
Attachment #8689580 - Flags: approval-mozilla-aurora- → approval-mozilla-beta?
(Assignee)

Comment 37

2 years ago
Comment on attachment 8700771 [details] [diff] [review]
0001-bug-1198209-Add-a-Total-Memory-Histogram-r-vladan.patch

Approval Request Comment
[Feature/regressing bug #]: Total Memory measurements
[User impact if declined]: We won't be able to measure differences in total memory used by different configurations of Firefox. This is especially needful in the ongoing Beta E10s Experiment.
[Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6741998a66e7
[Risks and why]: Little-to-None. The chrome js is talking to the content js instances and requesting that they collate information that they already have. No new APIs added or trails blazed.
[String/UUID change made/needed]: None
Attachment #8700771 - Flags: approval-mozilla-beta?
Attachment #8700771 - Flags: approval-mozilla-aurora?
Comment on attachment 8700771 [details] [diff] [review]
0001-bug-1198209-Add-a-Total-Memory-Histogram-r-vladan.patch

It seems like this is needed for Beta44. Aurora45+ as it measures memory footprint in e10s mode.
Attachment #8700771 - Flags: approval-mozilla-beta?
Attachment #8700771 - Flags: approval-mozilla-beta+
Attachment #8700771 - Flags: approval-mozilla-aurora?
Attachment #8700771 - Flags: approval-mozilla-aurora+

Updated

2 years ago
status-firefox44: --- → affected
Comment on attachment 8689580 [details] [diff] [review]
0001-bug-1198209-Add-a-histogram-for-USS.patch

Beta44+
Attachment #8689580 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-aurora/rev/e86d52914d4d

Comment 41

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/7b36fa08941f
https://hg.mozilla.org/releases/mozilla-beta/rev/2a04abb3101e
status-firefox44: affected → fixed

Comment 42

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/7b36fa08941f
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/2a04abb3101e
status-b2g-v2.5: --- → fixed
(Reporter)

Updated

2 years ago
Duplicate of this bug: 1062006
Depends on: 1340134
You need to log in before you can comment on or make changes to this bug.