943744 - Minimize memory consumed while dumping memory reports to file

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Ideally, dumping memory reports to file should itself consume very little
memory.  Particularly since on B2G a dev might try dumping when memory gets
tight (and we're talking about auto-dumping when the OOM killer strikes).

I instrumented a build and found that dumping reports just after start-up in a
64-bit desktop build cause a ~8.4MB spike in heap-allocated, and about 90% of
this spike is due to the duplicate string detection in the JS engine -- it uses
a hash table to detect duplicates, and there can be lots of strings in a zone,
which means the hash table can get quite big.

Simply disabling the duplicate detection would fix the spike, but then we'd
lose the useful information.  Someone will probably suggest making it an
option, but that sucks, because we'd need to provide alternative ways of
invoking it and devs would need to guess which one is more appropriate.

Another option is to just look for big strings.  This is how it used to be.
This would miss the cases where you have a zillion copies of a small string,
but they are in the minority.

Comment 1

[Andrew McCreight [:mccr8]]

I wonder what the distribution of number of times each strings occurs is.  If 90% of strings occur only once, you could use a Bloom filter to represent the set of all strings, and then only add it to the hashtable if it is in the Bloom filter.  Or something.  Disclaimer: I don't really know anything about Bloom filters.

Or sample them like DMD does, and only add them with a % chance proportional to its length.  Interpreting any kind of probabilistically derived results may be awful, though.

Does the hash table store copies of the strings, or does it point to strings?  You might be able to save some space by doing the latter, though of course you have to root the strings, so it would have to be a cross-zone pointer or something...

Comment 2

[Nicholas Nethercote [inactive]]

I don't much like the idea of sampling.  It's not something any of the current memory reporters do.

The table stores pointers to the JSStrings, plus five other measurements:  length, numCopies, shortGCHeap, normalGCHeap, normalMallocHeap.  It wouldn't be hard to reduce the size by 1/3 or so -- length can be gotten easily from the JSString itself, and the latter three measurements could be represented more compactly.

Interesting suggestion re the Bloom Filter.  I'll take a look at that in more detail tomorrow.

Comment 3

[Nicholas Nethercote [inactive]]

> Interesting suggestion re the Bloom Filter.  I'll take a look at that in
> more detail tomorrow.

Yeah... I'm struggling to see how a Bloom Filter could be used here :)

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove StringInfo::length.

We don't need to store |length| in StringInfo, because we store the JSString*,
from which we can get the length.  This patch removes it.

We do, however, now need to store |length| in NotableStringInfo.

Comment 5

[Nicholas Nethercote [inactive]]

> (part 1) - Remove StringInfo::length.

I forgot to say:  this reduces the size of StringInfo from 7 words to 6 words.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Represent the sizes in StringInfo more compactly.

This patch tweaks how the sizes are stored, and reduces the size of StringInfo
from 6 words to 5 words.

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Re-use the |strings| table from the zone with the most strings when computing totals.

Typically, one zone (call it Z) within a runtime contains the vast majority of
the strings.  So when we combine the |strings| tables for each zone into
|zTotals.strings|, we end up with a table that has only marginally more entries
in it than Z's table.

This patch changes things so that after we find the notable strings for Z, we
reuse Z.strings table for |zTotals.strings|.  This saves times (we start the
|strings| combining process with most of the work already done) and memory (we
don't have two large |strings| tables alive at once).

In practice, this roughly halves the peak memory consumption from |strings|,
because we only have one large table alive at a time, instead of two.  

This patch also fixes a minor problem -- ZoneStats' constructor wasn't checking
for failure from strings.init().  So I added ZoneStats.initStrings().

----

Overall, these three patches reduce the memory spike while dumping reports just
after start-up from ~8.4MB to ~3.7MB.

Comment 8

[Till Schneidereit [:till]]

Comment on attachment 8340840 [details] [diff] [review]
(part 1) - Remove StringInfo::length.

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

yep

Comment 9

[Till Schneidereit [:till]]

Comment on attachment 8340842 [details] [diff] [review]
(part 2) - Represent the sizes in StringInfo more compactly.

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

Nice. I like how the code got easier to follow, too.

Comment 10

[Till Schneidereit [:till]]

Comment on attachment 8340844 [details] [diff] [review]
(part 3) - Re-use the |strings| table from the zone with the most strings when computing totals.

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

This is very nice!

Comment 11

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b89621c00ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/a00cff07e0f0
https://hg.mozilla.org/integration/mozilla-inbound/rev/15fda52eaf2f

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4b89621c00ca
https://hg.mozilla.org/mozilla-central/rev/a00cff07e0f0
https://hg.mozilla.org/mozilla-central/rev/15fda52eaf2f

Comment 13

[John Schoenick [:johns]]

Attachment: [b2g26 WIP] (part 1) - Remove StringInfo::length.

Comment 14

[John Schoenick [:johns]]

Comment on attachment 8364522 [details] [diff] [review]
[b2g26 WIP] (part 1) - Remove StringInfo::length.

So, these don't apply to b2g26 very well due to a lot of rename churn, but I did most of the busywork and left a few XXX comments for spots that I'm not sure of.

@njn - can you take a look and see if these are sane, and if so, if they should be nominated for b2g26?

Comment 15

[John Schoenick [:johns]]

Attachment: [b2g26 WIP] (part 2) - Represent the sizes in StringInfo more compactly.

For this the variables were renamed twice since b2g26, and a few of them might not make sense to apply as is.

Comment 16

[John Schoenick [:johns]]

Attachment: [b2g26 WIP] (part 3) - Re-use the |strings| table from the zone with the most strings when computing totals.

The final chunk for this one doesn't mesh with the totalArenaSize / gcHeapUnusedGcThings / rtStats->gcHeapUnusedArenas accounting as it was in b2g26

If fixing these up for b2g26 is more effort than its worth, let me know, we can probably survive without them there -- with slightly less accurate memory dumps in low ram situations for AWSY-B2G.

Comment 17

[John Schoenick [:johns]]

(In reply to John Schoenick [:johns] from comment #16)
> If fixing these up for b2g26 is more effort than its worth, let me know, we
> can probably survive without them there -- with slightly less accurate
> memory dumps in low ram situations for AWSY-B2G.

These being "the patches" in this case, not advocating breaking random memory reporters

Comment 18

[Nicholas Nethercote [inactive]]

Comment on attachment 8364522 [details] [diff] [review]
[b2g26 WIP] (part 1) - Remove StringInfo::length.

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

Looks fine.

Comment 19

[John Schoenick [:johns]]

Attachment: [b2g26] (part 1) - Remove StringInfo::length.

If part 4/5 of bug 919889 and bug 920852 are applied first, this is a mostly trivial backport

Comment 20

[John Schoenick [:johns]]

Attachment: [b2g26] (part 2) - Represent the sizes in StringInfo more compactly.

Comment 21

[John Schoenick [:johns]]

Attachment: [b2g26] (part 3) - Re-use the |strings| table from the zone with the most strings when computing totals.

Comment 22

[Nicholas Nethercote [inactive]]

Comment on attachment 8364685 [details] [diff] [review]
[b2g26] (part 3) - Re-use the |strings| table from the zone with the most strings when computing totals.

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

This one has unresolved conflicts.

Comment 23

[John Schoenick [:johns]]

So, turns out we don't need this on b2g26 anyway. Yay!