Avoid unnecessary checking in memory reporters

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

3 years ago
A CollectReports() implementation typically looks like this:

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

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

  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.)
Assignee

Comment 1

3 years ago
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)
Assignee

Comment 2

3 years ago
(Updated patch fixes a minor error in SystemMemoryReporter.cpp.)
Attachment #8784322 - Flags: review?(erahm)
Assignee

Updated

3 years ago
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+
Assignee

Comment 4

3 years ago
Thank you for the fast review. I know it was a big patch!

Comment 6

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7651ea2490e0
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.