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

Status: RESOLVED DUPLICATE of bug 1062006

(Toolkit :: about:memory, defect)

Description

[Justin Lebar (not reading bugmail)]

Right now, nsIMemoryReporterManager::GetResident (used by telemetry) returns just the chrome process's size.  It should return our total size, I think.

Comment 1

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

I tested this on Android, printing out the result here and comparing it to the results in about:memory.

Comment 2

[Justin Lebar (not reading bugmail)]

Attachment: Patch v2 (now with less debugging code)

Comment 3

[Nicholas Nethercote [inactive]]

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.

Comment 4

[Justin Lebar (not reading bugmail)]

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.

Comment 5

[Justin Lebar (not reading bugmail)]

(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

[Nicholas Nethercote [inactive]]

I've closed this in favour of bug 1062006, which I think describes the problem more clearly.