799476 - Update MemoryObserver.dumpMemoryStats to reuse existing code

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

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

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

(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.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

(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.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e369eed5f9f

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7e369eed5f9f