715453 - Remove computedSize from nsMallocSizeOfFun

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Nicholas Nethercote [inactive]]

nsMallocSizeOfFun is used in all SizeOf{In,Ex}cludingThis functions:

  typedef size_t(*nsMallocSizeOfFun)(const void *p, size_t computedSize);

The |computedSize| is only used as a fall-back if |moz_malloc_usable_size(p)| returns 0, and that only happens on platforms where malloc_usable_size() or a similar function isn't available.  Solaris is the only such platform that I know of.

We could just get rid of the fall-back:

  typedef size_t(*nsMallocSizeOfFun)(const void *p);

nsMallocSizeOfFun is almost always used for memory reporters, and if we get zero on some obscure platforms that's not a big deal.  In a few cases (e.g. bug 711901 proposes one) nsMallocSizeOfFun is used for other purposes where a non-zero fall-back is important;  for those we could create a 2nd typedef that does have a computedSize, e.g.:

  typedef size_t(*nsMallocSizeOfWithFallbackFun)(const void *p, size_t computedSize);

This would also get rid of the computedSize sanity check, which is failing often enough that it's getting annoying (e.g. bug 705945, bug 710883).

Comment 1

[Phil Ringnalda (:philor)]

The reason behind my annoyance is that it makes the log too big for tbpl to parse, so to star failures in a mochitest-other run, I have to open a different log, get the basedir on ftp.m.o from that, let the 1.7MB compressed log expand out to its full glory of 60MB+, and then find the failure within it myself, like an *animal* :)

Comment 2

[Nicholas Nethercote [inactive]]

I pre-emptively removed the computedSize sanity check because it was failing a lot on some of the tinderbox machines, resulting in huge logs.  edmorley rubber-stamped the patch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f25e8a59a1e

Note that there is at least one more patch to come for this bug, please don't close it when this patch is merged to mozilla-central.

Comment 3

[Ed Morley [:emorley]]

(In reply to Nicholas Nethercote [:njn] from comment #2)
> I pre-emptively removed the computedSize sanity check

https://hg.mozilla.org/mozilla-central/rev/8f25e8a59a1e

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: patch to ignore computedSize

This patch ignores |computedSize| without removing it.  I want to land this and let it bake for a week or two -- to see if we get any complaints -- before removing |computedSize|, because that's a much more invasive patch.

I was worried that on platforms that don't support malloc_usable_size or equivalent, about:memory will be misleading.  So I added some infrastructure to the memory reporter manager, and aboutMemory.js uses that to print a warning up the top when inaccuracies might be present.  I suspect this might save us some hassle in the future by preventing the case where someone reports weird results in about:memory because they're on an odd platform and we don't realize it.

(I tested all three of the warnings by selectively disabling different things, but I can't think how to add a test for them.)

I also fixed the description for the |aHasProblem| case -- now that non-leaf reporters are disallowed, the old description was bogus.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: screenshot of an about:memory accuracy warning

Here's what an accuracy warning in about:memory looks like.  (The final wording I used in the patch changed since I took this screenshot.)

Comment 6

[Justin Lebar (not reading bugmail)]

Comment on attachment 586873 [details] [diff] [review]
patch to ignore computedSize

>-  var reportersByProcess = getReportersByProcess();
>+  var reportersByProcess = getReportersByProcess(mgr);

OOC, why did you make this change?

>+NS_IMETHODIMP
>+nsMemoryReporterManager::GetHasMozMallocUsableSize(bool *aHas)
>+{
>+    void *p = malloc(16);

It's pedantic, but we should probably check the return value of malloc here.

Comment 7

[Nicholas Nethercote [inactive]]

> >-  var reportersByProcess = getReportersByProcess();
> >+  var reportersByProcess = getReportersByProcess(mgr);
> 
> OOC, why did you make this change?

|mgr| was previously only used in getReportersByProcess(), but it's now also used in update().


> >+NS_IMETHODIMP
> >+nsMemoryReporterManager::GetHasMozMallocUsableSize(bool *aHas)
> >+{
> >+    void *p = malloc(16);
> 
> It's pedantic, but we should probably check the return value of malloc here.

moz_malloc_usable_size(NULL) returns 0 so we'll end up assigning |false| to |aHas|, which is a reasonable thing to do  (i.e. "don't know" becomes "no").  I'll add a comment.

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: patch to ignore computedSize, v2

The old patch had a problem with test_aboutmemory.xul in --enable-trace-malloc builds (which includes debug Linux and Windows builds are on tinderbox).  Such builds don't use jemalloc and so "heap-allocated" is unknown, so we get one of the warnings, which causes the test to fail.

My best idea for avoiding this was to remove the hasKnownHeapAllocated property from nsMemoryReporterManager, and instead just see in aboutMemory.js if we succeeded in getting a "heap-allocated".  This must be done on a per-process basis.
The nice thing about this is that in test_aboutmemory.xul we fake three processes, and in one of them we have an unknown "heap-allocated", so one of the new warning messages gets tested.

Also in this patch, I changed GetHasMozMallocUsableSize to return NS_ERROR_OUT_OF_MEMORY in the malloc-failure case.  This causes about:memory to throw an exception, which seems a reasonable thing to do.  If a 16-byte allocation fails the browser's probably about to die anyway.

Comment 9

[Justin Lebar (not reading bugmail)]

> My best idea for avoiding this was to remove the hasKnownHeapAllocated property
> from nsMemoryReporterManager, and instead just see in aboutMemory.js if we
> succeeded in getting a "heap-allocated".  This must be done on a per-process
> basis.

You could also just style the error div as not-copyable, as the bottom divs are styled.

Comment 10

[Justin Lebar (not reading bugmail)]

Let me know if you'd prefer to do that or take the approach in this patch.  I don't have a preference.

Comment 11

[Nicholas Nethercote [inactive]]

> You could also just style the error div as not-copyable, as the bottom divs
> are styled.

I thought about that but I like this approach better:

- it's testable

- it's one less thing in nsIMemoryReporterManager

- If someone cut-and-pastes an about:memory with that warning into a bug report, we won't have actively suppressed the warning that tells us it's bogus :)

Comment 12

[Justin Lebar (not reading bugmail)]

> @@ -558,16 +564,18 @@ function fixUpExplicitTree(aT, aReporter
>    heapUnclassifiedT._description =
>        kindToString(KIND_HEAP) +
>       "Memory not classified by a more specific reporter. This includes " +
>       "slop bytes due to internal fragmentation in the heap allocator "
>       "(caused when the allocator rounds up request sizes).";
 
>   aT._kids.push(heapUnclassifiedT);
>   aT._amount += heapUnclassifiedT._amount;
>+
>+  return hasKnownHeapAllocated;
> }

Please add a comment to fixUpExplicitTree indicating what it returns.

>+  // Generate any warnings about inaccuracies due to platform limitations.
>+  // The newlines give nice spacing if we cut+paste into a text buffer.
>+  var warningText = "";
>+  if (!hasKnownHeapAllocated && !aHasMozMallocUsableSize) {
>+  ...

Kind of ugly to inline these long strings in here.  Can you pull these out as constants or a function?

Hm.  This change means that the about:memory test will fail on any platform/configuration where we don't have malloc_usable_size.  Is that really what you want?

Comment 13

[Nicholas Nethercote [inactive]]

> Hm.  This change means that the about:memory test will fail on any
> platform/configuration where we don't have malloc_usable_size.  Is that
> really what you want?

I don't think it matters.  We won't be running Sparc tinderboxes any time soon.

Comment 14

[Nicholas Nethercote [inactive]]

Attachment: patch to ignore computedSize, v3

New patch that addresses comment 12.  I put the warnings in a function primarily because I didn't want to have to choose names for them.  The only reasonable names I could think of were far too long.

Comment 15

[Justin Lebar (not reading bugmail)]

(In reply to Nicholas Nethercote [:njn] from comment #13)
> > Hm.  This change means that the about:memory test will fail on any
> > platform/configuration where we don't have malloc_usable_size.  Is that
> > really what you want?
> 
> I don't think it matters.  We won't be running Sparc tinderboxes any time
> soon.

Does valgrind expose malloc_usable_size?

Comment 16

[Nicholas Nethercote [inactive]]

> Does valgrind expose malloc_usable_size?

On Linux, yes, on Mac it exposes/handles malloc_size.  So that will be fine.

Comment 17

[Justin Lebar (not reading bugmail)]

Comment on attachment 588365 [details] [diff] [review]
patch to ignore computedSize, v3

I'm uncomfortable with making a hard dependency on malloc_usable_size in our tests, and the fact that this dependency comes out only as a side-effect of how this test is written doesn't help.

But r=me if you'll fix the test in the event that it burns sometime in the future when we run tests in a configuration without malloc_usable_size.

Comment 18

[Nicholas Nethercote [inactive]]

> I'm uncomfortable with making a hard dependency on malloc_usable_size in our
> tests, and the fact that this dependency comes out only as a side-effect of
> how this test is written doesn't help.
> 
> But r=me if you'll fix the test in the event that it burns sometime in the
> future when we run tests in a configuration without malloc_usable_size.

If necessary, it'll be easy to modify the expected output string according to the value of mgr.hasMozMallocUsableSize.

I'm more worried about the loss of functionality on some configurations than I am about the tests!

Comment 19

[Justin Lebar (not reading bugmail)]

> I'm more worried about the loss of functionality on some configurations than I am about 
> the tests!

Well, we have some test-only platforms (e.g. valgrind, trace-malloc) where I'm totally OK with not having malloc_usable_size.  Incidentally, we have malloc_usable_size on all these test platforms, so it's not an issue.  So we can worry about this later when the test turns red.  :)

Comment 20

[Nicholas Nethercote [inactive]]

Part 2:
https://hg.mozilla.org/integration/mozilla-inbound/rev/87ad0c8ae369

There's more to come, please leave this bug open.

Comment 21

[Nicholas Nethercote [inactive]]

Ginn Chen mentioned today that Solaris builds can use --enable-malloc, which is great!  Any build that can use --enable-malloc won't be affected by the removal of computedSize.

Comment 22

[Justin Lebar (not reading bugmail)]

I think you mean --enable-jemalloc?

Comment 23

[Nicholas Nethercote [inactive]]

Yes!

Comment 24

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/87ad0c8ae369

Comment 25

[Nicholas Nethercote [inactive]]

Attachment: remove computedSize, non-JS parts

This patch removes all traces of computedSize outside the JS engine.  The most important changes are in:  mozalloc.{h,cpp}, nsIMemoryReporter.idl, nscore.h.  Everything else is just argument removal.

Comment 26

[Nicholas Nethercote [inactive]]

Attachment: remove computedSize, JS parts

This just removes the computedSize arguments, all very boring.

Comment 27

[Justin Lebar (not reading bugmail)]

> -size_t moz_malloc_size_of(const void *ptr, size_t computedSize)
> +size_t moz_malloc_size_of(const void *ptr)
>  {
> -    size_t usable = moz_malloc_usable_size((void *)ptr);
> -    return usable ? usable : computedSize;
> +    return moz_malloc_usable_size((void *)ptr);
>  }

Why not completely remove moz_malloc_size_of?  Is it just for the const cast?  If so, can we just make moz_malloc_usable_size accept a const pointer?  "Explicit is better than implicit" and all that.

>  * Functions generated via this macro should be used by all traversal-based
>- * memory reporters.
>- *
>- * - On platforms where moz_malloc_usable_size(ptr) returns 0 it just returns
>- *   |computedSize|.  This happens on platforms where malloc_usable_size() or
>- *   equivalent isn't available.
>- *
>- * - Otherwise, it returns |moz_malloc_usable_size(ptr)|.
>+ * memory reporters.  It returns |moz_malloc_size_of(ptr)|;  this will
>+ * always be zero on some obscure platforms.

Subject-verb agreement: "Functions" doesn't match "It".

Comment 28

[Nicholas Nethercote [inactive]]

(In reply to Justin Lebar [:jlebar] from comment #27)
> > -size_t moz_malloc_size_of(const void *ptr, size_t computedSize)
> > +size_t moz_malloc_size_of(const void *ptr)
> >  {
> > -    size_t usable = moz_malloc_usable_size((void *)ptr);
> > -    return usable ? usable : computedSize;
> > +    return moz_malloc_usable_size((void *)ptr);
> >  }
> 
> Why not completely remove moz_malloc_size_of?  Is it just for the const
> cast?  If so, can we just make moz_malloc_usable_size accept a const
> pointer?  "Explicit is better than implicit" and all that.

It's mainly for the const cast.  I'm reluctant to add const to moz_malloc_usable_size because those moz_ functions mirror the libc ones.  And having moz_malloc_size_of is a nice parallel to the ones like JsMallocSizeOf.

Comment 29

[Nicholas Nethercote [inactive]]

> >  * Functions generated via this macro should be used by all traversal-based
> >+ * memory reporters.  It returns |moz_malloc_size_of(ptr)|;  this will
> >+ * always be zero on some obscure platforms.
> 
> Subject-verb agreement: "Functions" doesn't match "It".

But it does match "this macro"! :)

I'll reword it.

Comment 30

[Justin Lebar (not reading bugmail)]

> I'm reluctant to add const to moz_malloc_usable_size because those moz_ functions mirror the libc 
> ones.

You could add an overload for const void*.

> And having moz_malloc_size_of is a nice parallel to the ones like JsMallocSizeOf.

I might rename these too.  :)  I guess a JsMallocSizeOf is not necessarily malloc_usable_size but instead a DMD-compatible wrapper around it, so maybe it deserves a different name.  But note that from the user's perspective, it's basically the same as calling malloc_usable_size.

Comment 31

[Nicholas Nethercote [inactive]]

> I might rename these too.  :)  I guess a JsMallocSizeOf is not necessarily
> malloc_usable_size but instead a DMD-compatible wrapper around it, so maybe
> it deserves a different name.  But note that from the user's perspective,
> it's basically the same as calling malloc_usable_size.

I don't really want to rename nsMallocSizeOf and JSMallocSizeOf -- those names are *common*.  I can live with renaming moz_malloc_size_of as (an overloading of) moz_malloc_usable_size.

Comment 32

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/01d0bab1636e

Comment 33

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/01d0bab1636e

Comment 34

[Justin Lebar (not reading bugmail)]

(In reply to Nicholas Nethercote [:njn] from comment #31)
> I don't really want to rename nsMallocSizeOf and JSMallocSizeOf -- those
> names are *common*.  I can live with renaming moz_malloc_size_of as (an
> overloading of) moz_malloc_usable_size.

But it looks like you didn't do this?

Comment 35

[Nicholas Nethercote [inactive]]

> > I can live with renaming moz_malloc_size_of as (an
> > overloading of) moz_malloc_usable_size.
> 
> But it looks like you didn't do this?

After thinking more I decided I liked keeping moz_malloc_size_of for the naming consistency.  I figured this was ok since you didn't appear to have a strong preference -- did I misinterpret?

Comment 36

[Justin Lebar (not reading bugmail)]

No, so long as it was intentional.  I just went on something of a wild goose chase this morning looking for it, is all.  :)