Add more JS runtime memory reporters and fix the existing ones

RESOLVED FIXED in mozilla11

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla11
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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.)
Attachment #576070 - Flags: review?(luke)
(Assignee)

Comment 1

6 years ago
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.)
Attachment #576070 - Attachment is obsolete: true
Attachment #576070 - Flags: review?(luke)
Attachment #576086 - Flags: review?(luke)
(Assignee)

Updated

6 years ago
Whiteboard: [MemShrink] → [MemShrink:P2]
(Assignee)

Comment 2

6 years ago
Created attachment 577174 [details] [diff] [review]
patch v3

New version, applies on top of the patch in bug 698968.
Attachment #576086 - Attachment is obsolete: true
Attachment #576086 - Flags: review?(luke)
Attachment #577174 - Flags: review?(luke)

Comment 3

6 years ago
Comment on attachment 577174 [details] [diff] [review]
patch v3

Sorry for the delay.
Attachment #577174 - Flags: review?(luke) → review+
(Assignee)

Comment 4

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

Updated

6 years ago
Depends on: 702426
(Assignee)

Comment 5

6 years ago
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
Attachment #577838 - Flags: review?(luke)

Comment 6

6 years ago
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.
Attachment #577838 - Flags: review?(luke) → review+
(Assignee)

Comment 7

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1c49a3b76e7
(Assignee)

Comment 8

6 years ago
Follow-up that unbreaks non-JS_THREADSAFE builds:
https://hg.mozilla.org/integration/mozilla-inbound/rev/43adf69a4a7c
https://hg.mozilla.org/mozilla-central/rev/f1c49a3b76e7
https://hg.mozilla.org/mozilla-central/rev/43adf69a4a7c
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
You need to log in before you can comment on or make changes to this bug.