747509 - nsDocument::DocSizeOfExcludingThis should include ID and styled links tables

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Nathan Froyd [:froydnj]]

Running DMD on a browser viewing the HTML5 single-page spec turns up:

==28808== Unreported: 1 block(s) in record 7 of 15608
==28808==  1,052,672 bytes (1,048,580 requested / 4,092 slop)
==28808==  0.47% of the heap (17.11% cumulative unreported)
==28808==    at 0x4C27193: malloc (vg_replace_malloc.c:263)
==28808==    by 0x6A5A1C9: moz_malloc (mozalloc.cpp:97)
==28808==    by 0x9C4FCED: PL_DHashAllocTable (pldhash.cpp:117)
==28808==    by 0x9C50710: ChangeTable(PLDHashTable*, int) (pldhash.cpp:560)
==28808==    by 0x9C50AB5: PL_DHashTableOperate (pldhash.cpp:645)
==28808==    by 0x8B62E3C: nsTHashtable<nsIdentifierMapEntry>::PutEntry(nsAString_internal const&) (nsTHashtable.h:198)
==28808==    by 0x8B46FDE: nsDocument::AddToIdTable(mozilla::dom::Element*, nsIAtom*) (nsDocument.cpp:2646)
==28808==    by 0x8B9A6D5: nsGenericElement::BindToTree(nsIDocument*, nsIContent*, nsIContent*, bool) (nsGenericElement.cpp:3227)
==28808==    by 0x8D1864D: nsGenericHTMLElement::BindToTree(nsIDocument*, nsIContent*, nsIContent*, bool) (nsGenericHTMLElement.cpp:1130)
==28808==    by 0x8B9C093: nsINode::doInsertChildAt(nsIContent*, unsigned int, bool, nsAttrAndChildArray&) (nsGenericElement.cpp:3802)
==28808==    by 0x8B9B987: nsGenericElement::InsertChildAt(nsIContent*, unsigned int, bool) (nsGenericElement.cpp:3735)
==28808==    by 0x875A73E: nsINode::AppendChildTo(nsIContent*, bool) (nsINode.h:563)
==28808== 
==28808== Unreported: 1 block(s) in record 8 of 15608
==28808==  1,052,672 bytes (1,048,580 requested / 4,092 slop)
==28808==  0.47% of the heap (17.59% cumulative unreported)
==28808==    at 0x4C27193: malloc (vg_replace_malloc.c:263)
==28808==    by 0x6A5A1C9: moz_malloc (mozalloc.cpp:97)
==28808==    by 0x9C4FCED: PL_DHashAllocTable (pldhash.cpp:117)
==28808==    by 0x9C50710: ChangeTable(PLDHashTable*, int) (pldhash.cpp:560)
==28808==    by 0x9C50AB5: PL_DHashTableOperate (pldhash.cpp:645)
==28808==    by 0x8B64D80: nsTHashtable<nsPtrHashKey<mozilla::dom::Link> >::PutEntry(mozilla::dom::Link*) (nsTHashtable.h:198)
==28808==    by 0x8B57F04: nsDocument::AddStyleRelevantLink(mozilla::dom::Link*) (nsDocument.cpp:7550)
==28808==    by 0x8C19402: mozilla::dom::Link::LinkState() const (Link.cpp:137)
==28808==    by 0x8B59CBA: EnumeratePendingLinkUpdates(nsPtrHashKey<mozilla::dom::Link>*, void*) (nsDocument.cpp:8087)
==28808==    by 0x8B691F5: nsTHashtable<nsPtrHashKey<mozilla::dom::Link> >::s_EnumStub(PLDHashTable*, PLDHashEntryHdr*, 
unsigned int, void*) (nsTHashtable.h:500)
==28808==    by 0x9C50F88: PL_DHashTableEnumerate (pldhash.cpp:750)
==28808==    by 0x8B64F62: nsTHashtable<nsPtrHashKey<mozilla::dom::Link> >::EnumerateEntries(PLDHashOperator (*)(nsPtrHashKey<mozilla::dom::Link>*, void*), void*) (nsTHashtable.h:251)

So those might be useful to keep track of, possibly along with the name table.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: patch

These came up again when viewing a fairly small merge to m-c:

==18974== Unreported: 1 block(s) in record 1 of 13741
==18974==  4,198,400 bytes (4,194,308 requested / 4,092 slop)
==18974==  3.29% of the heap (3.29% cumulative unreported)
==18974==    at 0x4C27193: malloc (vg_replace_malloc.c:263)
==18974==    by 0x6A5A1C9: moz_malloc (mozalloc.cpp:64)
==18974==    by 0x9D95553: PL_DHashAllocTable (pldhash.cpp:82)
==18974==    by 0x9D95F76: ChangeTable(PLDHashTable*, int) (pldhash.cpp:525)
==18974==    by 0x9D9631B: PL_DHashTableOperate (pldhash.cpp:610)
==18974==    by 0x8BAC66E: nsTHashtable<nsIdentifierMapEntry>::PutEntry(nsAString_internal const&, mozilla::fallible_t const&) (nsTHashtable.h:184)
==18974==    by 0x8BA7B42: nsTHashtable<nsIdentifierMapEntry>::PutEntry(nsAString_internal const&) (nsTHashtable.h:170)
==18974==    by 0x8B8B302: nsDocument::AddToIdTable(mozilla::dom::Element*, nsIAtom*) (nsDocument.cpp:2595)
==18974==    by 0x8BE0831: nsGenericElement::BindToTree(nsIDocument*, nsIContent*, nsIContent*, bool) (nsGenericElement.cpp:3186)
==18974==    by 0x8D904FD: nsGenericHTMLElement::BindToTree(nsIDocument*, nsIContent*, nsIContent*, bool) (nsGenericHTMLElement.cpp:1700)
==18974==    by 0x8DAC277: nsHTMLAnchorElement::BindToTree(nsIDocument*, nsIContent*, nsIContent*, bool) (nsHTMLAnchorElement.cpp:229)
==18974==    by 0x8BE21EF: nsINode::doInsertChildAt(nsIContent*, unsigned int, bool, nsAttrAndChildArray&) (nsGenericElement.cpp:3761)
==18974== 
==18974== Unreported: 3 block(s) in record 5 of 13741
==18974==  1,073,152 bytes (1,060,876 requested / 12,276 slop)
==18974==  0.84% of the heap (5.92% cumulative unreported)
==18974==    at 0x4C27193: malloc (vg_replace_malloc.c:263)
==18974==    by 0x6A5A1C9: moz_malloc (mozalloc.cpp:64)
==18974==    by 0x9D95553: PL_DHashAllocTable (pldhash.cpp:82)
==18974==    by 0x9D95F76: ChangeTable(PLDHashTable*, int) (pldhash.cpp:525)
==18974==    by 0x9D9631B: PL_DHashTableOperate (pldhash.cpp:610)
==18974==    by 0x8BADD16: nsTHashtable<nsPtrHashKey<mozilla::dom::Link> >::PutEntry(mozilla::dom::Link*, mozilla::fallible_t const&) (nsTHashtable.h:184)
==18974==    by 0x8BA9A7C: nsTHashtable<nsPtrHashKey<mozilla::dom::Link> >::PutEntry(mozilla::dom::Link*) (nsTHashtable.h:170)
==18974==    by 0x8B9BF6A: nsDocument::AddStyleRelevantLink(mozilla::dom::Link*) (nsDocument.cpp:7480)
==18974==    by 0x8C5FECB: mozilla::dom::Link::LinkState() const (Link.cpp:104)
==18974==    by 0x8B9DD20: EnumeratePendingLinkUpdates(nsPtrHashKey<mozilla::dom::Link>*, void*) (nsDocument.cpp:8017)
==18974==    by 0x8BADE35: nsTHashtable<nsPtrHashKey<mozilla::dom::Link> >::s_EnumStub(PLDHashTable*, PLDHashEntryHdr*, unsigned int, void*) (nsTHashtable.h:486)
==18974==    by 0x9D967EE: PL_DHashTableEnumerate (pldhash.cpp:715)

(4% of heap!) so I thought it'd be worth fixing that.

I wasn't sure whether to assign them their own subnodes in about:memory, or lump them into the DOM.  Is there a guideline for when things get their own subtree?

Comment 2

[Nicholas Nethercote [inactive]]

Comment on attachment 630255 [details] [diff] [review]
patch

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

f=me.  Please run DMD to check there's no double-counting.  As for splitting off to separate sub-trees, there are no rules.  Do it if you think it's worthwhile.  But I think these entries will usually be small.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: patch

Yeah, probably small enough.  We'll just let them sit in dom/other.

Comment 4

[Nicholas Nethercote [inactive]]

Comment on attachment 631227 [details] [diff] [review]
patch

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

Looks good, 

BTW, I hope/assume you're doing DMD runs each time you improve the covereage to (a) make sure the memory you think is covered really *is* covered, and (b) there's no double-counting.

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Nicholas Nethercote [:njn] from comment #4)
> BTW, I hope/assume you're doing DMD runs each time you improve the covereage
> to (a) make sure the memory you think is covered really *is* covered, and
> (b) there's no double-counting.

Yup, did that.  Pushed as:

https://hg.mozilla.org/integration/mozilla-inbound/rev/885395287478

Going to leave open, as bug 762766 needs to get fixed before we can properly account for all the id table memory.

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/885395287478

Comment 7

[Nathan Froyd [:froydnj]]

Attachment: followup for hash keys

After jlebar pointed out my mental block in bug 762766, writing this patch became much easier.

Comment 8

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/542074e589eb

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/542074e589eb