Closed Bug 1297658 Opened 5 years ago Closed 5 years ago
Avoid unnecessary checking in memory reporters
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.)
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.
(Updated patch fixes a minor error in SystemMemoryReporter.cpp.)
Attachment #8784322 - 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!
https://hg.mozilla.org/integration/mozilla-inbound/rev/7651ea2490e0f0d36757ee238fcf4446b1869d18 Bug 1297658 - Avoid unnecessary checking in memory reporters. r=erahm.
You need to log in before you can comment on or make changes to this bug.