Add telemetry, memory reporter for counting low-memory events

RESOLVED FIXED in mozilla11

Status

()

Core
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: Justin Lebar (not reading bugmail))

Tracking

unspecified
mozilla11
x86
Windows XP
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink])

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Somehow I didn't think to add telemetry and memory reporters for the low-memory events added in bug 670967.
Did you add telemetry reporters for page faults on Linux and Mac?
(Assignee)

Comment 2

5 years ago
Yes, some time ago.

http://hg.mozilla.org/mozilla-central/file/7cc472108eb4/toolkit/components/telemetry/TelemetryPing.js#l65
Whiteboard: [MemShrink]
(Assignee)

Comment 3

5 years ago
Created attachment 582374 [details] [diff] [review]
Patch v1
Attachment #582374 - Flags: review?(nnethercote)
(Assignee)

Comment 4

5 years ago
I kind of went overboard with these MR descriptions, but I think they're nice.  :)
Comment on attachment 582374 [details] [diff] [review]
Patch v1

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

Looks good.  Lots of comments below;  mostly nits, but there's one bad copy and paste bug.

::: toolkit/components/telemetry/TelemetryPing.js
@@ +63,5 @@
>      "MEMORY_IMAGES_CONTENT_USED_UNCOMPRESSED",
>    "heap-allocated": "MEMORY_HEAP_ALLOCATED",
> +  "page-faults-hard": "PAGE_FAULTS_HARD",
> +  "low-virtual-mem-events": "MEMORY_LOW_VIRTUAL_MEM_EVENTS",
> +  "low-physical-mem-events": "MEMORY_LOW_PHYSICAL_MEM_EVENTS"

Is the "MEMORY_" prefix necessary?  PAGE_FAULTS_HARD doesn't have it.

::: xpcom/base/AvailableMemoryTracker.cpp
@@ +336,5 @@
> +  NS_DECL_ISUPPORTS
> +
> +  NS_IMETHOD GetPath(nsACString &aPath)
> +  {
> +    aPath.AssignLiteral("num-low-virtual-memory-events");

I like it better without the "num-" prefix.  And it's consistent with "page-faults-hard".  Likewise with num-low-physical-memory-events.

@@ +344,5 @@
> +  NS_IMETHOD GetAmount(PRInt64 *aAmount)
> +  {
> +    // This memory reporter shouldn't be installed on 64-bit machines, since we
> +    // force-disable virtual-memory tracking there.
> +    MOZ_ASSERT(sizeof(void*) <= 4);

Be bold!  Change it to |sizeof(void*) == 4|!

@@ +363,5 @@
> +        sHooksInstalled ? "" : " and restarting"));
> +    }
> +    else {
> +      aDescription.Append(nsPrintfCString(1024,
> +        "We fire such an event if we notice there is less than %dmb of virtual "

Make it "%d MB" to match the rest of about:memory, please.

@@ +397,5 @@
> +  {
> +    aDescription.AssignLiteral(
> +      "Number of low-physical-memory events fired since startup. ");
> +
> +    if (sLowVirtualMemoryThreshold == 0 || !sHooksInstalled) {

Copy and paste error?  I think this should be |sLowPhysicalMemoryThreshold|.  When you fix that one, it might be worth double-checking all the other uses :)

@@ +405,5 @@
> +        sHooksInstalled ? "" : " and restarting"));
> +    }
> +    else {
> +      aDescription.Append(nsPrintfCString(1024,
> +        "We fire such an event if we notice there is less than %dmb of "

"%d MB" again.

@@ +409,5 @@
> +        "We fire such an event if we notice there is less than %dmb of "
> +        "available physical memory (controlled by the "
> +        "'memory.low_physical_mem_threshold_mb' pref).  The machine will start "
> +        "to page if it runs out of physical memory; this is bad, but it's not "
> +        "the end of the world.",

Explain briefly why it's bad?  E.g. "this is bad because it can degrade performance greatly...".

@@ +466,5 @@
> +    sHooksInstalled = false;
> +  }
> +
> +  NS_RegisterMemoryReporter(new NumLowPhysicalMemoryEventsMemoryReporter());
> +  if (sizeof(void*) <= 4) {

You can be bold here too!
Attachment #582374 - Flags: review?(nnethercote) → review+
(Assignee)

Comment 6

5 years ago
Thanks for catching that copy/paste bug!
(Assignee)

Updated

5 years ago
Assignee: nobody → justin.lebar+bug
> > +    aPath.AssignLiteral("num-low-virtual-memory-events");
> 
> I like it better without the "num-" prefix.  And it's consistent with
> "page-faults-hard".  Likewise with num-low-physical-memory-events.

And "low-memory-events-{physical,virtual}" would be more consistent with other entries, by putting the distinguishing part at the end.  The telemetry names would also have to change to match.
(Assignee)

Comment 8

5 years ago
I'm not wild about LOW_MEMORY_EVENTS_{VIRTUAL,PHYSICAL} as telemetry names because we don't have descriptions in telemetry, but okay.  Maybe the front-end will be fixed eventually.
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/79b2b83c030b
https://hg.mozilla.org/mozilla-central/rev/79b2b83c030b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11

Comment 11

5 years ago
It missed the Firefox 11 release for a few hours.
Target Milestone: mozilla11 → mozilla12
(Assignee)

Comment 12

5 years ago
Well, that's a bummer.
You could nominate it for aurora once it has baked for a few days on m-c.  At the platform meeting they said they'd be a little more lenient about allowing additional low risk telemetry patches.

In case it isn't clear to those reading, this will measure a new kind of low memory pressure trigger added in Firefox 11 (bug 670967).
(Assignee)

Comment 14

5 years ago
Comment on attachment 582374 [details] [diff] [review]
Patch v1

Nominating now, rather than in a few days, because I'll be on vacation then.

If this gets a+'ed before Jan 3, I'd certainly appreciate someone pushing this to Aurora for me!
Attachment #582374 - Flags: approval-mozilla-aurora?

Comment 15

5 years ago
I was wrong about the Firefox 11 target because this changeset is between:
* The latest changeset in Nightly 11.0a1/20111220: http://hg.mozilla.org/mozilla-central/rev/2afd7ae68e8b.
* The first changeset in 11.0a2/20111220: http://hg.mozilla.org/mozilla-central/rev/a8506ab2c654
Target Milestone: mozilla12 → mozilla11
Okay, I guess we can cancel the nom then.  Thanks for looking into this.
Attachment #582374 - Flags: approval-mozilla-aurora?
You need to log in before you can comment on or make changes to this bug.