Last Comment Bug 765344 - xpti-working-set numbers should include stats on the hashtables in xptiWorkingSet
: xpti-working-set numbers should include stats on the hashtables in xptiWorkin...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Nathan Froyd [:froydnj]
:
: Nathan Froyd [:froydnj]
Mentors:
Depends on:
Blocks: DarkMatter
  Show dependency treegraph
 
Reported: 2012-06-15 12:52 PDT by Nathan Froyd [:froydnj]
Modified: 2012-06-19 01:18 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (5.70 KB, patch)
2012-06-15 12:56 PDT, Nathan Froyd [:froydnj]
n.nethercote: review+
Details | Diff | Splinter Review

Description Nathan Froyd [:froydnj] 2012-06-15 12:52:41 PDT
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] 2012-06-15 12:56:39 PDT
Created attachment 633634 [details] [diff] [review]
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 [:njn] 2012-06-15 15:00:06 PDT
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) 2012-06-15 15:05:18 PDT
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) 2012-06-15 15:36:11 PDT
>+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] 2012-06-18 08:20:51 PDT
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 [:njn] 2012-06-18 16:08:14 PDT
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 8 Ed Morley [:emorley] 2012-06-19 01:18:32 PDT
https://hg.mozilla.org/mozilla-central/rev/657f9a63368b

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