711490 - Add telemetry, memory reporter for counting low-memory events

Status: RESOLVED FIXED

Description

[Justin Lebar (not reading bugmail)]

Somehow I didn't think to add telemetry and memory reporters for the low-memory events added in bug 670967.

Comment 1

[Marco Castelluccio [:marco]]

Did you add telemetry reporters for page faults on Linux and Mac?

Comment 2

[Justin Lebar (not reading bugmail)]

Yes, some time ago.

http://hg.mozilla.org/mozilla-central/file/7cc472108eb4/toolkit/components/telemetry/TelemetryPing.js#l65

Comment 3

[Justin Lebar (not reading bugmail)]

Created attachment 582374 [details] [diff] [review]
Patch v1

Comment 4

[Justin Lebar (not reading bugmail)]

I kind of went overboard with these MR descriptions, but I think they're nice.  :)

Comment 5

[Nicholas Nethercote [:njn]]

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!

Comment 6

[Justin Lebar (not reading bugmail)]

Thanks for catching that copy/paste bug!

Comment 7

[Nicholas Nethercote [:njn]]

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

Comment 8

[Justin Lebar (not reading bugmail)]

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.

Comment 9

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/79b2b83c030b

Comment 10

[Ed Morley (Away 27th-2nd) [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/79b2b83c030b

Comment 11

[Scoobidiver (away)]

It missed the Firefox 11 release for a few hours.

Comment 12

[Justin Lebar (not reading bugmail)]

Well, that's a bummer.

Comment 13

[Andrew McCreight [:mccr8]]

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

Comment 14

[Justin Lebar (not reading bugmail)]

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!

Comment 15

[Scoobidiver (away)]

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

Comment 16

[Andrew McCreight [:mccr8]]

Okay, I guess we can cancel the nom then.  Thanks for looking into this.