1063247 - [jsdbg2] Debugger.Memory needs a mallocSizeOf function from the embedding to produce accurate byte sizes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

Debugger.Memory needs the embedding to provide it with a function that can find the size of a malloc'd block, given the address of the block.

Various Debugger.Memory functions want to return the byte sizes of various kinds of objects. Ideally, these sizes should include whatever internal padding malloc adds to the requested block sizes. about:memory accomplishes this by having all reporting functions require a pointer to a function that takes a 'void *' and returns the size of the block it points to, presumably by consulting malloc's internal data structures. (For example, the GNU C library provides a function, malloc_usable_size, that does this.)

The code that implements the about:memory reports chooses the right function for the platform and passes it in at measurement time. But functions like Debugger.Memory.prototype.takeCensus need to be ready for use by JavaScript, which isn't in a position to supply pointers to random platform-specific C functions.

So: the embedding should be able to set a malloc block sizing function on the JSRuntime, and Debugger should use that when it needs one.

Comment 1

[Jim Blandy :jimb]

Attachment: Allow implicit construction of JS::ubi::Node from JS::HandleValue.

Comment 2

[Jim Blandy :jimb]

Attachment: Amend JS::ubi::Node::size and its implementations to expect a mozilla::MallocSizeOf function.

Comment 3

[Jim Blandy :jimb]

Attachment: Let embeddings tell Debugger how to find the size of a malloc'd block of memory.

Comment 4

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd80b2984fe2
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7ffa64956c6
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6033944c4c1

Comment 5

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

I backed this and bug 1063233 out for non-unified Windows build bustage:
https://hg.mozilla.org/integration/mozilla-inbound/rev/04be894027e6
https://tbpl.mozilla.org/php/getParsedLog.php?id=47507112&tree=Mozilla-Inbound

Comment 6

[Nicholas Nethercote [inactive]]

jimb, the third patch as landed has much more stuff in it than the reviewed version, which seems suspicious.

Also, before relanding, you should add this extra case to ShellMallocSizeOf (which I cribbed from moz_malloc_usable_size):

> #elif defined(XP_WIN)
>     return _msize(ptr);

Comment 7

[Nicholas Nethercote [inactive]]

Ah, I just saw bug 1063697, which mentions _msize().

Comment 8

[Jim Blandy :jimb]

(In reply to Nicholas Nethercote [:njn] from comment #7)
> Ah, I just saw bug 1063697, which mentions _msize().

That whole thing was quite a trainwreck.

Comment 9

[Mike Hommey [:glandium]]

For Firefox, you need to use mozmemory.h and malloc_usable_size for jemalloc-enabled builds and replace-malloc-enabled builds.

Comment 10

[Jim Blandy :jimb]

(In reply to Mike Hommey [:glandium] (out from Sep 6 to Sep 22) from comment #9)
> For Firefox, you need to use mozmemory.h and malloc_usable_size for
> jemalloc-enabled builds and replace-malloc-enabled builds.

I can't use that header from js/src/shell/js.cpp, though.

Comment 11

[Jim Blandy :jimb]

Mike, I need a way to get malloc block sizes in the JS shell, js/src/shell/js.cpp. This can't use headers from memory/build, as far as I know.

Comment 12

[Jim Blandy :jimb]

Here's a try push with lots of disunification:
https://tbpl.mozilla.org/?tree=Try&rev=8b1340a4c337

Comment 13

[Nicholas Nethercote [inactive]]

> Mike, I need a way to get malloc block sizes in the JS shell,
> js/src/shell/js.cpp. This can't use headers from memory/build, as far as I
> know.

Just copying the code from moz_malloc_usable_size into ShellMallocSizeOf seems reasonable to me.

Comment 14

[Jim Blandy :jimb]

That one looks good, so here's a try push with no disunification:
https://tbpl.mozilla.org/?tree=Try&rev=d0c954be7700

Comment 15

[Jim Blandy :jimb]

(In reply to Nicholas Nethercote [:njn] from comment #13)
> > Mike, I need a way to get malloc block sizes in the JS shell,
> > js/src/shell/js.cpp. This can't use headers from memory/build, as far as I
> > know.
> 
> Just copying the code from moz_malloc_usable_size into ShellMallocSizeOf
> seems reasonable to me.

Yeah, that's what I've done --- with the concomitant configure.in grunge.

Comment 16

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b1cbc5079834
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef6d81ac0bba
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed38f85902f7

Comment 17

[Nicholas Nethercote [inactive]]

Should the condition in ShellMallocSizeOf be this:

> #elif defined(HAVE_MALLOC_USABLE_SIZE) || defined(MOZ_MEMORY)

instead of this:

> #elif defined(HAVE_MALLOC_USABLE_SIZE)

jemalloc (i.e. MOZ_MEMORY) seems to be enabled for shell builds so it would be a shame to miss some cases. But maybe it's ok; I'm just speculating.

Comment 18

[Jan Beich]

jemalloc isn't linked into shell. And according to bug 704045 mozalloc relies on specific behavior of mozjemalloc that may not be true for jemalloc3.

  In file included from js/src/shell/Unified_cpp_js_src_shell0.cpp:15:
  js/src/shell/js.cpp:5870:12: error: use of undeclared identifier 'malloc_usable_size'; did you mean 'malloc_good_size'?
      return malloc_usable_size(ptr);
             ^~~~~~~~~~~~~~~~~~
             malloc_good_size
  /usr/include/malloc/malloc.h:101:15: note: 'malloc_good_size' declared here
  extern size_t malloc_good_size(size_t size);
                ^
  In file included from js/src/shell/Unified_cpp_js_src_shell0.cpp:15:
  js/src/shell/js.cpp:5870:31: error: cannot initialize a parameter of type 'size_t' (aka 'unsigned long') with an lvalue of type 'void *'
      return malloc_usable_size(ptr);
                                ^~~
  /usr/include/malloc/malloc.h:101:39: note: passing argument to parameter 'size' here
  extern size_t malloc_good_size(size_t size);
                                        ^
  2 errors generated.

or

  js/src/shell/tmpHMv2CG.list:
      Unified_cpp_js_src_shell0.o
      ../editline/Unified_c_js_src_editline0.o

  ld: warning: could not create compact unwind for _ffi_call_unix64: does not use RBP or RSP based frame
  Undefined symbols for architecture x86_64:
    "_malloc_usable_size", referenced from:
        ShellMallocSizeOf(void const*) in Unified_cpp_js_src_shell0.o
  ld: symbol(s) not found for architecture x86_64
  clang: error: linker command failed with exit code 1 (use -v to see invocation)
  make[5]: *** [js] Error 1
  make[4]: *** [js/src/shell/target] Error 2

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=828c171af981
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c67fea8f95eb

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b1cbc5079834
https://hg.mozilla.org/mozilla-central/rev/ef6d81ac0bba
https://hg.mozilla.org/mozilla-central/rev/ed38f85902f7