Closed
Bug 947802
Opened 11 years ago
Closed 11 years ago
Replace enumerateReporters with something better
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(2 files)
7.43 KB,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
18.68 KB,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
More memory reporters clean-ups, basically.
Assignee | ||
Comment 1•11 years ago
|
||
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•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #8344468 -
Flags: review?(continuation) → review+
Comment 3•11 years ago
|
||
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•11 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•11 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.
Comment 6•11 years ago
|
||
(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•11 years ago
|
||
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•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9a394d1c11e https://hg.mozilla.org/integration/mozilla-inbound/rev/1965b63bb333
Flags: needinfo?(dave.hunt)
Blocks: 948273
Comment 9•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b9a394d1c11e https://hg.mozilla.org/mozilla-central/rev/1965b63bb333
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Comment 10•11 years ago
|
||
(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•11 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.
Comment 12•11 years ago
|
||
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•11 years ago
|
||
Thanks, johns!
You need to log in
before you can comment on or make changes to this bug.
Description
•