Closed Bug 666075 Opened 9 years ago Closed 9 years ago

Add memory multi-reporters

Categories

(Core :: XPCOM, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla7

People

(Reporter: njn, Assigned: njn)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [MemShrink:P1][inbound])

Attachments

(1 file, 1 obsolete file)

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.
Attached patch patch (obsolete) — Splinter 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 on attachment 541596 [details] [diff] [review]
patch

sr=me.  Thank you for the comments!
Attachment #541596 - Flags: superreview?(bzbarsky) → superreview+
Attached patch patch v2Splinter Review
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+
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
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
A follow-up to add some testing of the new code:

http://hg.mozilla.org/integration/mozilla-inbound/rev/d997ca0997de
This is dev-doc-needed for the nsIMemoryReporterManager changes, and the addition of nsIMemoryMultiReporterCallback and nsIMemoryMultiReporter.
Keywords: dev-doc-needed
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)
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. :)
The docs look good, thanks!
You need to log in before you can comment on or make changes to this bug.