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

RESOLVED FIXED in mozilla35

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jimb, Assigned: jimb)

Tracking

unspecified
mozilla35
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
Blocks: 1011300
(Assignee)

Comment 1

4 years ago
Created attachment 8484649 [details] [diff] [review]
Allow implicit construction of JS::ubi::Node from JS::HandleValue.
Attachment #8484649 - Flags: review?(terrence)
(Assignee)

Comment 2

4 years ago
Created attachment 8484651 [details] [diff] [review]
Amend JS::ubi::Node::size and its implementations to expect a mozilla::MallocSizeOf function.
Attachment #8484651 - Flags: review?(terrence)
(Assignee)

Comment 3

4 years ago
Created attachment 8484652 [details] [diff] [review]
Let embeddings tell Debugger how to find the size of a malloc'd block of memory.
Attachment #8484652 - Flags: review?(terrence)
Attachment #8484649 - Flags: review?(terrence) → review+
Attachment #8484651 - Flags: review?(terrence) → review+
Attachment #8484652 - Flags: review?(terrence) → review+
(Assignee)

Updated

4 years ago
Flags: in-testsuite-
Target Milestone: --- → mozilla35

Updated

4 years ago
Depends on: 1063726
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);
Ah, I just saw bug 1063697, which mentions _msize().
(Assignee)

Comment 8

4 years ago
(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.
Flags: needinfo?(jimb)
For Firefox, you need to use mozmemory.h and malloc_usable_size for jemalloc-enabled builds and replace-malloc-enabled builds.
(Assignee)

Comment 10

4 years ago
(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.
(Assignee)

Comment 11

4 years ago
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.
Flags: needinfo?(mh+mozilla)
(Assignee)

Comment 12

4 years ago
Here's a try push with lots of disunification:
https://tbpl.mozilla.org/?tree=Try&rev=8b1340a4c337
> 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.
(Assignee)

Comment 14

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

Comment 15

4 years ago
(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.
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

4 years ago
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
You need to log in before you can comment on or make changes to this bug.