666075 - Add memory multi-reporters

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nicholas Nethercote [inactive]]

Memory reporters implement the nsIMemoryReporter interface, which requires that each reporter take a single measurement.  This works in a lot of cases, but sometimes you really want to do some operation (eg. traverse a data structure) and collect multiple measurements during that operation.

The aim of this bug is to add a nsIMemoryMultiReporter interface that allows this second mode of measurement.  I'm going to blatantly steal Shaver's interface from bug 625305 because it looks exactly like what I need.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: patch

Here's the patch.  I'm confident it's the right interface because I'm doing the per-compartment reporters (bug 661474) on top of it and it makes thing really nice.

bz, the stuff needing super-review attention is basically the .idl changes

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 541596 [details] [diff] [review]
patch

sr=me.  Thank you for the comments!

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: patch v2

A couple of minor changes from the first patch:

- I changed the |description| argument of nsIMemoryMultiReporterCallback::callback from ACString to AUTF8String, to match bz's suggestion for nsIMemoryReporter::description in bug 653627.

- I updated test_aboutmemory.xul appropriately.

Carrying over bz's sr+ from the first patch.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 542355 [details] [diff] [review]
patch v2

Review of attachment 542355 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good ... a few minor things.

::: dom/ipc/ContentChild.cpp
@@ +308,5 @@
> +};
> +NS_IMPL_THREADSAFE_ISUPPORTS1(
> +  MemoryReportsWrapper
> +, nsISupports
> +)

Why does this need to be threadsafe?  Also, you want ISUPPORTS0.  nsISupports is implemented by default.

@@ +322,5 @@
> +    }
> +
> +    NS_IMETHOD Callback(const nsACString &process, const nsACString &path,
> +                        PRInt32 kind, const nsACString &description,
> +                        PRInt64 amount, nsISupports *iWrappedReports)

aProcess, aPath, aKind ... etc

@@ +338,5 @@
> +};
> +NS_IMPL_THREADSAFE_ISUPPORTS1(
> +  MemoryReportCallback
> +, nsIMemoryMultiReporterCallback
> +)

Again, why is this threadsafe?

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +399,5 @@
> +nsMemoryReporterManager::RegisterMultiReporter(nsIMemoryMultiReporter *reporter)
> +{
> +    mozilla::MutexAutoLock autoLock(mMutex);
> +    if (mMultiReporters.IndexOf(reporter) != -1)
> +        return NS_ERROR_FAILURE;

Is this expected to ever happen?  If not, just assert.

Comment 5

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/a053ab0d8633

Comment 6

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/a053ab0d8633

Comment 7

[Nicholas Nethercote [inactive]]

A follow-up to add some testing of the new code:

http://hg.mozilla.org/integration/mozilla-inbound/rev/d997ca0997de

Comment 8

[Nicholas Nethercote [inactive]]

This is dev-doc-needed for the nsIMemoryReporterManager changes, and the addition of nsIMemoryMultiReporterCallback and nsIMemoryMultiReporter.

Comment 9

[Marco Bonardo [:mak]]

test merge http://hg.mozilla.org/mozilla-central/rev/d997ca0997de

Comment 10

[Eric Shepherd [:sheppy]]

I'm confused. I'm looking at nsIMemoryReporter.idl in mozilla-central, and it is missing things that are included in this patch (such as the memoryUsed field on nsIMemoryReporter). What am I doing wrong?

Comment 11

[Ed Morley [:emorley]]

Locally or on MXR? (MXR is like 2 weeks out of date)

Comment 12

[Eric Shepherd [:sheppy]]

MXR. Wait, MXR is out of date? Argh... that makes work hard.

Comment 13

[Ed Morley [:emorley]]

Indeed. (See bug 678527 for progress)

Comment 14

[Nicholas Nethercote [inactive]]

The problem is not that MXR is out of date -- this bug landed 6 weeks ago.  But things have changed in nsIMemoryReporter.idl subsequently.  See bug 671280, bug 653627, bug 660731.

Oh, but this patch only removes references to memoryUsed.

Comment 15

[Eric Shepherd [:sheppy]]

OK, so the actual problem is that bugs are not getting marked as impacting docs, so I didn't know about them. Now they have been. :)

Comment 16

[Eric Shepherd [:sheppy]]

Documented:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIMemoryMultiReporter
https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIMemoryMultiReporterCallback

Updated:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIMemoryReporterManager

Also mentioned on Firefox 7 for developers.

Comment 17

[Nicholas Nethercote [inactive]]

The docs look good, thanks!