Last Comment Bug 711490 - Add telemetry, memory reporter for counting low-memory events
: Add telemetry, memory reporter for counting low-memory events
Status: RESOLVED FIXED
[MemShrink]
:
Product: Core
Classification: Components
Component: General (show other bugs)
: unspecified
: x86 Windows XP
: -- normal (vote)
: mozilla11
Assigned To: Justin Lebar (not reading bugmail)
:
Mentors:
Depends on: 670967
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-16 08:37 PST by Justin Lebar (not reading bugmail)
Modified: 2011-12-21 09:45 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (11.73 KB, patch)
2011-12-16 13:29 PST, Justin Lebar (not reading bugmail)
n.nethercote: review+
Details | Diff | Review

Description Justin Lebar (not reading bugmail) 2011-12-16 08:37:44 PST
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] 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.

Note You need to log in before you can comment on or make changes to this bug.