Update MemoryObserver.dumpMemoryStats to reuse existing code

RESOLVED FIXED in Firefox 19

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

unspecified
Firefox 19
All
Android
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Bug 788021 added some code to dump about:memory stats to a file. We should change the MemoryObserver code in browser.js (added in bug 783230) to just invoke that instead of duplicating a bunch of stuff.
Posted patch PatchSplinter Review
This changes it from dumping out to logcat to saving the dump into a gzipped file. The location of the file is written out to logcat, e.g.:

10-09 16:29:16.906 E/GeckoConsole( 5353): nsIMemoryReporterManager::dumpReports() dumped reports to /data/data/org.mozilla.fennec_kats/app_tmp/memory-report-foo-5353.json.gz
Attachment #669711 - Flags: review?(mark.finkle)
Depends on: 799686
Comment on attachment 669711 [details] [diff] [review]
Patch

>+    memMgr.dumpMemoryReportsToFile(aLabel, false, true);

The last param is "dumpChildProcesses". Do we want to do that? What child processes do we have?

r+, but I'm all for doing less work if we can get away with it.
Attachment #669711 - Flags: review?(mark.finkle) → review+
(In reply to Mark Finkle (:mfinkle) from comment #2)
> The last param is "dumpChildProcesses". Do we want to do that? What child
> processes do we have?

I threw that in for future-proofing. If we ever spawn a child process we'll probably want to count it as well, and if we start spawning a child process accidentally this might help us notice and fix it. I don't believe that it adds significant overhead, just an extra call to ContentParent::GetAll.
(In reply to Kartikaya Gupta (:kats) from comment #3)
> (In reply to Mark Finkle (:mfinkle) from comment #2)
> > The last param is "dumpChildProcesses". Do we want to do that? What child
> > processes do we have?
> 
> I threw that in for future-proofing. If we ever spawn a child process we'll
> probably want to count it as well, and if we start spawning a child process
> accidentally this might help us notice and fix it. I don't believe that it
> adds significant overhead, just an extra call to ContentParent::GetAll.

OK, as long as the overhead is minimal. Thanks for checking.
https://hg.mozilla.org/mozilla-central/rev/7e369eed5f9f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
You need to log in before you can comment on or make changes to this bug.