The default bug view has changed. See this FAQ.

Add GetExplicit and GetResident methods to NSIMemoryReporterManager

RESOLVED FIXED in mozilla8

Status

()

Core
XPCOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

({dev-doc-complete})

unspecified
mozilla8
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
"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: 656869
(Assignee)

Updated

6 years ago
Blocks: 663423
(Assignee)

Comment 1

6 years ago
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.
Attachment #543042 - Flags: review?(jst)
(Assignee)

Comment 2

6 years ago
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.
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)
(Assignee)

Comment 5

6 years ago
Created attachment 543718 [details] [diff] [review]
patch, v3
Attachment #543314 - Attachment is obsolete: true
Attachment #543718 - Flags: review?(khuey)
Attachment #543718 - Flags: review?(khuey) → review+
(Assignee)

Updated

6 years ago
Attachment #543718 - Flags: superreview+
(Assignee)

Comment 6

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/8ab62172a2cb
Whiteboard: [inbound]
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
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
(Assignee)

Comment 10

6 years ago
Attempt 2:

http://hg.mozilla.org/integration/mozilla-inbound/rev/30f74655c985
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
(Assignee)

Comment 12

6 years ago
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).
(Assignee)

Comment 13

6 years ago
(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.
(Assignee)

Comment 14

6 years ago
3rd time lucky:

http://hg.mozilla.org/integration/mozilla-inbound/rev/a90ef2d76c54
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/a90ef2d76c54
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Keywords: dev-doc-needed
Documentation was already updated:

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

I've added mentions to the Firefox 8 for developers page.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.