Add memory multi-reporters

RESOLVED FIXED in mozilla7

Status

()

Core
XPCOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

({dev-doc-complete})

unspecified
mozilla7
x86_64
Linux
dev-doc-complete
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

6 years ago
Created attachment 541596 [details] [diff] [review]
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
Assignee: nobody → nnethercote
Attachment #541596 - Flags: superreview?(bzbarsky)
Attachment #541596 - Flags: review?(khuey)
Comment on attachment 541596 [details] [diff] [review]
patch

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

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 @@
> +};
> +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.
Attachment #542355 - Flags: review?(khuey) → review+
(Assignee)

Comment 5

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/a053ab0d8633
Whiteboard: [MemShrink:P1] → [MemShrink:P1][inbound]
http://hg.mozilla.org/mozilla-central/rev/a053ab0d8633
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
(Assignee)

Comment 7

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

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

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 http://hg.mozilla.org/mozilla-central/rev/d997ca0997de
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)
(Assignee)

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. :)
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.
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

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.