Closed Bug 806473 Opened 7 years ago Closed 7 years ago

Expose MemoryInfoDumper via a service

Categories

(Core :: XPCOM, defect)

19 Branch
All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(2 files, 4 obsolete files)

Bug 800486 moved the dump methods from nsMemoryReporterManager to MemoryInfoDumper but also made them inaccessible from script. Bug 799476 relies on those methods being accessible to script, so that we can dump about:memory on demand and analyze the dumps for potential memory reductions.

The attached patch exposes the methods again via a newly-created nsIMemoryInfoDumper service.
Attachment #676239 - Flags: review?(justin.lebar+bug)
Trivial patch to update the mobile/android/chrome/content/browser.js file to use the new service instead of the old one (which doesn't work any more)
Attachment #676240 - Flags: review?(justin.lebar+bug)
Attachment #676240 - Flags: review?(justin.lebar+bug) → review+
Comment on attachment 676239 [details] [diff] [review]
Add an nsIMemoryInfoDumper service

This looks good, but would you mind merging nsMemoryInfoDumper and MemoryInfoDumper?  It's more of a pain to call an XPCOM service than it is to invoke some static methods, but it's better not to have two classes that do the same thing, I think.  We could add the static methods back onto the one class, if you wanted.
Attachment #676239 - Flags: review?(justin.lebar+bug)
Updated to just have a single nsMemoryInfoDumper class which implements the nsIMemoryInfoDumper interface. I also renamed the file to nsMemoryInfoDumper.{h, cpp}. I'll attach the diffs of the old MemoryInfoDumper.cpp to the new nsMemoryInfoDumper.cpp as well for easier review.
Attachment #676239 - Attachment is obsolete: true
Attachment #676307 - Flags: review?(justin.lebar+bug)
Same for the .h file, again for ease of reviewing
(In reply to Kartikaya Gupta (:kats) from comment #4)
> Created attachment 676308 [details] [diff] [review]
> Diff of MemoryInfoDumper.cpp -> nsMemoryInfoDumper.cpp

You need to do hg rename, so hg knows that you renamed the file.  You'll get nice diffs that way as a side-effect.
Yeah, I'll do that when I land the patch.. using git for development though :)
(In reply to Kartikaya Gupta (:kats) from comment #7)
> Yeah, I'll do that when I land the patch.. using git for development though
> :)

With git it's even easier; just pass -M -C to git diff along with -U8.
Now with magical -M -C options!
Attachment #676307 - Attachment is obsolete: true
Attachment #676308 - Attachment is obsolete: true
Attachment #676310 - Attachment is obsolete: true
Attachment #676307 - Flags: review?(justin.lebar+bug)
Attachment #676336 - Flags: review?(justin.lebar+bug)
>diff --git a/xpcom/base/nsMemoryInfoDumper.h b/xpcom/base/nsMemoryInfoDumper.h
>+#ifndef mozilla_MemoryInfoDumper_h
>+#define mozilla_MemoryInfoDumper_h

Nit: Please rename the ifdefs.

>+#include "mozilla/StandardInteger.h"
>+class nsIGZFileWriter;

Nit that's not your fault: These don't appear to be necessary anymore; can you
please remove them?

Looks good to me with these small changes.  Thanks for writing this patch.
Attachment #676336 - Flags: review?(justin.lebar+bug) → review+
https://hg.mozilla.org/mozilla-central/rev/963873ff3081
https://hg.mozilla.org/mozilla-central/rev/471b0731ff8d
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Why did you change the C++ callers to use the service? That doesn't make any sense at all.
Well the methods are no longer static so I had to change something. I assume you would have preferred just stack-allocating an instance of the dumper and using that? While that is probably more efficient I thought it would be less good going forward because if we add non-static state to the dumper then it could have unexpected effects. Speculative robustification, so to speak.
(In reply to :Ms2ger from comment #13)
> Why did you change the C++ callers to use the service? That doesn't make any
> sense at all.

Comment 2?

Hey, I don't like XPCOM either; I would have done this with webidl if that had an equivalent of do_GetService callable from chrome JS.
You need to log in before you can comment on or make changes to this bug.