Closed
Bug 711490
Opened 11 years ago
Closed 11 years ago
Add telemetry, memory reporter for counting low-memory events
Categories
(Core :: General, defect)
Tracking
()
RESOLVED
FIXED
mozilla11
People
(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)
References
Details
(Whiteboard: [MemShrink])
Attachments
(1 file)
11.73 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
Somehow I didn't think to add telemetry and memory reporters for the low-memory events added in bug 670967.
Comment 1•11 years ago
|
||
Did you add telemetry reporters for page faults on Linux and Mac?
Assignee | ||
Comment 2•11 years ago
|
||
Yes, some time ago. http://hg.mozilla.org/mozilla-central/file/7cc472108eb4/toolkit/components/telemetry/TelemetryPing.js#l65
![]() |
||
Updated•11 years ago
|
Whiteboard: [MemShrink]
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #582374 -
Flags: review?(nnethercote)
Assignee | ||
Comment 4•11 years ago
|
||
I kind of went overboard with these MR descriptions, but I think they're nice. :)
![]() |
||
Comment 5•11 years ago
|
||
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•11 years ago
|
||
Thanks for catching that copy/paste bug!
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → justin.lebar+bug
![]() |
||
Comment 7•11 years ago
|
||
> > + 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•11 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•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/79b2b83c030b
Comment 10•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/79b2b83c030b
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Comment 11•11 years ago
|
||
It missed the Firefox 11 release for a few hours.
Target Milestone: mozilla11 → mozilla12
Assignee | ||
Comment 12•11 years ago
|
||
Well, that's a bummer.
Comment 13•11 years ago
|
||
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•11 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•11 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
Comment 16•11 years ago
|
||
Okay, I guess we can cancel the nom then. Thanks for looking into this.
Updated•11 years ago
|
Attachment #582374 -
Flags: approval-mozilla-aurora?
You need to log in
before you can comment on or make changes to this bug.
Description
•