Add memory multi-reporters

RESOLVED FIXED in mozilla7



6 years ago
6 years ago


(Reporter: njn, Assigned: njn)



Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [MemShrink:P1][inbound])


(1 attachment, 1 obsolete attachment)



6 years ago
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

6 years ago
Created attachment 541596 [details] [diff] [review]

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
Assignee: nobody → nnethercote
Attachment #541596 - Flags: superreview?(bzbarsky)
Attachment #541596 - Flags: review?(khuey)

Comment 2

6 years ago
Comment on attachment 541596 [details] [diff] [review]

sr=me.  Thank you for the comments!
Attachment #541596 - Flags: superreview?(bzbarsky) → superreview+

Comment 3

6 years ago
Created attachment 542355 [details] [diff] [review]
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.
Attachment #541596 - Attachment is obsolete: true
Attachment #542355 - Flags: superreview+
Attachment #542355 - Flags: review?(khuey)
Attachment #541596 - Flags: review?(khuey)
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 @@
> +};
> +  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 @@
> +};
> +  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.
Attachment #542355 - Flags: review?(khuey) → review+

Comment 5

6 years ago
Whiteboard: [MemShrink:P1] → [MemShrink:P1][inbound]
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7

Comment 7

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

Comment 8

6 years ago
This is dev-doc-needed for the nsIMemoryReporterManager changes, and the addition of nsIMemoryMultiReporterCallback and nsIMemoryMultiReporter.
Keywords: dev-doc-needed
test merge
Flags: in-testsuite+
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?
Locally or on MXR? (MXR is like 2 weeks out of date)
MXR. Wait, MXR is out of date? Argh... that makes work hard.
Indeed. (See bug 678527 for progress)

Comment 14

6 years ago
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.
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. :)


Also mentioned on Firefox 7 for developers.
Keywords: dev-doc-needed → dev-doc-complete

Comment 17

6 years ago
The docs look good, thanks!
You need to log in before you can comment on or make changes to this bug.