Closed Bug 697016 Opened 13 years ago Closed 13 years ago

Refine JS memory reporters

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: n.nethercote, Assigned: n.nethercote)

Details

(Whiteboard: [MemShrink])

Attachments

(2 files, 2 obsolete files)

Attached patch patch (obsolete) — Splinter Review
This draft patch refines the JS memory reporters.

- "objects" is sub-divided into "function" and "non-function".

- "shapes" is sub-divided into "tree" and "dict".

- "property-tables" is sub-divided into "shapes-extra/tree-tables" and "shapes-extra/dict-tables".

- "shape-kids" is changed to "shapes-extra/tree-shape-kids"

- "arena-*" are changed to "arena/*"

I still need to fix the inDictionary() hack.


Here's an example of the new output:

│   ├──33,010,352 B (25.10%) -- compartment(https://mail.google.com/mail/u/0/?shva=1#inbox)
│   │  ├──18,681,856 B (14.21%) -- gc-heap
│   │  │  ├───6,380,752 B (04.85%) -- objects
│   │  │  │   ├──3,291,984 B (02.50%) -- function
│   │  │  │   └──3,088,768 B (02.35%) -- non-function
│   │  │  ├───4,064,032 B (03.09%) -- scripts
│   │  │  ├───3,725,120 B (02.83%) -- shapes
│   │  │  │   ├──2,335,168 B (01.78%) -- tree
│   │  │  │   └──1,389,952 B (01.06%) -- dict
│   │  │  ├───3,672,048 B (02.79%) -- arena
│   │  │  │   ├──3,314,072 B (02.52%) -- unused
│   │  │  │   ├────212,024 B (00.16%) -- padding
│   │  │  │   └────145,952 B (00.11%) -- headers
│   │  │  ├─────568,768 B (00.43%) -- type-objects
│   │  │  └─────271,136 B (00.21%) -- strings
│   │  ├───4,534,640 B (03.45%) -- script-data
│   │  ├───3,272,608 B (02.49%) -- type-inference
│   │  │   ├──1,986,064 B (01.51%) -- script-main
│   │  │   ├──1,121,808 B (00.85%) -- object-main
│   │  │   └────164,736 B (00.13%) -- tables
│   │  ├───2,359,296 B (01.79%) -- mjit-code
│   │  │   ├──2,262,504 B (01.72%) -- method
│   │  │   ├─────96,040 B (00.07%) -- regexp
│   │  │   └────────752 B (00.00%) -- unused
│   │  ├───2,155,904 B (01.64%) -- shapes-extra
│   │  │   ├──1,440,288 B (01.10%) -- tree-tables
│   │  │   ├────369,376 B (00.28%) -- dict-tables
│   │  │   └────346,240 B (00.26%) -- tree-shape-kids
│   │  ├───1,324,024 B (01.01%) -- object-slots
│   │  ├─────368,328 B (00.28%) -- analysis-temporary
│   │  ├─────176,320 B (00.13%) -- string-chars
│   │  └─────137,376 B (00.10%) -- object-empty-shapes
Attached patch patchSplinter Review
I don't much like making inDictionary public, but I don't see a better way to do it.
Attachment #569287 - Attachment is obsolete: true
Attachment #569901 - Flags: review?(bhackett1024)
Attached patch patch 2 (obsolete) — Splinter Review
This patch adds numbers to the "other measurements" list giving total memory
used for JS objects, scripts, shapes, strings, and the method JIT.  This
seems interesting.  I could also do type inference, but I wasn't sure
whether to include "analysis-temporary" and "object-empty-shapes" on top of
"type-inference/*"
Attachment #569902 - Flags: review?(bhackett1024)
Comment on attachment 569901 [details] [diff] [review]
patch

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

::: js/src/jsscope.h
@@ +536,4 @@
>          PUBLIC_FLAGS    = HAS_SHORTID | METHOD
>      };
>  
> +    JS_FRIEND_API(bool) inDictionary() const   { return (flags & IN_DICTIONARY) != 0; }

Is the JS_FRIEND_API needed here?  inDictionary() is an inline function, and other inlines used by XPCJSRuntime like obj->isFunction() don't have this.

FWIW jsscope.h and jsobj.h are no longer installed headers and shouldn't be used outside js/src, but js/xpconnect/src has js/src in its LOCAL_INCLUDES.  This will get fixed pretty soon by bug 692277, but that doesn't need to affect this patch.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1688,2 @@
>                         "Memory on the compartment's garbage-collected JavaScript heap that holds "
> +                       "shapes that are in a property tree.",

Bit of a nit, but it would be nice if the property tree and dictionaries had a short explanation, so someone who isn't deeply familiar with the engine can follow along.

"The property tree encodes the property layout of objects in a way that is shareable among many objects."

"Dictionaries encode the property layout of particular objects."
Attachment #569901 - Flags: review?(bhackett1024) → review+
Comment on attachment 569902 [details] [diff] [review]
patch 2

Nice, I've had to write scrapers to get this data before.  For TI, I think that the analysis-temporary should be a separate category from type-inference/*, and object-empty-shapes should go in the shapes category.
Attachment #569902 - Flags: review?(bhackett1024) → review+
(In reply to Brian Hackett from comment #3)
> Is the JS_FRIEND_API needed here?  inDictionary() is an inline function, and
> other inlines used by XPCJSRuntime like obj->isFunction() don't have this.

From my previous poking around, JS_FRIEND_API specifies linker behavior on Windows (and does nothing elsewhere), so I'd think it wouldn't be needed for inline functions, at least in terms of getting things to work (no idea what the style calls for).
Attached patch patch 2, v2Splinter Review
Relative to patch 2, this patch:

- Renames all the "total" counts so the total is at the front (e.g. 
  "js-objects-total" becomes "js-total-objects", which ensures
  they're listed together.
  
- Renames "object-empty-shapes" as "shapes-extra/empty-shape-arrays", and 
  includes it in "js-total-shapes".

- Adds "js-total-type-inference" and "js-total-analysis-temporary".
Attachment #569902 - Attachment is obsolete: true
Attachment #570150 - Flags: review?(bhackett1024)
Attachment #570150 - Flags: review?(bhackett1024) → review+
Whiteboard: [MemShrink]
https://hg.mozilla.org/mozilla-central/rev/343cb6ea05bd
https://hg.mozilla.org/mozilla-central/rev/6ec5b28142d1
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
OS: Linux → All
Hardware: x86_64 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.