765344 - xpti-working-set numbers should include stats on the hashtables in xptiWorkingSet

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nathan Froyd [:froydnj]]

They are fairly substantial in a freshly-opened browser:

==11758== Unreported: 1 block(s) in record 8 of 10862
==11758==  131,072 bytes (131,072 requested / 0 slop)
==11758==  0.43% of the heap (1.93% cumulative unreported)
==11758==    at 0x4C284AF: malloc (vg_replace_malloc.c:267)
==11758==    by 0x685312F: moz_malloc (mozalloc.cpp:64)
==11758==    by 0x8DAF02C: PL_DHashAllocTable (pldhash.cpp:82)
==11758==    by 0x8DAED8B: ChangeTable(PLDHashTable*, int) (pldhash.cpp:525)
==11758==    by 0x8DAF3C5: PL_DHashTableOperate (pldhash.cpp:610)
==11758==    by 0x8DF96D3: xptiInterfaceInfoManager::VerifyAndAddEntryIfNew(XPTInterfaceDirectoryEntry*, unsigned short, xptiTypelibGuts*) (nsTHashtable.h:184)
==11758==    by 0x8DF984F: xptiInterfaceInfoManager::RegisterXPTHeader(XPTHeader*) (xptiInterfaceInfoManager.cpp:99)
==11758==    by 0x8DF98ED: xptiInterfaceInfoManager::RegisterBuffer(char*, unsigned int) (xptiInterfaceInfoManager.cpp:81)

==11758== Unreported: 1 block(s) in record 12 of 10862
==11758==  98,304 bytes (98,304 requested / 0 slop)
==11758==  0.32% of the heap (3.31% cumulative unreported)
==11758==    at 0x4C284AF: malloc (vg_replace_malloc.c:267)
==11758==    by 0x685312F: moz_malloc (mozalloc.cpp:64)
==11758==    by 0x8DAF02C: PL_DHashAllocTable (pldhash.cpp:82)
==11758==    by 0x8DAED8B: ChangeTable(PLDHashTable*, int) (pldhash.cpp:525)
==11758==    by 0x8DAF3C5: PL_DHashTableOperate (pldhash.cpp:610)
==11758==    by 0x8DF96F2: xptiInterfaceInfoManager::VerifyAndAddEntryIfNew(XPTInterfaceDirectoryEntry*, unsigned short, xptiTypelibGuts*) (nsTHashtable.h:184)
==11758==    by 0x8DF984F: xptiInterfaceInfoManager::RegisterXPTHeader(XPTHeader*) (xptiInterfaceInfoManager.cpp:99)
==11758==    by 0x8DF98ED: xptiInterfaceInfoManager::RegisterBuffer(char*, unsigned int) (xptiInterfaceInfoManager.cpp:81)

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: patch

It seemed easier to move all the memory-reporting bits to xptiInterfaceInfoManager, since that's what's actually controlling everything.

Comment 2

[Nicholas Nethercote [inactive]]

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

I'm happy to review this, but it's Saturday morning here and I won't get to it until Monday.

Comment 3

[Justin Lebar (not reading bugmail)]

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

I, on the other hand, will look at this right now.  :)

Comment 4

[Justin Lebar (not reading bugmail)]

>+NS_MEMORY_REPORTER_MALLOC_SIZEOF_FUN(XPTMallocSizeOf, "xpti-working-set")
>+
>+size_t
>+xptiInterfaceInfoManager::SizeOfIncludingThis(nsMallocSizeOfFun aMallocSizeOf)
>+{
>+    size_t n = aMallocSizeOf(this);
>+    ReentrantMonitorAutoEnter monitor(mWorkingSet.mTableReentrantMonitor);
>+    // The entries themselves are allocated out of an arena accounted
>+    // for elsewhere, so don't measure them
>+    n += mWorkingSet.mIIDTable.SizeOfExcludingThis(NULL, XPTMallocSizeOf);
>+    n += mWorkingSet.mNameTable.SizeOfExcludingThis(NULL, XPTMallocSizeOf);
>+    return n;
>+}

What about the things inside this hashtable?  The xptiInterfaceEntry objects don't look small.  (Ignoring the size of the hash *keys* looks correct here.  nsDepCharHashKey doesn't own its string, while nsIDHashKey has no pointers.)

>+// static
>+PRInt64
>+xptiInterfaceInfoManager::GetXPTIWorkingSetSize()
>+{
>+    size_t n = XPT_SizeOfArena(gXPTIStructArena, XPTMallocSizeOf);
>+
>+    if (gInterfaceInfoManager) {
>+        n += gInterfaceInfoManager->SizeOfIncludingThis(XPTMallocSizeOf);
>+    }
>+
>+    return n;
>+}
>+
>+NS_MEMORY_REPORTER_IMPLEMENT(xptiWorkingSet,
>+                             "explicit/xpti-working-set",
>+                             KIND_HEAP,
>+                             UNITS_BYTES,
>+                             xptiInterfaceInfoManager::GetXPTIWorkingSetSize,
>+                             "Memory used by the XPCOM typelib system.")
>+

Would it make sense to have two (or three) reporters here, instead of just one?  I don't pretend to know what this is measuring, but usually more granularity is good.  If, in the quest for granularity, you left off mallocsizeof(xptiInterfaceInfoManager), that probably wouldn't be a loss.

Clearing the r?, but you can reset it (to me or Nick!) if you think the patch is good as-is.

Comment 5

[Nathan Froyd [:froydnj]]

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

(In reply to Justin Lebar [:jlebar] from comment #4)
> >+NS_MEMORY_REPORTER_MALLOC_SIZEOF_FUN(XPTMallocSizeOf, "xpti-working-set")
> >+
> >+size_t
> >+xptiInterfaceInfoManager::SizeOfIncludingThis(nsMallocSizeOfFun aMallocSizeOf)
> >+{
> >+    size_t n = aMallocSizeOf(this);
> >+    ReentrantMonitorAutoEnter monitor(mWorkingSet.mTableReentrantMonitor);
> >+    // The entries themselves are allocated out of an arena accounted
> >+    // for elsewhere, so don't measure them
> >+    n += mWorkingSet.mIIDTable.SizeOfExcludingThis(NULL, XPTMallocSizeOf);
> >+    n += mWorkingSet.mNameTable.SizeOfExcludingThis(NULL, XPTMallocSizeOf);
> >+    return n;
> >+}
> 
> What about the things inside this hashtable?  The xptiInterfaceEntry objects
> don't look small.  (Ignoring the size of the hash *keys* looks correct here.
> nsDepCharHashKey doesn't own its string, while nsIDHashKey has no pointers.)

xptiInterfaceEntry is allocated out of gXPTIStructArena:

http://dxr.lanedo.com/mozilla-central/xpcom/reflect/xptinfo/src/xptiInterfaceInfo.cpp.html#l42

And we already measure that arena.

> >+NS_MEMORY_REPORTER_IMPLEMENT(xptiWorkingSet,
> >+                             "explicit/xpti-working-set",
> >+                             KIND_HEAP,
> >+                             UNITS_BYTES,
> >+                             xptiInterfaceInfoManager::GetXPTIWorkingSetSize,
> >+                             "Memory used by the XPCOM typelib system.")
> >+
> 
> Would it make sense to have two (or three) reporters here, instead of just
> one?  I don't pretend to know what this is measuring, but usually more
> granularity is good.  If, in the quest for granularity, you left off
> mallocsizeof(xptiInterfaceInfoManager), that probably wouldn't be a loss.

My rough guideline has been trying to split out anything that takes >= ~1% of memory in whatever scenario you're trying to measure.  xpti takes a decent chunk of memory on startup (~2%), but for more complex scenarios, it's not going to be that much.

The lazy part of me says one reporter is just fine. :)  But if you think a second reporter would be worthwhile for gInterfaceInfoManager, I can do that.  I'll bounce the patch to Nicholas as a tie-breaker vote.

Comment 6

[Nicholas Nethercote [inactive]]

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

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

One bucket seems fine.  We can add more later if necessary.

Comment 7

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/657f9a63368b

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/657f9a63368b