Closed
Bug 660731
Opened 13 years ago
Closed 12 years ago
Add GetExplicit and GetResident methods to NSIMemoryReporterManager
Categories
(Core :: XPCOM, defect)
Core
XPCOM
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)
17.51 KB,
patch
|
khuey
:
review+
n.nethercote
:
superreview+
|
Details | Diff | Splinter Review |
"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.
![]() |
Assignee | |
Comment 1•12 years ago
|
||
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•12 years ago
|
||
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 3•12 years ago
|
||
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•12 years ago
|
||
Attachment #543314 -
Attachment is obsolete: true
Attachment #543718 -
Flags: review?(khuey)
Attachment #543718 -
Flags: review?(khuey) → review+
![]() |
Assignee | |
Updated•12 years ago
|
Attachment #543718 -
Flags: superreview+
![]() |
Assignee | |
Comment 6•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/8ab62172a2cb
Whiteboard: [inbound]
Comment 7•12 years ago
|
||
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]
Comment 8•12 years ago
|
||
this also caused a funny "leaked 9223372036854775808 bytes during test execution" in mochitest-other
Comment 9•12 years ago
|
||
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•12 years ago
|
||
Attempt 2: http://hg.mozilla.org/integration/mozilla-inbound/rev/30f74655c985
Comment 11•12 years ago
|
||
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•12 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•12 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•12 years ago
|
||
3rd time lucky: http://hg.mozilla.org/integration/mozilla-inbound/rev/a90ef2d76c54
Whiteboard: [inbound]
Comment 15•12 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/a90ef2d76c54
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Updated•12 years ago
|
Keywords: dev-doc-needed
Comment 16•12 years ago
|
||
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.
Description
•