Last Comment Bug 697016 - Refine JS memory reporters
: Refine JS memory reporters
Status: RESOLVED FIXED
[MemShrink]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Nicholas Nethercote [:njn]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-24 23:02 PDT by Nicholas Nethercote [:njn]
Modified: 2011-10-28 04:27 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (16.04 KB, patch)
2011-10-24 23:02 PDT, Nicholas Nethercote [:njn]
no flags Details | Diff | Splinter Review
patch (14.11 KB, patch)
2011-10-26 21:27 PDT, Nicholas Nethercote [:njn]
bhackett1024: review+
Details | Diff | Splinter Review
patch 2 (6.61 KB, patch)
2011-10-26 21:28 PDT, Nicholas Nethercote [:njn]
bhackett1024: review+
Details | Diff | Splinter Review
patch 2, v2 (9.48 KB, patch)
2011-10-27 18:58 PDT, Nicholas Nethercote [:njn]
bhackett1024: review+
Details | Diff | Splinter Review

Description Nicholas Nethercote [:njn] 2011-10-24 23:02:53 PDT
Created attachment 569287 [details] [diff] [review]
patch

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
Comment 1 Nicholas Nethercote [:njn] 2011-10-26 21:27:01 PDT
Created attachment 569901 [details] [diff] [review]
patch

I don't much like making inDictionary public, but I don't see a better way to do it.
Comment 2 Nicholas Nethercote [:njn] 2011-10-26 21:28:23 PDT
Created attachment 569902 [details] [diff] [review]
patch 2

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/*"
Comment 3 Brian Hackett (:bhackett) 2011-10-27 10:06:59 PDT
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."
Comment 4 Brian Hackett (:bhackett) 2011-10-27 10:13:02 PDT
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.
Comment 5 Andrew McCreight [:mccr8] 2011-10-27 10:14:33 PDT
(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).
Comment 6 Nicholas Nethercote [:njn] 2011-10-27 18:58:05 PDT
Created attachment 570150 [details] [diff] [review]
patch 2, v2

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".

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