806473 - Expose MemoryInfoDumper via a service

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

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

Attachment: Add an nsIMemoryInfoDumper service

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.

Comment 1

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

Attachment: Update browser.js to use nsIMemoryInfoDumper

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)

Comment 2

[Justin Lebar (not reading bugmail)]

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.

Comment 3

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

Attachment: Add an nsIMemoryInfoDumper service (v2)

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.

Comment 4

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

Attachment: Diff of MemoryInfoDumper.cpp -> nsMemoryInfoDumper.cpp

Comment 5

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

Attachment: Diff of MemoryInfoDumper.h -> nsMemoryInfoDumper.h

Same for the .h file, again for ease of reviewing

Comment 6

[Justin Lebar (not reading bugmail)]

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

Comment 7

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

Yeah, I'll do that when I land the patch.. using git for development though :)

Comment 8

[Justin Lebar (not reading bugmail)]

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

Comment 9

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

Attachment: Add an nsIMemoryInfoDumper service (v2)

Now with magical -M -C options!

Comment 10

[Justin Lebar (not reading bugmail)]

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

Comment 11

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

Build was green on all platforms: https://tbpl.mozilla.org/?tree=Try&rev=b74dfeaf130e

Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/963873ff3081
https://hg.mozilla.org/integration/mozilla-inbound/rev/471b0731ff8d

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/963873ff3081
https://hg.mozilla.org/mozilla-central/rev/471b0731ff8d

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Why did you change the C++ callers to use the service? That doesn't make any sense at all.

Comment 14

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

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.

Comment 15

[Justin Lebar (not reading bugmail)]

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