Last Comment Bug 704391 - Add more JS runtime memory reporters and fix the existing ones
: Add more JS runtime memory reporters and fix the existing ones
Status: RESOLVED FIXED
[MemShrink:P2]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla11
Assigned To: Nicholas Nethercote [:njn] (on vacation until July 11)
:
Mentors:
Depends on: 702426
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-21 19:19 PST by Nicholas Nethercote [:njn] (on vacation until July 11)
Modified: 2011-12-02 03:26 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (9.01 KB, patch)
2011-11-21 19:19 PST, Nicholas Nethercote [:njn] (on vacation until July 11)
no flags Details | Diff | Review
patch v2 (10.34 KB, patch)
2011-11-21 21:33 PST, Nicholas Nethercote [:njn] (on vacation until July 11)
no flags Details | Diff | Review
patch v3 (6.90 KB, patch)
2011-11-27 17:15 PST, Nicholas Nethercote [:njn] (on vacation until July 11)
luke: review+
Details | Diff | Review
patch v4, diff'd against v3 (23.48 KB, patch)
2011-11-29 19:57 PST, Nicholas Nethercote [:njn] (on vacation until July 11)
luke: review+
Details | Diff | Review

Description Nicholas Nethercote [:njn] (on vacation until July 11) 2011-11-21 19:19:58 PST
Created attachment 576070 [details] [diff] [review]
patch

This patch adds some new memory reporters:

- js/runtime/temporary, for JSThreadData::tempLifoAlloc.  This is usually a
  few 100s of KB, but sometimes it spikes to multiple MBs due to parse
  nodes, and so can reduce heap-unclassified by a decent amount.
  
- js/runtime/thread-objects, for JSThreads.  On 64-bit it's usually about
  135KB or 270KB depending on whether there are one or two JSThreads.

- js/runtime/contexts, for JSContexts.  This is usually pretty small, eg.
  10s of KB.

Also, two existing reporters, js/runtime/runtime-object and
js/runtime/atoms-table, were incorrectly marked as being non-heap memory.  
This means that both "explicit" and "heap-unclassified" were over-reporting
by the sum of these two measurements, which is often about 4.5MB.  The patch
fixes this.  (I also audited all our NONHEAP-marked reporters, all the 
others were correct.)
Comment 1 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-11-21 21:33:03 PST
Created attachment 576086 [details] [diff] [review]
patch v2

This version includes JSContext::busyArrays in js/runtime/contexts.  It turns out the busyArrays are typically 2 to 3x bigger than the JSContext objects themselves.  (Nb: the |sizeOfIncludingThis| naming convention is something I'm introducing concurrently in bug 698968, it's much better than the current |countMe| boolean flag.)
Comment 2 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-11-27 17:15:49 PST
Created attachment 577174 [details] [diff] [review]
patch v3

New version, applies on top of the patch in bug 698968.
Comment 3 Luke Wagner [:luke] 2011-11-29 16:00:54 PST
Comment on attachment 577174 [details] [diff] [review]
patch v3

Sorry for the delay.
Comment 4 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-11-29 17:08:54 PST
Oh crap, I was about to remove the review request because I discovered that bug 702426 broke the existing mjit-code/regexp memory reporter and so we haven't been reporting regexp memory usage since it landed.  I'll fix that in my next patch.
Comment 5 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-11-29 19:57:21 PST
Created attachment 577838 [details] [diff] [review]
patch v4, diff'd against v3

New patch with the following additional changes (diff'd against patch v3):

- I renamed various size-getting functions to use the new sizeOfFoo() 
  conventions, as per https://wiki.mozilla.org/Platform/Memory_Reporting.

- I added measurement of ThreadData::dtoaState, just for kicks.

- I moved explicit/js/stack under explicit/js/runtime/threads, since that's
  where it logically is in the data structures.  And I renamed it 
  stack-committed, which is more accurate.

- I added explicit/js/runtime/threads/regexp-code.  I removed the old 
  distinction between "method" and "unused" under "mjit-code".

The js/runtime reporters now look like this:

│  ├────7,311,792 B (02.76%) -- runtime
│  │    ├──4,792,320 B (01.81%) -- threads
│  │    │  ├──4,194,304 B (01.58%) -- stack-committed
│  │    │  ├────262,144 B (00.10%) -- regexp-code
│  │    │  ├────196,608 B (00.07%) -- temporary
│  │    │  └────139,264 B (00.05%) -- normal
│  │    ├──2,097,152 B (00.79%) -- atoms-table
│  │    ├────331,776 B (00.13%) -- runtime-object
│  │    └─────90,544 B (00.03%) -- contexts
Comment 6 Luke Wagner [:luke] 2011-11-30 09:34:09 PST
Comment on attachment 577838 [details] [diff] [review]
patch v4, diff'd against v3

Review of attachment 577838 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/assembler/jit/ExecutableAllocator.cpp
@@ +37,5 @@
>      m_allocator->releasePoolPages(this);
>  }
>  
>  void
> +ExecutableAllocator::sizeOfCode(size_t& method, size_t& regexp, size_t& unused) const

Can you pass by * instead.  Also, SM style is the * goes next to the identifier not the type.

::: js/src/jscompartment.cpp
@@ +149,5 @@
>      return true;
>  }
>  
>  void
> +JSCompartment::sizeOfCode(size_t& method, size_t& regexp, size_t& unused) const

Same comment as above.
Comment 7 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-12-01 19:10:08 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1c49a3b76e7
Comment 8 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-12-01 21:28:36 PST
Follow-up that unbreaks non-JS_THREADSAFE builds:
https://hg.mozilla.org/integration/mozilla-inbound/rev/43adf69a4a7c

Note You need to log in before you can comment on or make changes to this bug.