Closed Bug 1297658 Opened 7 years ago Closed 7 years ago

Avoid unnecessary checking in memory reporters


(Core :: General, defect)

Not set



Tracking Status
firefox51 --- fixed


(Reporter: n.nethercote, Assigned: n.nethercote)



(1 file, 1 obsolete file)

A CollectReports() implementation typically looks like this:

  rv = aHandleReport->Callback("path1", ...);

  rv = aHandleReport->Callback("path2", ...);

  return NS_OK;

Where aHandleReport is a JS function from aboutMemory.js. The callback can
fail, e.g. if the kind or units don't make sense. But if that happens, there's
no reason why we shouldn't try to continue with any remaining calls.

Furthermore, the main place CollectReports() is called is in from a lambda
inside an nsIRunnable that's dispatched to the main thread from
nsMemoryReporterManager::DispatchReporter(). The return value of that
CollectReports() call isn't checked. (It would be hard to propagate failure
backwards, because of all the asyncness/indirection/dispatching. Maybe with
promises? Not that it would gain much.)

Therefore, we might as well just do this instead:

  aHandleReport->Callback("path1", ...);
  aHandleReport->Callback("path2", ...);

  return NS_OK;

because it's simpler.

(If you're wondering why I'm looking at this, it's because I tried adding
[must_use] throughout nsIMemoryReporter.idl. It found a few places where
aHandleReport->Callback() checks were missing, but after thinking about it I
realized that the existing checks weren't useful.)
This patch removes checking of all the callback calls in memory reporter
CollectReport() functions, because it's not useful.

The patch also does some associated clean-up.

- Replaces some uses of nsIMemoryReporterCallback with the preferred
  nsIHandleReportCallback typedef.

- Replaces aCallback/aCb/aClosure with aHandleRepor/aData for CollectReports()
  parameter names, for consistency.

- Adds MOZ_MUST_USE/[must_use] in a few places in nsIMemoryReporter.idl.

- Uses the MOZ_COLLECT_REPORT macro in all suitable places.

Overall the patch reduces code size by ~300 lines and reduces the size of
libxul by about 37 KiB on my Linux64 builds.
Attachment #8784309 - Flags: review?(erahm)
(Updated patch fixes a minor error in SystemMemoryReporter.cpp.)
Attachment #8784322 - Flags: review?(erahm)
Attachment #8784309 - Attachment is obsolete: true
Attachment #8784309 - Flags: review?(erahm)
Comment on attachment 8784322 [details] [diff] [review]
Avoid unnecessary checking in memory reporters

Review of attachment 8784322 [details] [diff] [review]:

Nice cleanup! I agree there's not much point in checking the callback return value. The only use case I can think of is trying to abort the memory reporting, but given each reporter is async(ish) and each process is async it would be rather complicated to support that.
Attachment #8784322 - Flags: review?(erahm) → review+
Thank you for the fast review. I know it was a big patch!
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.