The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla16

Status

()

Core
XPCOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

(Blocks: 1 bug)

unspecified
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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)
(Assignee)

Comment 1

5 years ago
Created attachment 633633 [details] [diff] [review]
patch

I don't believe it's necessary to determine the size of the AtomImpls, hence the NULL EntrySizeOf func.
Attachment #633633 - Flags: review?(justin.lebar+bug)
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?
Attachment #633633 - Flags: review?(justin.lebar+bug) → review-
> 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.
(Assignee)

Comment 4

5 years ago
(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.)
> 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.
(Assignee)

Comment 6

5 years ago
Created attachment 634069 [details] [diff] [review]
patch

I went ahead and counted the string keys in the hashtable.
Attachment #633633 - Attachment is obsolete: true
Attachment #634069 - Flags: review?(n.nethercote)
Comment on attachment 634069 [details] [diff] [review]
patch

Wrong patch!
Attachment #634069 - Flags: review?(n.nethercote)
> (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.
(Assignee)

Comment 9

5 years ago
Created attachment 634387 [details] [diff] [review]
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.
Attachment #634069 - Attachment is obsolete: true
Attachment #634387 - Flags: review?(n.nethercote)
(Assignee)

Comment 10

5 years ago
(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 on attachment 634387 [details] [diff] [review]
patch

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

Looks good.  Thanks!
Attachment #634387 - Flags: review?(n.nethercote) → review+
(Assignee)

Comment 12

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/408e24552eeb
Assignee: nobody → nfroyd
Status: NEW → ASSIGNED
(Assignee)

Comment 13

5 years ago
...and a followup for breaking the about:memory tests:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac2bddd99d8f
https://hg.mozilla.org/mozilla-central/rev/408e24552eeb
https://hg.mozilla.org/mozilla-central/rev/ac2bddd99d8f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.