Comment 1 Marco Castelluccio [:marco] 2011-12-16 10:21:56 PST

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

Comment 2 Justin Lebar (not reading bugmail) 2011-12-16 10:28:17 PST

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) 2011-12-16 13:29:09 PST

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

Comment 4 Justin Lebar (not reading bugmail) 2011-12-16 13:30:51 PST

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

Comment 5 Nicholas Nethercote [:njn] 2011-12-16 19:16:11 PST

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) 2011-12-16 19:18:16 PST

Thanks for catching that copy/paste bug!

Comment 7 Nicholas Nethercote [:njn] 2011-12-17 18:03:43 PST

> > +    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) 2011-12-19 08:09:49 PST

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) 2011-12-19 09:27:51 PST

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

Comment 10 Ed Morley [:emorley] 2011-12-20 06:09:36 PST

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

Comment 11 Scoobidiver (away) 2011-12-21 05:21:00 PST

It missed the Firefox 11 release for a few hours.

Comment 12 Justin Lebar (not reading bugmail) 2011-12-21 08:08:21 PST

Well, that's a bummer.

Comment 13 Andrew McCreight [:mccr8] 2011-12-21 08:14:52 PST

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) 2011-12-21 08:21:21 PST

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) 2011-12-21 09:37:22 PST

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] 2011-12-21 09:43:07 PST

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