Last Comment Bug 660731 - Add GetExplicit and GetResident methods to NSIMemoryReporterManager
: Add GetExplicit and GetResident methods to NSIMemoryReporterManager
Status: RESOLVED FIXED
[inbound]
: dev-doc-complete
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla8
Assigned To: Nicholas Nethercote [:njn] (on vacation until July 11)
:
Mentors:
Depends on:
Blocks: 656869 663423
  Show dependency treegraph
 
Reported: 2011-05-30 20:17 PDT by Nicholas Nethercote [:njn] (on vacation until July 11)
Modified: 2011-10-18 08:22 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (13.98 KB, patch)
2011-06-29 20:08 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
no flags Details | Diff | Review
patch, v2 (14.12 KB, patch)
2011-06-30 18:06 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
bzbarsky: superreview+
Details | Diff | Review
patch, v3 (17.51 KB, patch)
2011-07-03 22:58 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
khuey: review+
n.nethercote: superreview+
Details | Diff | Review

Description Nicholas Nethercote [:njn] (on vacation until July 11) 2011-05-30 20:17:49 PDT
"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 [:njn] (on vacation until July 11) 2011-06-29 20:08:32 PDT
Created attachment 543042 [details] [diff] [review]
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 [:njn] (on vacation until July 11) 2011-06-30 18:06:07 PDT
Created attachment 543314 [details] [diff] [review]
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 [:bz] (Out June 25-July 6) 2011-06-30 21:20:03 PDT
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 [:khuey] (khuey@mozilla.com) 2011-07-03 22:37:38 PDT
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 [:njn] (on vacation until July 11) 2011-07-03 22:58:52 PDT
Created attachment 543718 [details] [diff] [review]
patch, v3
Comment 6 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-07-03 23:22:45 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/8ab62172a2cb
Comment 7 Marco Bonardo [::mak] 2011-07-04 03:43:22 PDT
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] 2011-07-04 08:14:43 PDT
this also caused a funny "leaked 9223372036854775808 bytes during test execution" in mochitest-other
Comment 10 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-07-05 00:30:05 PDT
Attempt 2:

http://hg.mozilla.org/integration/mozilla-inbound/rev/30f74655c985
Comment 11 Marco Bonardo [::mak] 2011-07-05 01:34:36 PDT
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 [:njn] (on vacation until July 11) 2011-07-05 02:04:36 PDT
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 [:njn] (on vacation until July 11) 2011-07-06 00:12:20 PDT
(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 [:njn] (on vacation until July 11) 2011-07-06 23:02:50 PDT
3rd time lucky:

http://hg.mozilla.org/integration/mozilla-inbound/rev/a90ef2d76c54
Comment 15 Marco Bonardo [::mak] 2011-07-07 03:36:34 PDT
http://hg.mozilla.org/mozilla-central/rev/a90ef2d76c54
Comment 16 Eric Shepherd [:sheppy] 2011-10-18 08:22:25 PDT
Documentation was already updated:

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

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

Note You need to log in before you can comment on or make changes to this bug.