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)
Tracking
()
RESOLVED
DUPLICATE
of bug 1062006
People
(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
n.nethercote
:
review-
|
Details | Diff | Splinter Review |
Right now, nsIMemoryReporterManager::GetResident (used by telemetry) returns just the chrome process's size. It should return our total size, I think.
Assignee | ||
Comment 1•13 years ago
|
||
I tested this on Android, printing out the result here and comparing it to the results in about:memory.
Attachment #568046 -
Flags: review?(nnethercote)
Assignee | ||
Comment 2•13 years ago
|
||
Attachment #568049 -
Flags: review?(nnethercote)
Assignee | ||
Updated•13 years ago
|
Attachment #568046 -
Attachment is obsolete: true
Attachment #568046 -
Flags: review?(nnethercote)
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → justin.lebar+bug
Comment 3•13 years ago
|
||
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-
Assignee | ||
Comment 4•13 years ago
|
||
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.
Assignee | ||
Updated•13 years ago
|
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)
Assignee | ||
Comment 5•13 years ago
|
||
(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.
Comment 6•10 years ago
|
||
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.
Description
•