The default bug view has changed. See this FAQ.

Add more JS runtime memory reporters and fix the existing ones

RESOLVED FIXED in mozilla11

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 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

5 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

5 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

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

Comment 2

5 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

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

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

Comment 4

5 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

5 years ago
Depends on: 702426
(Assignee)

Comment 5

5 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

5 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

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

Comment 8

5 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: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
You need to log in before you can comment on or make changes to this bug.