711130 - summarize per-compartment data so that it is easy to see aggregate quantities/percentages

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Attachment: patch

Three times now I've wanted to see the sum total for all dictionary/tree/base shapes in my browser (say, after running membuster).  This patch adds this.

On a more general note, it seems like devs would want this for every measurement that gets reported per-compartment.  Perhaps we should consider eventually adding the general mechanism (which would make patches like this one unnecessary) which simply sum up ever per-compartment measurement and report the results (perhaps in a separate expando-group from Other Measurements)?

Comment 1

[Luke Wagner [:luke]]

Attachment: patch (v.2)

On second thought, I think I should have named the new entries js-total-shapes-tree instead of js-total-tree-shapes.

Comment 2

[Brian Hackett [Laid off!]]

(In reply to Luke Wagner [:luke] from comment #0)
> On a more general note, it seems like devs would want this for every
> measurement that gets reported per-compartment.  Perhaps we should consider
> eventually adding the general mechanism (which would make patches like this
> one unnecessary) which simply sum up ever per-compartment measurement and
> report the results (perhaps in a separate expando-group from Other
> Measurements)?

I would like to see this, either as a separate expando-group or (maybe less confusing, and won't change the current output) have a link next to and similar to the 'more verbose' one that just collapses all compartments together.

Comment 3

[Nicholas Nethercote [inactive]]

Comment on attachment 582018 [details] [diff] [review]
patch (v.2)

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

I've given detailed comments below, but I have sufficient misgivings about the patch to give r-.  My initial inclination was to say "sorry, about:memory will never be all things to all people".  But I can see why you want this.

If we're going to head down this road, we should do it in a systematic way, as you suggested.  It's annoying that we have so many different measurements for the JS engine.  (Any suggestions for removing some are welcome... I've often thought that arena/headers and arena/padding could be combined.)  I don't want to create another expando group if it can be avoided, but enough people have requested trees in "Other Measurements" I can see the writing on the wall.  I need to think about this a bit.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +2048,5 @@
>  
>          ReportMemoryBytes(NS_LITERAL_CSTRING("js-total-shapes"),
>                            nsIMemoryReporter::KIND_OTHER, data.totalShapes,
>                            "Memory used for all shape-related data.  This is the sum of all "
> +                          "'compartments' 'gc-heap/shapes/tree', 'gc-heap/shapes/dict', "

This change is wrong.  It should be |compartments'|, that's a possessive apostrophe.

@@ +2054,5 @@
>                            "'shapes-extra/tree-tables', 'shapes-extra/dict-tables', "
>                            "'shapes-extra/tree-shape-kids' and 'shapes-extra/empty-shape-arrays'.",
>                            callback, closure);
>  
> +        ReportMemoryBytes(NS_LITERAL_CSTRING("js-total-shapes-tree"),

I don't like "js-total-shapes-tree" because we already have "js-total-shapes" and it covers stuff on the GC heap as well as malloc-allocated stuff.  "js-total-gc-heap-shapes-tree" would be better.  Likewise for the others.

@@ +2057,5 @@
>  
> +        ReportMemoryBytes(NS_LITERAL_CSTRING("js-total-shapes-tree"),
> +                          nsIMemoryReporter::KIND_OTHER, data.totalShapesTree,
> +                          "Memory used for tree shape on the gc heap.  This is the sum of all "
> +                          "'gc-heap/shapes/tree'.",

These descriptions need improvement.  They're not grammatically correct ("used for tree shape") and they don't match the existing ones.  Something like this is better:

"Memory for shapes that are in a property tree.  This is the
sum of all compartments' 'gc-heap/shapes/tree' numbers."

Similar changes are needed for the other descriptions.

::: js/xpconnect/src/xpcpublic.h
@@ +249,5 @@
>          totalObjects(0),
>          totalShapes(0),
> +        totalShapesTree(0),
> +        totalShapesDict(0),
> +        totalShapesBase(0),

These names should change to match the path used.

Comment 4

[Luke Wagner [:luke]]

Fine, I'll take the general case then.  In addition to bhackett, billm has also expressed the desire for such a feature.

Comment 5

[Steve Fink [:sfink] [:s:]]

So right now about:memory's js section is sort of GROUP BY compartment  (or rather GROUP BY compartment.first_loaded_url). It'd be nice to have a set of alternative options like GROUP BY shapes.*

The layout section is GROUP BY shell. I would suggest putting the about:memory data into sqlite and allowing actual GROUP BY queries, but then njn or taras would come and break my fingers and I need them to type.

Comment 6

[Justin Lebar (not reading bugmail)]

> I don't want to create another expando group if it can be avoided, but enough people have requested 
> trees in "Other Measurements" I can see the writing on the wall.

What other use-cases have been presented?

Comment 7

[Nicholas Nethercote [inactive]]

> What other use-cases have been presented?

The author of DownThemAll! wanted a tree for the add-on's 5 (or so) reporters.  That was mostly for presentation purposes, not for adding, AFAIK.

But I can imagine doing this for the layout/shell reporters -- having totals for each of "arenas", "styledata" and "text-runs".  The same thing could be done in any case where we have a fixed number of reports about a variable number of things (compartments, layout shells, DOM windows, SQLite connections, etc).  And if we ever rearrange things so that the primary division is per-tab, I think this will become even more useful.

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: Fix "window-objects" measurement inconsistency.

The "window-objects" number in the "explicit" tree doesn't match the one in
the "Other Measurements" section in about:memory.  This is because frame 
measurements that get lumped into "sundries" aren't counted toward the 
latter.  This patch fixes that.

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 636207 [details] [diff] [review]
Fix "window-objects" measurement inconsistency.

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

Doh, yes.  Thanks for fixing that!

Comment 10

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Merge "arena/unused" and "arena/padding" into "arena-admin".

This patch merges "arena/unused" and "arena/padding" into "arena-admin".
Distinguishing the two has never been helpful, and now it's consistent with
"chunk-admin".

Comment 11

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Use consistent names for reports of unused JS memory.

I've always found the "chunk-clean-unused" and "chunk-dirty-unused" names
hard to remember.  This patches renames them as "unused-chunks" and
"unused-arenas", and also renames "arena/unused" as "unused-gc-things".
This is more consistent and I find it much easier to remember what each
thing means.

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Treeify the non-compartment gc-heap-XYZ measurements.

This patch puts the non-compartment "gc-heap-XYZ" numbers into a sub-tree
under "explicit/js".  For example:

│  ├───2,748,416 B (04.24%) -- gc-heap
│  │   ├──1,486,848 B (02.30%) ── unused-arenas
│  │   ├──1,048,576 B (01.62%) ── unused-chunks
│  │   ├────212,992 B (00.33%) ── chunk-admin
│  │   └──────────0 B (00.00%) ── decommitted

I just think it's nicer that way.

Comment 13

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Overhaul the "other measurements" measurements for JS memory consumption.

This patch is the main one.  It's best explained by looking at the JS numbers
in the "Other Measurements" section of about:memory looks like this:

  99,164,984 B (100.0%) -- js-main-runtime
  ├──78,757,224 B (79.42%) -- compartments
  │  ├──30,883,840 B (31.14%) -- gc-heap
  │  │  ├───8,968,384 B (09.04%) -- objects
  │  │  │   ├──4,673,504 B (04.71%) ── non-function
  │  │  │   └──4,294,880 B (04.33%) ── function
  │  │  ├───7,480,752 B (07.54%) ── unused-gc-things
  │  │  ├───6,328,312 B (06.38%) -- shapes
  │  │  │   ├──3,516,120 B (03.55%) ── tree
  │  │  │   ├──1,571,400 B (01.58%) ── dict
  │  │  │   └──1,240,792 B (01.25%) ── base
  │  │  ├───3,924,576 B (03.96%) ── strings
  │  │  ├───2,899,296 B (02.92%) ── scripts
  │  │  ├─────826,752 B (00.83%) ── type-objects
  │  │  ├─────455,448 B (00.46%) ── arena-admin
  │  │  └─────────320 B (00.00%) ── sundries
  │  ├──27,832,608 B (28.07%) ── analysis-temporary
  │  ├───7,546,048 B (07.61%) ── script-data
  │  ├───4,323,792 B (04.36%) -- shapes-extra
  │  │   ├──1,506,944 B (01.52%) ── compartment-tables
  │  │   ├──1,295,072 B (01.31%) ── tree-tables
  │  │   ├────785,504 B (00.79%) ── dict-tables
  │  │   └────736,272 B (00.74%) ── tree-shape-kids
  │  ├───4,078,448 B (04.11%) -- type-inference
  │  │   ├──3,179,168 B (03.21%) ── script-main
  │  │   ├────638,416 B (00.64%) ── object-main
  │  │   └────260,864 B (00.26%) ── tables
  │  ├───1,618,992 B (01.63%) -- objects
  │  │   ├──1,339,136 B (01.35%) ── slots
  │  │   ├────205,808 B (00.21%) ── elements
  │  │   └─────74,048 B (00.07%) ── misc
  │  ├───1,499,928 B (01.51%) ── string-chars
  │  ├─────550,720 B (00.56%) ── mjit-data
  │  └─────422,848 B (00.43%) ── cross-compartment-wrappers
  ├──14,591,440 B (14.71%) ── runtime
  └───5,816,320 B (05.87%) -- gc-heap
      ├──3,313,664 B (03.34%) ── decommitted
      ├──1,048,576 B (01.06%) ── unused-chunks
      ├────897,024 B (00.90%) ── unused-arenas
      └────557,056 B (00.56%) ── chunk-admin

  33,386,496 B (100.0%) -- js-main-runtime-gc-heap-committed
  ├──23,960,144 B (71.77%) -- used
  │  ├──22,947,640 B (68.73%) ── gc-things
  │  ├─────557,056 B (01.67%) ── chunk-admin
  │  └─────455,448 B (01.36%) ── arena-admin
  └───9,426,352 B (28.23%) -- unused
      ├──7,480,752 B (22.41%) ── gc-things
      ├──1,048,576 B (03.14%) ── chunks
      └────897,024 B (02.69%) ── arenas

Things to note in the output above:

- The "js-main-runtime" measurement matches the "explicit/js" one.  It's
  dominated by the "compartments" sub-tree, which is just the sum of all the
  individual compartment sub-trees from the "explicit/js" tree.  The other two
  sub-trees are for "runtime" and "gc-heap".

- The "js-main-runtime-gc-heap-committed" tree is just about the committed
  portion of the GC heap.  It's more detailed and much easier to understand
  than the mish-mash of numbers we used to have relating to that stuff.

Changes of note in the patch:

- I added a very useful comment in MemoryMetrics.h that helps explain how
  the GC heap is measured.

- I loosened a constraint on memory reporters;  reports that end up in the
  "Other Measurement" section of about:memory can now have KIND_HEAP or
  KIND_NONHEAP.  I did this so I could reuse ReportCompartmentStats() for
  the compartment totals.

Comment 14

[Justin Lebar (not reading bugmail)]

(In reply to Nicholas Nethercote [:njn] from comment #11)
> Created attachment 636273 [details] [diff] [review]
> (part 3) - Use consistent names for reports of unused JS memory.
> 
> I've always found the "chunk-clean-unused" and "chunk-dirty-unused" names
> hard to remember.  This patches renames them as "unused-chunks" and
> "unused-arenas", and also renames "arena/unused" as "unused-gc-things".
> This is more consistent and I find it much easier to remember what each
> thing means.

fwiw I think these are good names.

Comment 15

[Luke Wagner [:luke]]

Thanks, this is going to be mega useful.  (I was just wishing for it 2 days ago.)

Comment 16

[Terrence Cole [:terrence]]

Comment on attachment 636272 [details] [diff] [review]
(part 2) - Merge "arena/unused" and "arena/padding" into "arena-admin".

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

Works for me.

Comment 17

[Terrence Cole [:terrence]]

Comment on attachment 636273 [details] [diff] [review]
(part 3) - Use consistent names for reports of unused JS memory.

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

I like the new names much better.

This orphaned gcHeapChunkCleanDecommitted and gcHeapChunkDirtyDecommitted, could you update those as well?

Comment 18

[Terrence Cole [:terrence]]

Comment on attachment 636276 [details] [diff] [review]
(part 4) - Treeify the non-compartment gc-heap-XYZ measurements.

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

I wondered why it wasn't this way already when I added decommitted to it.

Comment 19

[Terrence Cole [:terrence]]

Comment on attachment 636278 [details] [diff] [review]
(part 5) - Overhaul the "other measurements" measurements for JS memory consumption.

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

::: js/public/MemoryMetrics.h
@@ +32,5 @@
>      size_t temporary;
> +
> +    void add(TypeInferenceSizes &sizes) {
> +        #define ADD(x)  this->x += sizes.x
> +

This macro doubles the line count, obfuscates, and doesn't save much typing.  Probably best to just type it out for this case, unless you anticipate having to add 10's more TI stats.

Comment 20

[Nicholas Nethercote [inactive]]

Attachment: (part 6) - Improve the measurement of decommitted GC memory.

This patch removes the gcHeapChunkCleanDecommitted measurement, because it's
(a) a slightly odd thing to measure, and (b) (almost?) always zero, in
practice.

It also renames gcHeapChunkDirtyDecommitted as gcHeapDecommittedArenas, and
uses "decommitted-arenas" in about:memory.  And
countCleanDecommittedArenas() isn't needed any more.

Comment 21

[Terrence Cole [:terrence]]

Comment on attachment 636549 [details] [diff] [review]
(part 6) - Improve the measurement of decommitted GC memory.

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

Looks good.

Comment 22

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7519820af249
https://hg.mozilla.org/integration/mozilla-inbound/rev/425e7d2dcbd7
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7e25244e43a
https://hg.mozilla.org/integration/mozilla-inbound/rev/b81037d455e4
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebab6b4a9c47
https://hg.mozilla.org/integration/mozilla-inbound/rev/947e8490af73

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7519820af249
https://hg.mozilla.org/mozilla-central/rev/425e7d2dcbd7
https://hg.mozilla.org/mozilla-central/rev/a7e25244e43a
https://hg.mozilla.org/mozilla-central/rev/b81037d455e4
https://hg.mozilla.org/mozilla-central/rev/ebab6b4a9c47
https://hg.mozilla.org/mozilla-central/rev/947e8490af73