532486 - Add some JS memory reporters

Status: RESOLVED DUPLICATE of bug 571249

(Core :: JavaScript Engine, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: add mem reporters in js

This patch adds two memory reporters for JS -- one for GC chunks in use, and one for memory in use by CodeAlloc objects (during assembly).  The latter should generally be 0 unless you catch it during compilation, but I'm more worried about noticing leaks there.

This also changes NewGCChunk to allocate using valloc() if jemalloc is in use, so that GC chunks can be tracked along with the rest of our memory usage.  valloc() internally turns into a pagesize-aligned malloc, which is why I'm using it instead of posix_memalign/memalign (because I don't know what the page size is here).

To get the CodeAlloc size, I had to move the codealloc allocators into jstracer.cpp from avmplus.cpp; the Allocator alloc functions are already in jstracer.cpp, so I think this makes sense.

Comment 1

[Brendan Eich [:brendan]]

Comment on attachment 415693 [details] [diff] [review]
add mem reporters in js

Nits like no gFoo naming for statics and function left brace on its own line in col. 1 aside, I am gonna duck this and ask graydon and igor to look. Graydon knows the NJ allocator code well, Igor the JS GC. I've got a hot orangeness fix to babysit and I'd rather these guys have a look on account of they have "dibs".

/be

Comment 2

[Igor Bukanov]

Comment on attachment 415693 [details] [diff] [review]
add mem reporters in js

>diff --git a/js/src/jsgc.cpp b/js/src/jsgc.cpp

>+static int32 gGCChunkCount = 0;

The number of GC chunks can be calculated at run-time via enumerationg over allocated GC arenas and chunks so there is no need for a static variable here.

>+
> static jsuword
> NewGCChunk(void)
> {
>     void *p;
> 
>-#if defined(XP_WIN)
>+    JS_ATOMIC_INCREMENT(&gGCChunkCount);
>+
>+#if defined(MOZ_MEMORY)
>+    // valloc() allocates page-aligned chunks; jemalloc already knows
>+    // the page size, so we just use this instead of figuring it out
>+    // here for jemalloc.
>+    p = valloc(GC_ARENAS_PER_CHUNK << GC_ARENA_SHIFT);

What is the extra overhead of valloc here? In particular, does jemalloc allocate 64K chunks from its own pool?

>+static int32 gCodeAllocChunks = 0;

Cannot we get this from enumerating the tracer memory?

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #2)
> (From update of attachment 415693 [details] [diff] [review])
> >diff --git a/js/src/jsgc.cpp b/js/src/jsgc.cpp
> 
> >+static int32 gGCChunkCount = 0;
> 
> The number of GC chunks can be calculated at run-time via enumerationg over
> allocated GC arenas and chunks so there is no need for a static variable here.

Assuming there are no leaks there, that's true (well, assuming you have a JSRuntime to start from, but we have that in xpcjs*).  But having this variable to compare to the value obtained from enumerating seems useful.

> > static jsuword
> > NewGCChunk(void)
> > {
> >     void *p;
> > 
> >-#if defined(XP_WIN)
> >+    JS_ATOMIC_INCREMENT(&gGCChunkCount);
> >+
> >+#if defined(MOZ_MEMORY)
> >+    // valloc() allocates page-aligned chunks; jemalloc already knows
> >+    // the page size, so we just use this instead of figuring it out
> >+    // here for jemalloc.
> >+    p = valloc(GC_ARENAS_PER_CHUNK << GC_ARENA_SHIFT);
> 
> What is the extra overhead of valloc here? In particular, does jemalloc
> allocate 64K chunks from its own pool?

jemalloc will allocate 64K chunks from its own arena; there is some overhead, but it should be negligible.  See http://hg.mozilla.org/mozilla-central/file/tip/memory/jemalloc/jemalloc.c#l47

> >+static int32 gCodeAllocChunks = 0;
> 
> Cannot we get this from enumerating the tracer memory?

I don't think so; these are only used while assembling a trace, that is per-Assembler.  I'm also worried about cases where we might leak one of these.

Comment 4

[Igor Bukanov]

(In reply to comment #3)
> Assuming there are no leaks there, that's true (well, assuming you have a
> JSRuntime to start from, but we have that in xpcjs*).

If there is a leak, how would one know about it without asking the engine about its view on the allocated memory? I.e. one still needs to enumerate the chunks and compare it with the counter.

>  But having this variable
> to compare to the value obtained from enumerating seems useful.

Right, but then that value should be protected with a special ifdef so it will be available only only when one wants to look for leaks.

Comment 5

[Robert Sayre]

What needs to change with this patch?

Comment 6

[Igor Bukanov]

Given the recent changes from the bug 553812 this patch needs an update for the GC chunk part. But a better way to proceed is to land the changes for the bug 557538 (I have a patch there and will submit it for review shortly) and then implement all the necessary GC chunk accounting outside the engine. Another possibility is to use jemalloc chunk allocation directly in the GC engine (again using the changes from the bug 553812). Then the accounting would happen automatically.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: add js gc & codealloc reporters

This patch builds on top of Igor's work in bug 557538 and adds instrumentation in xpcjsruntime to use jemalloc for gc chunks, and to report how much memory is in use by gc chunks.

It also adds a similar mechanism to that exposed by 557538 for overriding the allocator functions used by nanojit's CodeAlloc.  This part's a little gross, especially the VMPI setPageProtection function exposure, but I'm not sure how better to do this without moving a lot of code.  This seems to work well as-is.

Comment 8

[Igor Bukanov]

(In reply to comment #7)
> Created an attachment (id=443386) [details]
> add js gc & codealloc reporters

It seems this results in proliferation of various callbacks that would setup the engine for what is effectively a compile-time option. So what about creating a special accounting/allocation library that the JS engine can link against?

That aside the patch is problematic since it uses static variables to account for nanojit allocations. Those can happen on multiple threads without any locking and the patch does not protect against that.


> 
> This patch builds on top of Igor's work in bug 557538 and adds instrumentation
> in xpcjsruntime to use jemalloc for gc chunks, and to report how much memory is
> in use by gc chunks.
> 
> It also adds a similar mechanism to that exposed by 557538 for overriding the
> allocator functions used by nanojit's CodeAlloc.  This part's a little gross,
> especially the VMPI setPageProtection function exposure, but I'm not sure how
> better to do this without moving a lot of code.  This seems to work well as-is.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Ah, I didn't think of the nj allocations, that's fixable.  The problem is one of linkage -- js doesn't link to the places where we'd want the tracking to happen, and so we'd have to introduce another library as you say.  Maybe a better option is to just have the engine to the types of accounting we're interested in (e.g. per-runtime gc, per-context (?) codealloc) and add methods to just ask the runtime for the current stats.

Comment 10

[Igor Bukanov]

(In reply to comment #9)
> Ah, I didn't think of the nj allocations, that's fixable.  The problem is one
> of linkage -- js doesn't link to the places where we'd want the tracking to
> happen, and so we'd have to introduce another library as you say.

I guess on Linux one can use weak symbols so the js dynamic library can be compiled without explicitly linking against them. But that is not available on Windows or Mac. So the question is if it is really difficult to create a special library?

>  Maybe a
> better option is to just have the engine to the types of accounting we're
> interested in (e.g. per-runtime gc, per-context (?) codealloc) and add methods
> to just ask the runtime for the current stats.

For the GC chunks counters are not neceesary. With recent changes all chunks are stored in a vector so counting them is trivial and we can just add an API like JS_GetTotalGCHeapSize(). For codealloc I am not sure if we can enumerate them a simple way so some extra counting may be necessary indeed.

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: add js alloc tracker, v0

So, here's a patch that tracks js_malloc/etc. as well as string allocation/deallocation (separately).  Unfortunately, there's a nonzero perf impact here, largely (I'd assume) the cost of calling malloc_size.  I don't have a jemalloc build here handy, so this is on win32/msvc10 using the built-in _msize.  The hit on a shell sunspider run seems to be around 1%-2%.

sayrer suggested an alternate approach, which is doing a mark and asking strings/arrays/etc. how big their data stores are.  That could work, though it would miss any leaks, and it wouldn't track "other" callers of js_malloc/etc.  Another approach might be to avoid msize entirely and just overallocate and shove the allocation size in the first 4 bytes of each allocation, though that might overallocate enough to cause us to bump up to the next jemalloc bin etc.

I'll try this with jemalloc as well as on OSX, but I'm posting the patch for feedback here even before I get those numbers.

Comment 12

[Jason Orendorff [:jorendorff]]

Comment on attachment 448125 [details] [diff] [review]
add js alloc tracker, v0

I'm not sure what kind of feedback you're looking for, though. Spending extra time in the allocation path seems like a non-starter. Sayrer's idea sounds like it could work.

Comment 13

[Nicholas Nethercote [inactive]]

I'm going to mark this bug as a duplicate of bug 571249.  The patch in this bug doesn't seem acceptable, for a couple of reasons:

- It slows down allocation paths (as mentioned above).

- I'm trying to move away from memory reporters that are implemented in terms of global counters.  The problem with them is that you have to add/subtract the counter in all the right places and it's all too easy to fail to do this as the code changes over time. (See bug 648490 for an example.)  Traversing the GC heap (ie. traversing the arenas, not doing a trace/mark pass) like Igor suggests seems a more robust alternative.

- Reporters for code allocations in TM were fixed in bug 633653.