1297658 - Avoid unnecessary checking in memory reporters

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Nicholas Nethercote [inactive]]

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

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Avoid unnecessary checking in memory reporters

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.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Avoid unnecessary checking in memory reporters

(Updated patch fixes a minor error in SystemMemoryReporter.cpp.)

Comment 3

[Eric Rahm [: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.

Comment 4

[Nicholas Nethercote [inactive]]

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

Comment 5

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7651ea2490e0f0d36757ee238fcf4446b1869d18
Bug 1297658 - Avoid unnecessary checking in memory reporters. r=erahm.

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7651ea2490e0