676732 - Measure and/or avoid slop in important JS memory reporters

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

The JS reporters should used malloc_usable_size/malloc_size where appropriate.  (Actually, all memory reporters should, but JS is a good place to start.)  Bug 675136 comment 14 showed that "scripts" under-reports the actual memory used by about 20%, for example.

Comment 1

[Paul Biggar]

We don't use mozalloc in JS, so you'll need to copy/tweak this bit:

size_t
moz_malloc_usable_size(void *ptr)
{
    if (!ptr)
        return 0;

#if defined(MOZ_MEMORY)
    return malloc_usable_size(ptr);
#elif defined(XP_MACOSX)
    return malloc_size(ptr);
#elif defined(XP_WIN)
    return _msize(ptr);
#else
    return 0;
#endif
}


I'm working on abstracting this in mozalloc so that we always provide a malloc_usable_size symbol, but it will be a while before that makes it near the JS shell at least.

Comment 2

[Nicholas Nethercote [inactive]]

Thanks for the tip.  I foresee a JS_malloc_usable_size in my near future.

Comment 3

[Nicholas Nethercote [inactive]]

Oh, but using MOZ_MEMORY in the JS engine doesn't appear to be kosher.  That messes with my plans significantly.  Hmm.

Comment 4

[Nicholas Nethercote [inactive]]

cjones, do you have any idea about how to do this?

Comment 5

[Nicholas Nethercote [inactive]]

Here's sample output from DMD that shows why this is important, BTW:

==23402== Unreported: 1,950,496 (cumulative: 11,740,992) bytes in 21,897 heap block(s) in record 4 of 11626:
==23402==  Requested bytes unreported: 0 / 9,052,832
==23402==  Slop      bytes unreported: 1,950,496 / 1,950,496
==23402==    at 0x4029F90: malloc (vg_replace_malloc.c:235)
==23402==    by 0x6E6692A: js_malloc (jsutil.h:248)
==23402==    by 0x6E66A02: JSRuntime::malloc_(unsigned long, JSContext*) (jscntxt.h:728)
==23402==    by 0x6E66B60: JSContext::malloc_(unsigned long) (jscntxt.h:1261)
==23402==    by 0x7D0E2C8: JSScript::NewScript(JSContext*, unsigned int, unsigned int, unsigned int, unsigned int, unsigned int, unsigned i
nt, unsigned int, unsigned int, unsigned int, unsigned short, unsigned short, JSVersion) (jsscript.cpp:963)
==23402==    by 0x7D0EB19: JSScript::NewScriptFromCG(JSContext*, JSCodeGenerator*) (jsscript.cpp:1145)


9MB are reported, but there's almost 2MB of slop that isn't reported.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Why not just use moz_malloc_usable_size() directly?  mozalloc is "lower level" than js/src, it's fine for js/src to use it.  Would want a way to turn that off for embedders though.

Comment 7

[Nicholas Nethercote [inactive]]

> Why not just use moz_malloc_usable_size() directly?

I can't work out how to do it.  I #included "mozilla/mozalloc.h" in jstracer.cpp and got this in a browser build:

  In file included from /home/njn/moz/mi2/js/src/jstracer.cpp:2511:0:
  ./../../dist/include/mozilla/mozalloc.h:53:26: fatal error: xpcom-config.h: No such file or directory

More generally, #including Gecko headers in the JS engine *really* doesn't feel right.  For example, I get the following in a shell build, which is hardly surprising:

  /home/njn/moz/mi2/js/src/jstracer.cpp:2511:30: fatal error: mozilla/mozalloc.h: No such file or directory

My plan B is to do all the moz_malloc_usable_size calls effectively from within xpconnect, either by (a) exposing the relevant heap blocks from the JS engine, or (b) passing in a pointer to moz_malloc_usable_size to various places.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Nicholas Nethercote [:njn] from comment #7)
> > Why not just use moz_malloc_usable_size() directly?
> 
> I can't work out how to do it.  I #included "mozilla/mozalloc.h" in
> jstracer.cpp and got this in a browser build:
> 
>   In file included from /home/njn/moz/mi2/js/src/jstracer.cpp:2511:0:
>   ./../../dist/include/mozilla/mozalloc.h:53:26: fatal error:
> xpcom-config.h: No such file or directory
> 

mozalloc depends on a few macros written by configure, like NS_ATTR_MALLOC.  These are checked by js/src/configure.in too, so the expedient fix here is to make mozalloc use the js config.h (not sure where that lives).

Generally, this stuff needs to live in mfbt or a central location, IMHO.

> More generally, #including Gecko headers in the JS engine *really* doesn't
> feel right. 

mozalloc isn't gecko, it's lower level.  It has the same depedencies as NSPR (i.e. none).  In fact, in android builds, NSPR depends on mozalloc (as does SM).

> For example, I get the following in a shell build, which is
> hardly surprising:
> 
>   /home/njn/moz/mi2/js/src/jstracer.cpp:2511:30: fatal error:
> mozilla/mozalloc.h: No such file or directory
> 

It's surprising to me! ;)  Maybe shell builds aren't jemalloc enabled, so memory/ isn't being built?  If so, that's generally bad (would skew performance measurements in the shell).

Comment 9

[Nicholas Nethercote [inactive]]

> Maybe shell builds aren't jemalloc enabled

They're not, I just checked.

Comment 10

[Paul Biggar]

(In reply to Nicholas Nethercote [:njn] from comment #9)
> > Maybe shell builds aren't jemalloc enabled
> 
> They're not, I just checked.

Bug 580409.

Comment 11

[Paul Biggar]

I took a brief look at how to call moz_malloc_usable_size.

There are two ways that jemalloc gets used. For Gecko and similar, mozalloc is #included, which replaces malloc with moz_malloc, which calles jemalloc. For parts of Firefox which don't have this #include, jemalloc does a system specific override of some memory bits.

In the shell, neither works, because we don't build memory/, and don't #include mozalloc.h.

We need both to use moz_malloc_usable_memory, but there's a few issues to overcome for landing:

 - we don't want to override new in the engine
 - we don't want to accidentally pollute embedders code by defining malloc to moz_malloc in something like jsutil.h
 - we can't rely on xpcom headers
 - historically, there has been some objection to how we pull the memory/ directory into js/src.

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Paul Biggar from comment #11)
> I took a brief look at how to call moz_malloc_usable_size.
> 
> There are two ways that jemalloc gets used. For Gecko and similar, mozalloc
> is #included, which replaces malloc with moz_malloc, which calles jemalloc.

moz_malloc() just calls malloc(), which is defined to the jemalloc impl on android and apparently OS X now, and the rest of the time calls the normal system malloc (which is jemalloc on "normal" linux and windows).

> For parts of Firefox which don't have this #include, jemalloc does a system
> specific override of some memory bits.
> 

That may be true for mac, but otherwise we either do the system-specific override, or (on android) we play linker tricks to make malloc et al. symbols resolve to jemalloc's.

> We need both to use moz_malloc_usable_memory, but there's a few issues to
> overcome for landing:
> 
>  - we don't want to override new in the engine

SM uses jsnew()/jsdelete() or something, right?  The default operator new throws on OOM, which nobody wants, so I assume SM doesn't use it (IIRC njn added a check for that).  Including mozalloc.h as it exists today would mean that njn's check would break, because vanilla operator new would boil away to a call to moz_xmalloc.  We could split moz_malloc_usable_memory into its own header, which we do for mozalloc_abort() already.  We should probably be using mozalloc_abort() or whatever in js/src too; it stinks maintaining separate copies of the abort-and-trigger-breakpad code.  Candidate for mfbt I guess.

>  - we don't want to accidentally pollute embedders code by defining malloc
> to moz_malloc in something like jsutil.h

That doesn't happen unless the mozalloc_macro_wrappers.h are included, which has to be done manually.

>  - historically, there has been some objection to how we pull the memory/
> directory into js/src.

js/src depends on top-level mfbt/ already.  We accomplish that today by building mfbt/ objects into libmozjs.  We could make building memory/ be js/src's responsibility, whether shell or browser build.

Comment 13

[Nicholas Nethercote [inactive]]

Attachment: patch

This patch uses moz_malloc_usable_size for various JS memory reporters in
order to measure slop bytes caused by the heap allocator rounding up request
sizes.  Details:

- The memory reporters affected: mjit-data, script-data, object-slots,
  property-tables, runtime/runtime-object.  I modified their descriptions to
  mention that slop bytes are included in them.

- I avoided the difficulties in calling malloc_usable_size (and equivalents)
  from within the JS engine by passing in a function pointer to
  moz_malloc_usable_size where necessary.  This seemed like the least worst
  solution.

- moz_malloc_usable_size will return 0 on platforms where it can't be
  implemented, and in that case we fall back to using the existing
  calculations, which don't account for slop.  (Passing in NULL for the
  function pointer also causes us to fall-back;  I do this in one place in
  the shell.)

- These changes account for roughly 3MB of what is currently dark matter
  when starting the browser and loading Gmail.

- I also changed the sizes of the reserve spaces used in TM's three
  VMAllocators.  These new sizes are all sizes that don't have any slop.
  Making them smaller seems unlikely to cause problems in practice, because
  they were very conservative (also, I accidentally switched the "test" and
  "trace" allocator reserve sizes when I introduced them, a botch that this
  patch fixes).  And it reduces TM's memory usage slightly, as well as
  making the tjit-data/allocators-reserve reporters more accurate.

Comment 14

[Igor Bukanov]

(In reply to Nicholas Nethercote [:njn] from comment #13)
> - I avoided the difficulties in calling malloc_usable_size (and equivalents)
>   from within the JS engine by passing in a function pointer to
>   moz_malloc_usable_size where necessary.  This seemed like the least worst
>   solution.

Why not to add a static pointer that will be set during initialization to moz_malloc_usable_size on platforms where we know that it works?

Comment 15

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/7fb15a645955

Comment 16

[Nicholas Nethercote [inactive]]

A follow-up to fix Windows bustage:

http://hg.mozilla.org/integration/mozilla-inbound/rev/3fdf87297e66

Comment 17

[Ed Morley [:emorley]]

Not sure if this is noise or not, but supposedly improved dromaeo css :-)
https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/fqr58Zlx4Oc

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/7fb15a645955

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/3fdf87297e66

Comment 20

[Nicholas Nethercote [inactive]]

(In reply to Ed Morley [:edmorley] from comment #17)
> Not sure if this is noise or not, but supposedly improved dromaeo css :-)
> https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/
> fqr58Zlx4Oc

I'm 99.99% certain that's noise :)