947802 - Replace enumerateReporters with something better

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nicholas Nethercote [inactive]]

More memory reporters clean-ups, basically.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Inline DumpReport().

This patch inlines DumpReport().  It also adds mWriter to DumpReportCallback,
so the writer is obtained that way instead of via the |aData| argument.  This
simplifies things because it lets us use nsGZFileWriters throughout, instead of
sometimes having a nsIGZFileWriter and having to QI.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Replace enumerateReporters() with getReportsForThisProcess().

Currently, to get memory reports from all processes you call getReports(),
which takes a callback.  And to get memory reports from just the current
process, you call enumerateReporters() and iterate over them, calling
collectReports() for each one, which takes a callback.

This patch removes enumerateReporters() and replaces it with
getReportsInThisProcess().  This matches getReports(), and is simpler for
callers to use.

Comment 3

[Andrew McCreight [:mccr8]]

Comment on attachment 8344470 [details] [diff] [review]
(part 2) - Replace enumerateReporters() with getReportsForThisProcess().

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

::: xpcom/base/nsMemoryInfoDumper.cpp
@@ +874,1 @@
>     return rv;

Remove this |return rv|; you are doing an unconditional early return.

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +1095,5 @@
>      // Get reports for this process.
> +    GetReportsForThisProcess(aHandleReport, aHandleReportData);
> +
> +    // If there are no child processes, we can finish up immediately.
> +    return (mNumChildProcesses == 0)

The use of the trinary operator here seems gratuitous.

@@ +1106,5 @@
> +    nsIHandleReportCallback* aHandleReport,
> +    nsISupports* aHandleReportData)
> +{
> +    // Memory reporters are not necessarily threadsafe, so this function must
> +    // be called from the main thread.

I guess GetReportsForThisProcessWhichMustBeTheMainProcess is a little long, but it still seems unfortunate that the name doesn't mention the mainthreadness of it.  GetReportsForThisMainProcess?  Maybe that's just me.  At a minimum, you should document this requirement in the idl file.

Comment 4

[Nicholas Nethercote [inactive]]

johns and Dave Hunt:  I think this change will affect AWSY and the Endurance tests.  You'll probably have to change a few lines of code... see the .js changes in part 2 for examples.

Can I do anything to make things easier for you?  E.g. wait a little while before landing?

Comment 5

[Nicholas Nethercote [inactive]]

> ::: xpcom/base/nsMemoryInfoDumper.cpp
> @@ +874,1 @@
> >     return rv;
> 
> Remove this |return rv|; you are doing an unconditional early return.

That's actually a splinter fail;  there was a bad newline (vim showed it as "^M") which I fixed.  The raw patch and the traditional diff view both show it correctly, but splinter gets it wrong.

> > +    // Memory reporters are not necessarily threadsafe, so this function must
> > +    // be called from the main thread.
> 
> I guess GetReportsForThisProcessWhichMustBeTheMainProcess is a little long,
> but it still seems unfortunate that the name doesn't mention the
> mainthreadness of it.  GetReportsForThisMainProcess?  Maybe that's just me. 
> At a minimum, you should document this requirement in the idl file.

Are you confusing the main process and the main thread?  This doesn't have to be called in the main process.  E.g. for the memory dumping on B2G it gets called in every child process, too.

Comment 6

[Andrew McCreight [:mccr8]]

(In reply to Nicholas Nethercote [:njn] from comment #5)
> That's actually a splinter fail;  there was a bad newline (vim showed it as
> "^M") which I fixed.  The raw patch and the traditional diff view both show
> it correctly, but splinter gets it wrong.

I checked the raw patch at https://bug947802.bugzilla.mozilla.org/attachment.cgi?id=8344470 (because I assumed it was just a splinter bug) and it shows me:

-  if (NS_WARN_IF(NS_FAILED(rv)))
    return rv;
+  if (NS_WARN_IF(NS_FAILED(rv)))
+    return rv;

But ok.

> Are you confusing the main process and the main thread?  This doesn't have
> to be called in the main process.  E.g. for the memory dumping on B2G it
> gets called in every child process, too.

Oops!  Yeah, sorry about that.

Comment 7

[John Schoenick [:johns]]

This should be a trivial change for AWSY, and I can re-queue any failed tests if I don't get to it before this lands.

Comment 8

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b9a394d1c11e
https://hg.mozilla.org/integration/mozilla-inbound/rev/1965b63bb333

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b9a394d1c11e
https://hg.mozilla.org/mozilla-central/rev/1965b63bb333

Comment 10

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Nicholas Nethercote [:njn] from comment #4)
> johns and Dave Hunt:  I think this change will affect AWSY and the Endurance
> tests.  You'll probably have to change a few lines of code... see the .js
> changes in part 2 for examples.
> 
> Can I do anything to make things easier for you?  E.g. wait a little while
> before landing?

I don't believe this will affect the endurance tests. If it does, we should be able to fix them up and re-run any failed tests.

Comment 11

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/65d76108e7356051e2b7f1b8d4006588c9b341f3
Bug 947802 (part 2) - Replace enumerateReporters() with getReportsForThisProcess(). r=mccr8.

Comment 12

[John Schoenick [:johns]]

AWSY updated for this:

https://github.com/Nephyrin/MozAreWeSlimYet/commit/cba6506c8ff4b503955e40b96cd986f8b412770a

I re-queued all incomplete builds on tinderbox so the AWSY failures since this landed will be re-tested

Comment 13

[Nicholas Nethercote [inactive]]

Thanks, johns!