660731 - Add GetExplicit and GetResident methods to NSIMemoryReporterManager

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nicholas Nethercote [inactive]]

"explicit" and "resident" are two of the most interesting measurements that about:memory reports.  For example, they're the two measurements that MozMill endurance tests record (as of bug 656869).

Both of them can be computed by looking at all the memory reporters.  "resident" is easy -- you just have to find the "resident" reporter.  "explicit" is harder;  you have to find "heap-used" and then sum it with all the reporters with kind=MR_MAPPED that don't have ancestor reporters higher up the tree.  This is a pain, likely to be required in multiple places in the long run, and requires running a moderate amount of JS to compute, which involves additional allocations, thus disturbing the measurements.  Also, there are some edge cases, like the fact that "heap-used" isn't available on --disable-jemalloc builds.

A better option would be to add GetExplicit and GetResident methods to nsIMemoryReporterManager.  These would be coded, once and properly (all edge cases accounted for) in C++ and also wouldn't require any allocation.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: patch

This patch:

- Adds nsIMemoryReporter::{resident,explicit}.  resident is trivial,
  explicit is a bit ugly but that's unavoidable.

- Adds some testing for GetExplicit() and GetResident().  I want to include
  some multi-reporters for the GetExplicit() testing, but I'm getting two
  problems -- an exception and a crash.  Details in the test.  I'm putting
  the patch up for review in the meantime because I want to get it in FF7 if
  possible.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: patch, v2

The old patch had some C++ XPCOM objects on the stack that were passed to JS code, which caused the crashes;  they're now all on the heap.  This includes some cases in ContentChild.cpp from an earlier patch of mine.

The exception was easily fixed, it was due to me confusing a JS function with a JS object-containing-a-single-function.

bz, for super-review the only changes of note are in nsIMemoryReporter.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 543314 [details] [diff] [review]
patch, v2

It would be good to document what "explicit" actually means... sr=me with that.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 543314 [details] [diff] [review]
patch, v2

Review of attachment 543314 [details] [diff] [review]:
-----------------------------------------------------------------

Looks pretty good, but a lot of comments, so I'd like to see another diff before giving an r+.

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +515,5 @@
> +    *aResident = ::GetResident();
> +    return NS_OK;
> +}
> +
> +struct MR {

Please give this a better name.  You can afford to burn a few chars on "MemoryReport"

@@ +516,5 @@
> +    return NS_OK;
> +}
> +
> +struct MR {
> +    MR(const nsACString &path, PRInt64 amount) : path(path), amount(amount) {}

Heap-allocated structs should have MOZ_COUNT_[C|D]TOR in the ctor/dtor respectively.  This gets them tracked so we notice if we leak them.

@@ +523,5 @@
> +};
> +
> +// This is just a wrapper for InfallibleTArray<MR> that implements
> +// nsISupports, so it can be passed to nsIMemoryMultiReporter::CollectReports.
> +class MRWrapper : public nsISupports {

MemoryReportsWrapper ...

@@ +531,5 @@
> +    InfallibleTArray<MR> *mReports;
> +};
> +NS_IMPL_ISUPPORTS0(MRWrapper)
> +
> +class MRCallback : public nsIMemoryMultiReporterCallback

MemoryReportsCallback?

@@ +539,5 @@
> +
> +    NS_IMETHOD Callback(const nsACString &aProcess, const nsACString &aPath,
> +                        PRInt32 aKind, PRInt32 aUnits, PRInt64 aAmount,
> +                        const nsACString &aDescription,
> +                        nsISupports *aiWrappedMRs)

aWrappedMRs?

@@ +552,5 @@
> +};
> +NS_IMPL_THREADSAFE_ISUPPORTS1(
> +  MRCallback
> +, nsIMemoryMultiReporterCallback
> +)

Why does this need to be threadsafe?

@@ +567,5 @@
> +    for (i = 0; i < path1.Length(); i++) {
> +        if (path1[i] != path2[i]) {
> +            return false;
> +        }
> +    }

Replace this loop with:

const nsACString& subStr = Substring(path2, 0, path1.Length());
if (!subStr.Equals(path1))
  return false;

or something like that (see https://developer.mozilla.org/En/Mozilla_internal_string_guide#Substrings_%28string_fragments%29) and welcome to the fun world of our horrific string classes.

@@ +568,5 @@
> +        if (path1[i] != path2[i]) {
> +            return false;
> +        }
> +    }
> +    return path2[i] == '/';    // first non-matching char must be a separator

and then

return path2[path1.Length()] == '/';

if I did the math right in my head.

@@ +572,5 @@
> +    return path2[i] == '/';    // first non-matching char must be a separator
> +}
> +
> +NS_IMETHODIMP
> +nsMemoryReporterManager::GetExplicit(PRInt64 *aExplicit)

This looks fine, but it's very terse.  Put in some newlines to group things logically?

@@ +580,5 @@
> +
> +    // Get "heap-used" and all the KIND_MAPPED measurements from vanilla
> +    // reporters.
> +    nsCOMPtr<nsISimpleEnumerator> e;
> +    EnumerateReporters(getter_AddRefs(e));

e.g. I'd put a newline here

@@ +584,5 @@
> +    EnumerateReporters(getter_AddRefs(e));
> +    PRBool more;
> +    while (NS_SUCCEEDED(e->HasMoreElements(&more)) && more) {
> +        nsCOMPtr<nsIMemoryReporter> r;
> +        e->GetNext(getter_AddRefs(r));

And here

@@ +587,5 @@
> +        nsCOMPtr<nsIMemoryReporter> r;
> +        e->GetNext(getter_AddRefs(r));
> +        PRInt32 kind;
> +        nsresult rv = r->GetKind(&kind);
> +        NS_ENSURE_SUCCESS(rv, rv);

and here

@@ +590,5 @@
> +        nsresult rv = r->GetKind(&kind);
> +        NS_ENSURE_SUCCESS(rv, rv);
> +        if (kind == nsIMemoryReporter::KIND_MAPPED) {
> +            char* path;
> +            rv = r->GetPath(&path);

You're leaking here ... (ironic, isn't it?)

I think you want to use nsAutoPtr<char> path and r->GetPath(getter_Transfers(path));

@@ +591,5 @@
> +        NS_ENSURE_SUCCESS(rv, rv);
> +        if (kind == nsIMemoryReporter::KIND_MAPPED) {
> +            char* path;
> +            rv = r->GetPath(&path);
> +            NS_ENSURE_SUCCESS(rv, rv);

anywhere you use NS_ENSURE_SUCCESS probably wants a break after it

@@ +596,5 @@
> +            PRInt64 amount;
> +            rv = r->GetAmount(&amount);
> +            NS_ENSURE_SUCCESS(rv, rv);
> +            if (amount != PRInt64(-1)) {
> +                MR mr(nsCString(path), amount);

I think you can use nsAdoptingCString(path.forget()) here to avoid another copy.

@@ +601,5 @@
> +                mapped.AppendElement(mr);
> +            }
> +        } else {
> +            char* path;
> +            rv = r->GetPath(&path);

You're leaking here too

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: patch, v3

Comment 6

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/8ab62172a2cb

Comment 7

[Marco Bonardo [:mak]]

backed out due to test failure:

TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/xpcshell/tests/toolkit/components/telemetry/tests/unit/test_TelemetryPing.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIMemoryReporterManager.explicit]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: jar:file:///home/cltbld/talos-slave/test/build/firefox/omni.jar!/components/TelemetryPing.js :: gatherMemory :: line 255" data: no]

Comment 8

[Marco Bonardo [:mak]]

this also caused a funny "leaked 9223372036854775808 bytes during test execution" in mochitest-other

Comment 9

[Marco Bonardo [:mak]]

comment 7 happens in Linux debug and Windows debug
http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-Inbound/1309778110.1309781345.22568.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-Inbound/1309769467.1309771210.1410.gz

comment 8 refers to Windows Debug:
http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-Inbound/1309778109.1309782410.28255.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-Inbound/1309778110.1309782336.27968.gz

Comment 10

[Nicholas Nethercote [inactive]]

Attempt 2:

http://hg.mozilla.org/integration/mozilla-inbound/rev/30f74655c985

Comment 11

[Marco Bonardo [:mak]]

Backed out again, now XPCShell is fixed, but Windows is still reporting nonsense leaks:

TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 9223372036854775808 bytes during test execution

http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-Inbound/1309848930.1309853257.4764.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-Inbound/1309848914.1309853309.5008.gz

Comment 12

[Nicholas Nethercote [inactive]]

khuey, this looks like the problem:

        if (aKind == nsIMemoryReporter::KIND_MAPPED && aAmount != PRInt64(-1)) {
            MemoryReportsWrapper *wrappedMRs =
                static_cast<MemoryReportsWrapper *>(aWrappedMRs);
            MemoryReport mr(aPath, aAmount);
            wrappedMRs->mReports->AppendElement(mr);
        }

A stack-allocated MemoryReport struct is being put into an InfallibleTArray<MemoryReport> that lives outside the function.  What's the right way to do this?  Presumably it should be on the heap;  can structs be made to fit in with Gecko's refcounting?  Should I turn the struct into a class that inherits from nsISupports?

It's also weird that this problem was only being reported on Windows, and that the output was so ridiculous (9223372036854775808 bytes).

Comment 13

[Nicholas Nethercote [inactive]]

(In reply to comment #12)
> khuey, this looks like the problem:
> 
>         if (aKind == nsIMemoryReporter::KIND_MAPPED && aAmount !=
> PRInt64(-1)) {
>             MemoryReportsWrapper *wrappedMRs =
>                 static_cast<MemoryReportsWrapper *>(aWrappedMRs);
>             MemoryReport mr(aPath, aAmount);
>             wrappedMRs->mReports->AppendElement(mr);
>         }
> 
> A stack-allocated MemoryReport struct is being put into an
> InfallibleTArray<MemoryReport> that lives outside the function.

Oh wait, presumably the MemoryReport is copied when the append happens?

An interesting thing I found -- the MOZ_COUNT_CTOR/MOZ_COUNT_DTOR counts for MemoryReport don't match up.  There are twice as many DTOR ones, presumably because the MemoryReports in the array get copy constructed.  Would that cause the leak message?  I have a try server run going to test that hyphothesis.

It's weird that MemoryReport is accused of leaking, because AFAICT all the MemoryReports are on the stack, either solo or in a stack-allocated InfallibleTArray.

Comment 14

[Nicholas Nethercote [inactive]]

3rd time lucky:

http://hg.mozilla.org/integration/mozilla-inbound/rev/a90ef2d76c54

Comment 15

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/a90ef2d76c54

Comment 16

[Eric Shepherd [:sheppy]]

Documentation was already updated:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIMemoryReporterManager

I've added mentions to the Firefox 8 for developers page.