Replace enumerateReporters with something better

RESOLVED FIXED in mozilla29

Status

()

Core
XPCOM
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla29
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

4 years ago
More memory reporters clean-ups, basically.
(Assignee)

Comment 1

4 years ago
Created attachment 8344468 [details] [diff] [review]
(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.
Attachment #8344468 - Flags: review?(continuation)
(Assignee)

Comment 2

4 years ago
Created attachment 8344470 [details] [diff] [review]
(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.
Attachment #8344470 - Flags: review?(continuation)
Attachment #8344468 - Flags: review?(continuation) → review+
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.
Attachment #8344470 - Flags: review?(continuation) → review+
(Assignee)

Comment 4

4 years ago
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?
Flags: needinfo?(jschoenick)
Flags: needinfo?(dave.hunt)
(Assignee)

Comment 5

4 years ago
> ::: 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.
(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.
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.
Flags: needinfo?(jschoenick)
(Assignee)

Comment 8

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9a394d1c11e
https://hg.mozilla.org/integration/mozilla-inbound/rev/1965b63bb333
Flags: needinfo?(dave.hunt)

Updated

4 years ago
Blocks: 948273
https://hg.mozilla.org/mozilla-central/rev/b9a394d1c11e
https://hg.mozilla.org/mozilla-central/rev/1965b63bb333
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
(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

4 years ago
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.
Depends on: 948686
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
(Assignee)

Comment 13

4 years ago
Thanks, johns!
You need to log in before you can comment on or make changes to this bug.