Closed Bug 695676 Opened 13 years ago Closed 10 years ago

nsIMemoryReporterManager::GetResident and GetExplicit should do the Right Thing on multiprocess. (Probably means replacing GetResident with GetPSS)

Categories

(Toolkit :: about:memory, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1062006

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

Details

Attachments

(1 file, 1 obsolete file)

Right now, nsIMemoryReporterManager::GetResident (used by telemetry) returns just the chrome process's size. It should return our total size, I think.
Attached patch Patch v1 (obsolete) (deleted) — Splinter Review
I tested this on Android, printing out the result here and comparing it to the results in about:memory.
Attachment #568046 - Flags: review?(nnethercote)
Attachment #568049 - Flags: review?(nnethercote)
Attachment #568046 - Attachment is obsolete: true
Attachment #568046 - Flags: review?(nnethercote)
Assignee: nobody → justin.lebar+bug
Comment on attachment 568049 [details] [diff] [review] Patch v2 (now with less debugging code) Review of attachment 568049 [details] [diff] [review]: ----------------------------------------------------------------- If we do this for GetResident, should we do something for GetExplicit? ::: xpcom/base/nsMemoryReporterManager.cpp @@ +538,5 @@ > nsMemoryReporterManager::GetResident(PRInt64 *aResident) > { > + // This assumes that resident is not in a multi-reporter. > + PRInt64 total = 0; > + mozilla::MutexAutoLock autoLock(mMutex); GetExplicit doesn't have a lock. Do we really need one here? @@ +551,5 @@ > + PRInt64 amount; > + rv = mReporters[i]->GetAmount(&amount); > + if (NS_FAILED(rv)) { > + continue; > + } I'm suspicious about this. The child process passes static memory reports to the parent process in response to a "child-memory-reporter-request" notification. AFAIK, aboutMemory.js is the only place that generates those notifications. So if you haven't loaded about:memory, the parent process won't have the resident measurement from the child process. In your testing, did you open about:memory first? If so, you might have had stale resident measurements from the child process from when about:memory was loaded. Also, iterating over all the reporters feels inelegant. I wonder if it would be better to somehow directly ask the child for its resident value. The IPC required for about:memory is annoying, and feels particularly unsatisfying to me in this case.
Attachment #568049 - Flags: review?(nnethercote) → review-
Another problem with this patch is that, on multiprocess FF, we really don't want to be measuring RSS. We want to measure PSS. Content processes share libxul with the chrome process, and multiple content processes share code as well. PSS is the right measure.
Summary: nsIMemoryReporterManager::GetResident should return total RSS size across all processes → nsIMemoryReporterManager::GetResident and GetExplicit should do the Right Thing on multiprocess. (Probably means replacing GetResident with GetPSS)
(In reply to Justin Lebar [:jlebar] from comment #4) > Another problem with this patch is that, on multiprocess FF, we really don't > want to be measuring RSS. We want to measure PSS. In particular, if you're measuring one process's memory usage, RSS is OK. If you're measuring total memory usage, you must use PSS.
I've closed this in favour of bug 1062006, which I think describes the problem more clearly.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: