Last Comment Bug 789398 - Rework type inference memory reporters
: Rework type inference memory reporters
Status: RESOLVED FIXED
[js:t][MemShrink]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: 16 Branch
: All All
: -- normal (vote)
: mozilla19
Assigned To: Nicholas Nethercote [:njn]
:
Mentors:
: 781849 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-06 23:14 PDT by Nicholas Nethercote [:njn]
Modified: 2012-11-19 07:40 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Bug 789398 - Rework the type inference memory reporters. (12.22 KB, patch)
2012-09-06 23:23 PDT, Nicholas Nethercote [:njn]
bhackett1024: review+
Details | Diff | Splinter Review

Description Nicholas Nethercote [:njn] 2012-09-06 23:14:49 PDT
Bug 781767 comment 4 suggests that the type inference memory reporters could do with some reworking.
Comment 1 Nicholas Nethercote [:njn] 2012-09-06 23:23:57 PDT
Created attachment 659138 [details] [diff] [review]
Bug 789398 - Rework the type inference memory reporters.

I'm not sure what my timing is like with this patch -- has bhackett left for
his long vacation yet?

This patch:

- Makes the type-inference memory reporters more fine-grained, going from
  four distinct measurements to nine.  In TypeInferenceSizes:

    scripts --> typeScripts + typeResults

    temporary --> analysisPool + typePool + pendingArrays

    tables --> allocationSiteTables + arrayTypeTables + objectTypeTables

    objects --> typeObjects

  The names I used were heavily based on the relevant types.  I haven't
  bothered with descriptions for each of the measurements yet, I figured I'd
  wait to see what the initial reaction is.

- It no longer subtracts some of the bytes measured in |temporary| and adds
  them to |scripts| and |objects|.  Computing sizes analytically is
  error-prone, and in this case leads to bug 775382.

- I've moved the temporary reporters into the "type-inference" sub-tree for
  the simple reason that I don't understand why they weren't there in the
  first place.  I'm happy to hear arguments against this move.

Here's an example with a few sites open:

│  ├───9,337,696 B (09.53%) -- type-inference
│  │   ├──5,767,168 B (05.89%) ── type-pool
│  │   ├──2,883,584 B (02.94%) ── analysis-pool
│  │   ├────311,888 B (00.32%) ── type-scripts
│  │   ├────293,648 B (00.30%) ── allocation-site-tables
│  │   ├─────60,752 B (00.06%) ── object-type-tables
│  │   ├─────12,464 B (00.01%) ── array-type-tables
│  │   └──────8,192 B (00.01%) ── pending-arrays

I guess "type-results" and "type-objects" were small enough that they got
folded into "other-sundries" (not shown).
Comment 2 Nicholas Nethercote [:njn] 2012-09-10 20:22:16 PDT
*** Bug 781849 has been marked as a duplicate of this bug. ***
Comment 3 Brian Hackett (:bhackett) 2012-11-15 17:58:17 PST
Comment on attachment 659138 [details] [diff] [review]
Bug 789398 - Rework the type inference memory reporters.

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

Sorry for the super late review!  This came in just after I left on my trip, and I forgot to take care of it during the work week.

::: js/src/jsinfer.cpp
@@ +6231,5 @@
>      /*
>       * Note: not all data in the pool is temporary, and some will survive GCs
>       * by being copied to the replacement pool. This memory will be counted
>       * elsewhere and deducted from the amount of temporary data.
>       */

This comment is obsolete now.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1547,5 @@
>                    "Memory used by cross-compartment wrappers.");
>  
> +    CREPORT_BYTES(cJSPathPrefix + NS_LITERAL_CSTRING("type-inference/type-scripts"),
> +                  cStats.typeInferenceSizes.typeScripts,
> +                  "XXX.");

"Memory for type sets associated with scripts"

@@ +1552,4 @@
>  
> +    CREPORT_BYTES(cJSPathPrefix + NS_LITERAL_CSTRING("type-inference/type-results"),
> +                  cStats.typeInferenceSizes.typeResults,
> +                  "XXX.");

"Memory for dynamic type results produced by scripts"

@@ +1556,4 @@
>  
> +    CREPORT_BYTES(cJSPathPrefix + NS_LITERAL_CSTRING("type-inference/analysis-pool"),
> +                  cStats.typeInferenceSizes.analysisPool,
> +                  "XXX.");

"Memory holding transient analysis information used during type inference and compilation"

@@ +1560,4 @@
>  
> +    CREPORT_BYTES(cJSPathPrefix + NS_LITERAL_CSTRING("type-inference/type-pool"),
> +                  cStats.typeInferenceSizes.typePool,
> +                  "XXX.");

"Memory holding contents of type sets and related data"

@@ +1563,5 @@
> +                  "XXX.");
> +
> +    CREPORT_BYTES(cJSPathPrefix + NS_LITERAL_CSTRING("type-inference/pending-arrays"),
> +                  cStats.typeInferenceSizes.pendingArrays,
> +                  "XXX.");

"Memory used for solving constraints during type inference"

@@ +1567,5 @@
> +                  "XXX.");
> +
> +    CREPORT_BYTES(cJSPathPrefix + NS_LITERAL_CSTRING("type-inference/allocation-site-tables"),
> +                  cStats.typeInferenceSizes.allocationSiteTables,
> +                  "XXX.");

"Memory indexing type objects associated with allocation sites"

@@ +1571,5 @@
> +                  "XXX.");
> +
> +    CREPORT_BYTES(cJSPathPrefix + NS_LITERAL_CSTRING("type-inference/array-type-tables"),
> +                  cStats.typeInferenceSizes.arrayTypeTables,
> +                  "XXX.");

"Memory indexing type objects associated with array literals"

@@ +1575,5 @@
> +                  "XXX.");
> +
> +    CREPORT_BYTES(cJSPathPrefix + NS_LITERAL_CSTRING("type-inference/object-type-tables"),
> +                  cStats.typeInferenceSizes.objectTypeTables,
> +                  "XXX.");

"Memory indexing type objects associated with object literals"

@@ +1579,5 @@
> +                  "XXX.");
> +
> +    CREPORT_BYTES(cJSPathPrefix + NS_LITERAL_CSTRING("type-inference/type-objects"),
> +                  cStats.typeInferenceSizes.typeObjects,
> +                  "XXX.");

"Miscellaneous additional information associated with type objects"
Comment 4 Nicholas Nethercote [:njn] 2012-11-18 16:46:55 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/e10975ff4a07
Comment 5 Ed Morley [:emorley] 2012-11-19 07:40:08 PST
https://hg.mozilla.org/mozilla-central/rev/e10975ff4a07

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