744311 - Don't use -1 to represent "unknown" in memory reporters

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: patch 1: trivial preliminary stuff

This patch just does trivial stuff as general clean-up and in preparation
for the next patch, e.g.:

- Moves code around in nsMemoryReporterManager.cpp.

- Changes 2-space indenting to 4-space in nsMemoryReporterManager to match
  the rest of the file.

- Makes the indenting of NS_MEMORY_REPORTER_IMPLEMENT macros more consistent
  throughout the codebase.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: patch 2: the interesting bits

This patch gets rid of the convention where we use (PRInt64)-1 for the
result of a memory reporter that fails.  This makes the code cleaner in a
few places, and allows for future memory reporters that might want to return
negative numbers legitimately.  There are two replacement behaviours.

- For reporters that aren't supported on a particular platform, we now avoid
  registering them, and handle their absence in various ways.

- For reporters that might fail if something weird happens, we now use the
  usual NS_ERROR_* values.

Specific changes of note in the patch:

- aboutMemory.js became a bit simpler.  It's also more careful about putting
  all memory reporter property accesses (which might throw) within a try
  block.

- A new macro, NS_FALLIBLE_MEMORY_REPORTER_IMPLEMENT, is used for
  implementing fallible reporters, such as the ones that query the OS.

- nsMemoryReporterManager.cpp minimizes the number of platform-specific
  #ifdefs by #defining values that indicate which reporters are implemented
  (e.g. HAVE_PAGE_FAULT_REPORTERS), and reusing them later in the file.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: patch 2, v2: the interesting bits

There was some test bustage on --enable-trace-malloc builds (e.g. the debug builds on the test machines) because |mgr.explicit| throws in that configuration, due to the lack of a "heap-allocated" report.  This version handles that.

Comment 3

[Justin Lebar (not reading bugmail)]

Comment on attachment 613872 [details] [diff] [review]
patch 1: trivial preliminary stuff

>+static PRInt64 GetExplicit()
>+{
>+    nsCOMPtr<nsIMemoryReporterManager> mgr = do_GetService("@mozilla.org/memory-reporter-manager;1");

While we're being anal about whitespace, linebreak after "="?

Comment 4

[Justin Lebar (not reading bugmail)]

> [This] allows for future memory reporters that might want to return
> negative numbers legitimately.

What might this be used for?

Comment 5

[Nicholas Nethercote [inactive]]

> What might this be used for?

Changes that could be negative.  When I was reducing the memory consumption of about:memory I was vaguely envisioning some reporters that measured the change in resident since the last time the reporters ran, or something like that.

Even if we can't image such a case, I still like this change for the fact that it gets rid of the non-standard error case (-1) and replaces it with the standard one (nsresult).

Comment 6

[Justin Lebar (not reading bugmail)]

Do you have a use case in mind for GetPath() or GetUnits() failing?  It's not clear to me that it's useful to consider these fallible.

Comment 7

[Justin Lebar (not reading bugmail)]

>diff --git a/toolkit/components/aboutmemory/content/aboutMemory.js b/toolkit/components/aboutmemory/content/aboutMemory.js
>--- a/toolkit/components/aboutmemory/content/aboutMemory.js
>+++ b/toolkit/components/aboutmemory/content/aboutMemory.js
>+    catch (ex) {
>+      // There are two exception cases that must be distinguished here.
>+      //
>+      // - We want to halt proceedings on exceptions thrown within this file
>+      //   (i.e. assertion failures in handleReport);  such exceptions contain
>+      //   gAssertionFailureMsgPrefix in their string representation.
>+      //
>+      // - We want to continue on when faced with exceptions thrown outside
>+      //   this file (i.e. in collectReports).

Maybe "(e.g. when reading a memory reporter's amount)"?  That happens to occur during collectReports, but that's not the important part.

>diff --git a/toolkit/components/telemetry/TelemetryPing.js b/toolkit/components/telemetry/TelemetryPing.js
>--- a/toolkit/components/telemetry/TelemetryPing.js
>+++ b/toolkit/components/telemetry/TelemetryPing.js
>@@ -365,53 +365,60 @@ TelemetryPing.prototype = {
>     } catch (e) {
>       // OK to skip memory reporters in xpcshell
>       return;
>     }
> 
>     let e = mgr.enumerateReporters();
>     while (e.hasMoreElements()) {
>       let mr = e.getNext().QueryInterface(Ci.nsIMemoryReporter);
>-      let id = MEM_HISTOGRAMS[mr.path];
>+      let mrPath, mrAmount, mrUnits;
>+      try {
>+        mrPath = mr.path;
>+      } catch (e) {
>+        continue;
>+      }
>+      let id = MEM_HISTOGRAMS[mrPath];
>       if (!id) {
>         continue;
>       }
>-      // mr.amount is expensive to read in some cases, so get it only once.
>-      let amount = mr.amount;
>-      if (amount == -1) {
>+      try {
>+        mrAmount = mr.amount;
>+        mrUnits = mr.units;

I'm not convinced there's a benefit to assuming that mr.units and mr.path can fail.

>diff --git a/xpcom/base/nsMemoryReporterManager.cpp b/xpcom/base/nsMemoryReporterManager.cpp
>--- a/xpcom/base/nsMemoryReporterManager.cpp
>+++ b/xpcom/base/nsMemoryReporterManager.cpp
>-static PRInt64 GetProcSelfStatmField(int n)
>+static nsresult GetProcSelfStatmField(int field, PRInt64 *n)
> {
>     // There are more than two fields, but we're only interested in the first
>     // two.
>     static const int MAX_FIELD = 2;
>     size_t fields[MAX_FIELD];
>-    NS_ASSERTION(n < MAX_FIELD, "bad field number");
>+    NS_ASSERTION(field < MAX_FIELD, "bad field number");

Would you mind making this MOZ_ASSERT (fatal debug asserrtion, rather than just
a warning), while we're at it?

>     FILE *f = fopen("/proc/self/statm", "r");
>     if (f) {
>         int nread = fscanf(f, "%zu %zu", &fields[0], &fields[1]);
>         fclose(f);
>-        return (PRInt64) ((nread == MAX_FIELD) ? fields[n]*getpagesize() : -1);
>+        if (nread == MAX_FIELD) {
>+            *n = fields[field] * getpagesize();
>+            return NS_OK;
>+        } 

Nit: Whitespace at the end of this line.  There's trailing whitespace in a few
other places too; I'd just strip it off the patch before you check in.

>-static PRInt64 GetVsize()
>+#define HAVE_VSIZE_AND_RESIDENT_REPORTERS 1
>+static nsresult GetVsize(PRInt64 *n)
> {
>-    PRInt64 Vsize, Resident;
>-    XMappingIter(Vsize, Resident);
>-    return Vsize;
>+    PRInt64 vsize, resident;
>+    XMappingIter(vsize, resident);
>+    *n = vsize;
>+    return NS_OK;

What if vsize is -1?

>-static PRInt64 GetResident()
>+static nsresult GetResident(PRInt64 *n)
> {
>-    PRInt64 Vsize, Resident;
>-    XMappingIter(Vsize, Resident);
>-    return Resident;
>+    PRInt64 vsize, resident;
>+    XMappingIter(vsize, resident);
>+    *n = resident;

Similarly for resident.

>-static PRInt64 GetPrivate()
>+#define HAVE_PRIVATE_REPORTER
>+static nsresult GetPrivate(PRInt64 *n)
> {
>     PROCESS_MEMORY_COUNTERS_EX pmcex;
>     pmcex.cb = sizeof(PROCESS_MEMORY_COUNTERS_EX);
> 
>     if (!GetProcessMemoryInfo(GetCurrentProcess(),
>                               (PPROCESS_MEMORY_COUNTERS) &pmcex, sizeof(pmcex)))
>-    return (PRInt64) -1;
>+    {
>+        return NS_ERROR_FAILURE;
>+    }

I don't care, but you may want to cuddle the brace to match (most of) the rest of the file.

>-    return pmcex.PrivateUsage;
>+    *n = pmcex.PrivateUsage;
>+    return NS_OK;
> }
> 
>-NS_MEMORY_REPORTER_IMPLEMENT(Private,
>+NS_FALLIBLE_MEMORY_REPORTER_IMPLEMENT(Private,
>     "private",
>     KIND_OTHER,
>     UNITS_BYTES,
>     GetPrivate,
>     "Memory that cannot be shared with other processes, including memory that "
>     "is committed and marked MEM_PRIVATE, data that is not mapped, and "
>     "executable pages that have been written to.")
> 
>-#else
>+#endif  // HAVE_PRIVATE_REPORTER

That's not accurate -- it's #endif for |#elif(XP_WIN)|.  Maybe you meant to use the HAVE_PRIVATE_REPORTER define to guard this?  I don't care either way, so long as the comment is right.

> static PRInt64 GetHeapCommittedFragmentation()
> {
>     jemalloc_stats_t stats;
>     jemalloc_stats(&stats);
>-    return (PRInt64) 10000 * (1 - stats.allocated / (double)stats.committed);
>+    return 10000 * (1 - stats.allocated / (double)stats.committed);

This doesn't cause a warning?

> static PRInt64 GetHeapDirty()
> {
>     jemalloc_stats_t stats;
>     jemalloc_stats(&stats);
>-    return (PRInt64) stats.dirty;
>+    return stats.dirty;
> }

I think the reason all these PRInt64 casts were there is to make explicit the cast from size_t (unsigned) to PRInt64 (signed).  Even if this doesn't warn now, perhaps -Wconversion would generate a warning.

>+#define HAVE_HEAP_ALLOCATED_AND_EXPLICIT_REPORTERS 1
> static PRInt64 GetHeapUnallocated()
> {
>     struct mstats stats = mstats();
>-    return (PRInt64) (stats.bytes_total - stats.bytes_used);
>+    return stats.bytes_total - stats.bytes_used;
> }

This one in particular might go negative.  Are you sure the behavior is right without the cast (or even, with it?).

I guess I should polish up on my C++ casting rules...

>+#ifdef HAVE_HEAP_ALLOCATED_AND_EXPLICIT_REPORTERS
> NS_MEMORY_REPORTER_IMPLEMENT(HeapUnallocated,
>     "heap-unallocated",
>     KIND_OTHER,
>     UNITS_BYTES,
>     GetHeapUnallocated,
>     "Memory mapped by the heap allocator that is not part of an active "
>-    "allocation. Much of this memory may be uncommitted -- that is, it does not "
>-    "take up space in physical memory or in the swap file.")
>+    "allocation. Much of this memory may be uncommitted -- that is, it does "
>+    "not take up space in physical memory or in the swap file.")
> 
> NS_MEMORY_REPORTER_IMPLEMENT(HeapAllocated,
>     "heap-allocated",
>     KIND_OTHER,
>     UNITS_BYTES,
>     GetHeapAllocated,
>     "Memory mapped by the heap allocator that is currently allocated to the "
>     "application.  This may exceed the amount of memory requested by the "
>     "application because the allocator regularly rounds up request sizes. (The "
>     "exact amount requested is not recorded.)")
> 
>-static PRInt64 GetExplicit()
>+// The computation of "explicit" fails if "heap-allocated" isn't available,
>+// which is why this is depends on HAVE_HEAP_ALLOCATED_AND_EXPLICIT_REPORTERS.

Shouldn't we have a HAVE_HEAP_ALLOCATED_REPORTER define, and then a separate HAVE_EXPLICIT define?  Seems kind of weird to say "we have jemalloc, therefore we have explicit".

> NS_IMETHODIMP
> nsMemoryReporterManager::GetResident(PRInt64 *aResident)
> {
>-    *aResident = ::GetResident();
>-    return NS_OK;
>+#if HAVE_VSIZE_AND_RESIDENT_REPORTERS
>+    return ::GetResident(aResident);
>+#else
>+    *aResident = 0;
>+    return NS_ERROR_NOT_IMPLEMENTED;

Do you think NS_ERROR_NOT_AVAILABLE would be better?

>@@ -812,20 +816,21 @@ nsMemoryReporterManager::GetExplicit(PRI
>     if (explicitNonHeapMultiSize != explicitNonHeapMultiSize2) {
>         char *msg = PR_smprintf("The two measurements of 'explicit' memory "
>                                 "usage don't match (%lld vs %lld)",
>                                 explicitNonHeapMultiSize,
>                                 explicitNonHeapMultiSize2);
>         NS_WARNING(msg);
>         PR_smprintf_free(msg);

FYI (you don't have to change this), you could use nsPrintfCString here instead:

  NS_WARNING(nsPrintfCString(128, "%lld", explicitNonHeapMultiSize).get());

Soon, you won't even need to specify a size in the PrintfCString, which will be nice...

Comment 8

[Justin Lebar (not reading bugmail)]

Comment on attachment 614259 [details] [diff] [review]
patch 2, v2: the interesting bits

r=me, but there are some little things we need to clear up.

Comment 9

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a38b48816c9
https://hg.mozilla.org/integration/mozilla-inbound/rev/62fc36c0317d

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1a38b48816c9
https://hg.mozilla.org/mozilla-central/rev/62fc36c0317d