Last Comment Bug 712835 - Add a memory reporter for the nsAtomTable
: Add a memory reporter for the nsAtomTable
Status: RESOLVED FIXED
[MemShrink]
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla12
Assigned To: Nicholas Nethercote [:njn] (on vacation until July 11)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-21 17:27 PST by Nicholas Nethercote [:njn] (on vacation until July 11)
Modified: 2011-12-28 11:10 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch 1 (1.25 KB, patch)
2011-12-22 17:21 PST, Nicholas Nethercote [:njn] (on vacation until July 11)
khuey: review+
Details | Diff | Review
patch 2 (5.32 KB, patch)
2011-12-22 17:22 PST, Nicholas Nethercote [:njn] (on vacation until July 11)
khuey: review+
Details | Diff | Review
patch 3 (5.31 KB, patch)
2011-12-22 17:23 PST, Nicholas Nethercote [:njn] (on vacation until July 11)
khuey: review+
Details | Diff | Review

Description Nicholas Nethercote [:njn] (on vacation until July 11) 2011-12-21 17:27:56 PST
DMD says it's reasonable prominent:

 Unreported: 1 block(s) in record 9 of 17967
  262,144 bytes (262,144 requested / 0 slop) 
  0.25% of the heap (7.19% cumulative unreported)
    at 0x402A063: malloc (vg_replace_malloc.c:263)
    by 0x403C0A4: moz_malloc (mozalloc.cpp:113)
    by 0x7A4126C: PL_DHashAllocTable (pldhash.cpp:114)
    by 0x7A41AAB: ChangeTable(PLDHashTable*, int) (pldhash.cpp:564)
    by 0x7A41D26: PL_DHashTableOperate (pldhash.cpp:649)
    by 0x7A565D9: GetAtomHashEntry(unsigned short const*, unsigned int) (nsAtomT
able.cpp:403)
    by 0x7A56A0F: NS_NewAtom(nsAString_internal const&) (nsAtomTable.cpp:534)
    by 0x68B4D95: do_GetAtom(nsAString_internal const&) (nsIAtom.h:199)

That's just the hash table storage, which I'm sure can get substantially bigger with more browsing.  There's also the AtomImpls pointed to by the hash table entries, and then the strings hanging off the AtomImpls, but I'll need to look more closely to see if they actually belong to the AtomImpls.
Comment 1 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-12-22 17:21:18 PST
Created attachment 583974 [details] [diff] [review]
patch 1

This patch just adds nsStringBuffer::SizeOfIncludingThisIfUnshared(), which the next patch will use.
Comment 2 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-12-22 17:22:20 PST
Created attachment 583975 [details] [diff] [review]
patch 2

This adds the "explicit/atom-table" memory reporter.  With Gmail and
TechCrunch open it's just over 1MB worth.

I created EnsureTableExists() for an earlier version where I registered the
memory reporter in nsAtomTable.cpp.  That ended up having to change, but I
left EnsureTableExists() in because I think it's an improvement.
Comment 3 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-12-22 17:23:43 PST
Created attachment 583977 [details] [diff] [review]
patch 3

This patch moves AtomImpl and PermanentAtomImpl out of the header, because they don't need to be there.
Comment 4 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-12-27 08:38:10 PST
Comment on attachment 583977 [details] [diff] [review]
patch 3

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

Why do we want this?
Comment 5 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-12-27 08:40:42 PST
Comment on attachment 583977 [details] [diff] [review]
patch 3

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

Ok, I guess hiding these is fine.
Comment 6 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-12-27 14:13:50 PST
> Why do we want this?

Basic information hiding, that's all.  I just figured I'd fix it while I was in the area.

Note You need to log in before you can comment on or make changes to this bug.