961883 - Improve JS memory reporters, for GGC and other things

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

I just did a DMD run of a browser with generational GC enabled. We need to write reporters for new GGC data structures, and I found a couple of large existing structures that also lack reporters.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 0) - Some tiny HashTable tweaks.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Measure and report the SourceDataCache.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Measure and report the StoreBuffer.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Measure and report the Nursery.

Here is sample output showing the effect of part 2 and 3:

│   │  ├──17,301,952 B (05.40%) -- gc
│   │  │  ├──16,777,216 B (05.23%) ── nursery
│   │  │  ├─────262,592 B (00.08%) -- store-buffer
│   │  │  │     ├───65,600 B (00.02%) ── cells
│   │  │  │     ├───65,600 B (00.02%) ── generics
│   │  │  │     ├───65,600 B (00.02%) ── slots
│   │  │  │     ├───65,600 B (00.02%) ── whole-cells
│   │  │  │     ├───────64 B (00.00%) ── reloc-cells
│   │  │  │     ├───────64 B (00.00%) ── reloc-vals
│   │  │  │     └───────64 B (00.00%) ── vals
│   │  │  └─────262,144 B (00.08%) ── marker

I tested all the new reporting code with DMD.

BTW, I think it's been said elsewhere, but 16 MiB for the nursery ain't gonna
fly for B2G, where we have multiple processes and multiple workers in the main
process.

Comment 5

[:Benjamin Peterson]

Comment on attachment 8362757 [details] [diff] [review]
(part 1) - Measure and report the SourceDataCache.

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

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +2273,5 @@
>                    "Memory used for the math cache.");
>  
> +    RREPORT_BYTES(rtPath + NS_LITERAL_CSTRING("runtime/source-data-cache"),
> +                  KIND_HEAP, rtStats.runtime.sourceDataCache,
> +                  "Memory used for the source data cache.");

Maybe mention that it's decompressed from the compressed version in script-sources?

Comment 6

[Hugh Nougher [:Hughman]]

(In reply to Nicholas Nethercote [:njn] from comment #4)
> 16 MiB for the nursery ain't gonna fly for B2G

See bug 957723

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Measure and report the StoreBuffer.

Making it compile on non-generational builds is probably a good idea...

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Measure and report the Nursery.

Comment 9

[Terrence Cole [:terrence]]

Comment on attachment 8362851 [details] [diff] [review]
(part 2) - Measure and report the StoreBuffer.

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

\o/ Great! Thanks for doing this! r=me

Comment 10

[Terrence Cole [:terrence]]

Comment on attachment 8362852 [details] [diff] [review]
(part 3) - Measure and report the Nursery.

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

r=me

::: js/src/gc/Nursery.h
@@ +122,5 @@
>  
>      /* Forward a slots/elements pointer stored in an Ion frame. */
>      void forwardBufferPointer(HeapSlot **pSlotsElems);
>  
> +    size_t sizeOfHeap() { return start() ? NurserySize : 0; }

Once bug 957723 lands, the mapped size will remain the same but the committed size will be |currentEnd_ - start()| (e.g. generally 1MiB). Which of these does about:memory expect? I guess we could have nursery-committed and nursery-decommitted? If that sounds reasonable I'll add it in the other bug after this lands.

Comment 11

[Nicholas Nethercote [inactive]]

> Once bug 957723 lands, the mapped size will remain the same but the
> committed size will be |currentEnd_ - start()| (e.g. generally 1MiB). Which
> of these does about:memory expect? I guess we could have nursery-committed
> and nursery-decommitted? If that sounds reasonable I'll add it in the other
> bug after this lands.

Having both is good. "nursery-committed" should be in the "explicit" tree where "nursery" currently is. "nursery-decommitted" should be in the "decommitted" tree in the "Other Measurements" section, where we have other known-to-be decommitted stuff. I.e. we'll just need to replace the "explicit/" prefix of the path with "decommitted/".

Comment 12

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b575ca55b37
https://hg.mozilla.org/integration/mozilla-inbound/rev/92081c50e18c
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff7db0b58713
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab2800166064

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9b575ca55b37
https://hg.mozilla.org/mozilla-central/rev/92081c50e18c
https://hg.mozilla.org/mozilla-central/rev/ff7db0b58713
https://hg.mozilla.org/mozilla-central/rev/ab2800166064