Closed Bug 660731 Opened 13 years ago Closed 13 years ago

Add GetExplicit and GetResident methods to NSIMemoryReporterManager

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [inbound])

Attachments

(1 file, 2 obsolete files)

"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.
Blocks: 663423
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #543042 - Flags: review?(jst)
Attached patch patch, v2 (obsolete) — Splinter Review
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.
Attachment #543042 - Attachment is obsolete: true
Attachment #543042 - Flags: review?(jst)
Attachment #543314 - Flags: superreview?(bzbarsky)
Attachment #543314 - Flags: review?(jst)
Comment on attachment 543314 [details] [diff] [review]
patch, v2

It would be good to document what "explicit" actually means... sr=me with that.
Attachment #543314 - Flags: superreview?(bzbarsky) → superreview+
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
Attachment #543314 - Flags: review?(jst)
Attached patch patch, v3Splinter Review
Attachment #543314 - Attachment is obsolete: true
Attachment #543718 - Flags: review?(khuey)
Attachment #543718 - Flags: superreview+
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]
Whiteboard: [inbound]
this also caused a funny "leaked 9223372036854775808 bytes during test execution" in mochitest-other
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
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).
(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.
http://hg.mozilla.org/mozilla-central/rev/a90ef2d76c54
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Documentation was already updated:

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

I've added mentions to the Firefox 8 for developers page.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: