765326 - about:memory's atom-table numbers should include the static atom table

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nathan Froyd [:froydnj]]

As determined by DMD post-startup on x86-64 Linux:

==32548== Unreported: 1 block(s) in record 7 of 10326
==32548==  131,072 bytes (131,072 requested / 0 slop)
==32548==  0.44% of the heap (1.42% cumulative unreported)
==32548==    at 0x4C284AF: malloc (vg_replace_malloc.c:267)
==32548==    by 0x685312F: moz_malloc (mozalloc.cpp:64)
==32548==    by 0x8DAF50C: PL_DHashAllocTable (pldhash.cpp:82)
==32548==    by 0x8DAF26B: ChangeTable(PLDHashTable*, int) (pldhash.cpp:525)
==32548==    by 0x8DAF8A5: PL_DHashTableOperate (pldhash.cpp:610)
==32548==    by 0x8DBD0E7: RegisterStaticAtoms(nsStaticAtom const*, unsigned int) (nsTHashtable.h:184)
==32548==    by 0x83DC3C3: unsigned int NS_RegisterStaticAtoms<1952u>(nsStaticAtom const (&) [1952u]) (nsStaticAtom.h:52)
==32548==    by 0x83DC3D9: nsGkAtoms::AddRefAtoms() (nsGkAtoms.cpp:36)

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: patch

I don't believe it's necessary to determine the size of the AtomImpls, hence the NULL EntrySizeOf func.

Comment 2

[Nicholas Nethercote [:njn] (On Long Service Leave)]

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

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

Stealing review.  On the right track, but I'd like to see a second version addressing the comments.  Thanks!

BTW, I'm happy to do as many memory reporter reviews as you like;  I think my reviewing load is lower than jlebar's.

::: xpcom/ds/nsAtomTable.cpp
@@ +451,5 @@
>    AtomTableEntry* entry = static_cast<AtomTableEntry*>(aHdr);
>    return entry->mAtom->SizeOfIncludingThis(aMallocSizeOf);
>  }
>  
>  size_t NS_SizeOfAtomTableIncludingThis(nsMallocSizeOfFun aMallocSizeOf) {

This should be renamed "SizeOfAtomTablesIncludingThis", and the path and description for the reporter should be updated.

@@ +460,5 @@
> +                                            aMallocSizeOf);
> +  }
> +  if (gStaticAtomTable) {
> +    n += aMallocSizeOf(gStaticAtomTable);
> +    n += gStaticAtomTable->SizeOfExcludingThis(NULL, aMallocSizeOf);

Can you add nsBaseHashTable::sizeOfIncludingThis() and call it here instead?

Comment 3

[Justin Lebar (not reading bugmail)]

> I don't believe it's necessary to determine the size of the AtomImpls

Ah, because atoms are basically empty objects.  They do have a vtable pointer, at least, so they're not completely free.  But I guess I buy this.

More significantly, what about the nsStringHashKey's themselves?  They contain nsString's, so I'd presume there's some meaningful external storage here?  It doesn't seem that the hashtable's mallocSizeOf interface is set up to allow you to compute the sizeof of the keys, although writing that shouldn't be too hard, if you're up for it.

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Justin Lebar [:jlebar] from comment #3)
> > I don't believe it's necessary to determine the size of the AtomImpls
> 
> Ah, because atoms are basically empty objects.  They do have a vtable
> pointer, at least, so they're not completely free.  But I guess I buy this.

AtomImpls are insignificant, at least according to DMD.  Maybe a couple K, couple pages max.

That's only right after startup with no addons; the situation could be different with addons or after several hours of usage.  I haven't looked into the latter two scenarios.  (I wouldn't expect to see a lot of difference, but you never know...)

> More significantly, what about the nsStringHashKey's themselves?  They
> contain nsString's, so I'd presume there's some meaningful external storage
> here?  It doesn't seem that the hashtable's mallocSizeOf interface is set up
> to allow you to compute the sizeof of the keys, although writing that
> shouldn't be too hard, if you're up for it.

This would need bug 762766 fixed.  I agree that it would be beneficial in several different areas.  But DMD doesn't seem to view these strings as a problem.  (You need to run with a high --num-callers to get past ns*String_internal's layers of code; I have done 16 in the past and don't recall things allocated along these paths as a problem.)

Comment 5

[Justin Lebar (not reading bugmail)]

> But DMD doesn't seem to view these strings as a problem.

Some arithmetic to check this: The hashtable is 128KB.  The keys are one nsString plus a 32-bit hash key (PLDHashEntryHdr).  An nsString is a pointer, 32-bit length, 32-bit flags = 16 bytes.  So each entry in the hashtable is 24 bytes (4 byte hash key, 4 bytes empty, 16 bytes nsString).

That means there are slots for ~5,400 entries in the hashtable.  I think our hashtable loads itself between 25% and 75% full, so there are 1,350 to 4,050 live nsString objects in each hashtable.

Suppose our strings are on average 5 chars long.  jemalloc rounds up to 8 bytes.  So the high-end estimate for space used by these strings is 4,050 * 8 bytes = ~32kb.

So unless these strings are usually longer than 8 bytes, I guess we're OK leaving them off.

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: patch

I went ahead and counted the string keys in the hashtable.

Comment 7

[Nicholas Nethercote [:njn] (On Long Service Leave)]

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

Wrong patch!

Comment 8

[Nicholas Nethercote [:njn] (On Long Service Leave)]

> (You need to run with a high --num-callers to get past
> ns*String_internal's layers of code; I have done 16 in the past and don't
> recall things allocated along these paths as a problem.)

Nathan, have you seen bug 717853?  It's a rewrite of DMD that doesn't require Valgrind and also produces a tree-based output.  The output is a bit harder to read at first, but it avoids the whole problem of having to choose a value for --num-callers.

Comment 9

[Nathan Froyd [:froydnj]]

Attachment: patch

(In reply to Nicholas Nethercote [:njn] from comment #7)
> Wrong patch!

...and that, ladies and gentlemen, is why you should never develop patches for two different bugs on the same branch.  Let's try that again.

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Nicholas Nethercote [:njn] from comment #8)
> > (You need to run with a high --num-callers to get past
> > ns*String_internal's layers of code; I have done 16 in the past and don't
> > recall things allocated along these paths as a problem.)
> 
> Nathan, have you seen bug 717853?  It's a rewrite of DMD that doesn't
> require Valgrind and also produces a tree-based output.  The output is a bit
> harder to read at first, but it avoids the whole problem of having to choose
> a value for --num-callers.

That'd be pretty snazzy, thanks for pointing that out.

Comment 11

[Nicholas Nethercote [:njn] (On Long Service Leave)]

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

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

Looks good.  Thanks!

Comment 12

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/408e24552eeb

Comment 13

[Nathan Froyd [:froydnj]]

...and a followup for breaking the about:memory tests:

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

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/408e24552eeb
https://hg.mozilla.org/mozilla-central/rev/ac2bddd99d8f