Last Comment Bug 704723 - Add memory reporter for XPConnect
: Add memory reporter for XPConnect
Status: RESOLVED FIXED
[MemShrink:P2]
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla11
Assigned To: Nicholas Nethercote [:njn]
:
Mentors:
Depends on: DMD
Blocks: DarkMatter
  Show dependency treegraph
 
Reported: 2011-11-22 19:02 PST by Nicholas Nethercote [:njn]
Modified: 2012-02-01 14:00 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (12.46 KB, patch)
2011-11-22 19:05 PST, Nicholas Nethercote [:njn]
mrbkap: review+
Details | Diff | Review
patch v2 (25.33 KB, patch)
2011-11-28 21:51 PST, Nicholas Nethercote [:njn]
mrbkap: review+
Details | Diff | Review
patch v3 (31.88 KB, patch)
2011-12-01 17:28 PST, Nicholas Nethercote [:njn]
mrbkap: review+
Details | Diff | Review

Description Nicholas Nethercote [:njn] 2011-11-22 19:02:28 PST

    
Comment 1 Nicholas Nethercote [:njn] 2011-11-22 19:04:58 PST
This patch adds a memory reporter for XPConnect, "explicit/js/xpconnect".  It measures some things that DMD has identified as taking non-trivial amounts of memory.  The reports are typically a few 100s of KBs, depending on workload.
Comment 2 Nicholas Nethercote [:njn] 2011-11-22 19:05:54 PST
Created attachment 576389 [details] [diff] [review]
patch
Comment 3 Blake Kaplan (:mrbkap) (please use needinfo!) 2011-11-25 06:40:45 PST
Comment on attachment 576389 [details] [diff] [review]
patch

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

::: js/src/jsdhash.cpp
@@ +783,5 @@
>      return i;
>  }
>  
> +extern JS_PUBLIC_API(size_t)
> +JS_DHashSizeOfExcludingThis(JSDHashTable *table, JSUsableSizeFun usf)

Don't forget to regenerate pldhash.{h,cpp} to make sure that they stay up to date.

::: js/xpconnect/src/XPCWrappedNativeScope.cpp
@@ +993,5 @@
> +size_t
> +XPCWrappedNativeScope::SizeOfAllScopesIncludingThis()
> +{
> +    // njn: not sure if this locking is needed and/or if this is the right way
> +    // to do it

This *is* the right way to do the locking here. I think that with luke's assertion that the main JSRuntime is only used on the main thread, the locking (all of it, actually) in XPConnect can go, but technically, this does need to be serialized. So, I'd leave the locking in and we'll take it out in one fell swoop in a later pass.
Comment 4 Nicholas Nethercote [:njn] 2011-11-25 17:05:09 PST
> > +extern JS_PUBLIC_API(size_t)
> > +JS_DHashSizeOfExcludingThis(JSDHashTable *table, JSUsableSizeFun usf)
> 
> Don't forget to regenerate pldhash.{h,cpp} to make sure that they stay up to
> date.

Oh, is pldhash generated from jsdhash?  I didn't know that... I just looked in the relevant Makefile.in files and couldn't see how/where the generation happens.
Comment 5 Nicholas Nethercote [:njn] 2011-11-28 14:33:15 PST
> Oh, is pldhash generated from jsdhash?  I didn't know that... I just looked
> in the relevant Makefile.in files and couldn't see how/where the generation
> happens.

I just found this comment in pldhash.h:

  "Try to keep this file in sync with js/src/jsdhash.h"

AFAICT one was copied from the other, but there are tons of minor differences between the two (due to different types, etc) and the keeping in sync is entirely manual.  Amusingly enough, in bug 698968 I added PL_DHashTableSizeOf() without adding JS_DHashTableSizeOf()... I'll rectify that.
Comment 6 Nicholas Nethercote [:njn] 2011-11-28 21:51:38 PST
Created attachment 577489 [details] [diff] [review]
patch v2

New version, measures more stuff.  Points of interest:

- I augmented jsdhash's SizeOf functions to make it easier to get the sizes of 
  anything pointed to by the entries.

- I got pldhash and jsdhash in sync.

- Here are the things that are now being measured:

  - XPCJSRuntime::this.

  - XPCJSRuntime::mWrappedJSMap (JSObject2WrappedJSMap) and its pointers to
    nsXPCWrappedJS objects.

  - XPCJSRuntime::mClassInfo2NativeSetMap (ClassInfo2NativeSetMap) and
    its pointers to XPCNativeSet objects.  Actually, I had to disable the
    XPCNativeSet measurements because DMD was telling me that some heap
    blocks were being measured twice.

  - XPCJSRuntime::mJSHolders, but nothing its entries point to.

  - XPCWrappedNativeScope::this.

  - XPCWrappedNativeScope::mWrappedNativeMap (Native2WrappedNativeMap) and 
    its pointers to XCPWrappedNative objects.

  - XPCWrappedNativeScope::{mWrappedNativeProtoMap,mMainThreadWrappedNativeProtoMap}
    (ClassInfo2WrappedNativeProtoMap) and their pointers to
    XPCWrappedNativeProto objects.

  Any hints on the mClassInfo2NativeSetMap double reporting would be 
  welcome!

With all the new stuff measured, the measured xpconnect usage easily exceeds
1 or 2MB on basic workloads, much more than I had in the last patch.
Comment 7 Blake Kaplan (:mrbkap) (please use needinfo!) 2011-11-30 06:26:27 PST
Comment on attachment 577489 [details] [diff] [review]
patch v2

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

::: js/xpconnect/src/XPCMaps.cpp
@@ +297,5 @@
> +{
> +    size_t n = 0;
> +    n += mallocSizeOf(this, sizeof(ClassInfo2NativeSetMap));
> +    // njn: not measuring the XPCNativeSets pointed to by the entries because
> +    // that causes some/all of them to be double-counted.

So the ClassInfo2NativeSetMap is an optimization that allows us to quickly find the XPCNativeSet that corresponds to a given ClassInfo implementation. So, this map is really a cache that covers a subset of XPCNativeSets. So, any other place that counts XPCNativeSets could easily double count this map's entries.

It might make sense to never count XPCNativeSets anywhere else, and do a separate pass to count the memory used by the native sets by iterating over mNativeSetMap.
Comment 8 Nicholas Nethercote [:njn] 2011-12-01 14:29:16 PST
> So the ClassInfo2NativeSetMap is an optimization that allows us to quickly
> find the XPCNativeSet that corresponds to a given ClassInfo implementation.
> So, this map is really a cache that covers a subset of XPCNativeSets. So,
> any other place that counts XPCNativeSets could easily double count this
> map's entries.

No other places currently count XPCNativeSets, but I found that ClassInfo2NativeSetMap has lots of duplicate entries (i.e. multiple entries point to the same XPCNativeSet), which explains the double-counting.  Are these duplicates expected?

Another odd thing -- moz_malloc_usable_size() tells me that the XPCNativeSets being pointed to are 64 bytes, but sizeof(XPCNativeSet) is only 16 bytes.  Why is that?  Oh, I see, its data members are like this:

  private:
    PRUint16                mMemberCount;
    PRUint16                mInterfaceCount;
    XPCNativeInterface*     mInterfaces[1];  // always last - object sized for array
  };

So I guess when measuring an XPCNativeSet I should do this:

  sizeof(XPCNativeSet) + (mInterfaceCount - 1) * sizeof(XPCNativeInterface *)

(And that assumes |mInterfaceCount > 0|.)
Comment 9 Nicholas Nethercote [:njn] 2011-12-01 17:28:14 PST
Created attachment 578453 [details] [diff] [review]
patch v3

New version.  I tried to interdiff against the previous version but it 
failed, sorry.

The new parts:

- I fixed the computedSize in jsdhash.cpp and pldhash.cpp to account for 
  ENTRY_STORE_EXTRA, which avoids some NS_ASSERTION failures in debug 
  builds.
  
- I'm now measuring mIID2NativeInterfaceMap and all the XPCNativeInterfaces
  that hang off it, as well as mNativeSetMap and all the XPCNativeSets that
  hang off it.  Both XPCNativeInterfaces and XPCNativeSets are 
  variable-sized and I've handled that.
  
  With these being measured, the xpconnect things that remain
  unmeasured are really small -- no individual DMD records are larger than a
  couple of KB with a few tabs open.
Comment 10 Blake Kaplan (:mrbkap) (please use needinfo!) 2011-12-02 06:13:29 PST
Ack, sorry. I wasn't CC'd to the bug, so I didn't see this until the review request.

(In reply to Nicholas Nethercote [:njn] from comment #8)
> No other places currently count XPCNativeSets, but I found that
> ClassInfo2NativeSetMap has lots of duplicate entries (i.e. multiple entries
> point to the same XPCNativeSet), which explains the double-counting.  Are
> these duplicates expected?

Yes. I'd expect classes that don't offer any interfaces by default (but implement nsIClassInfo) would end up sharing an XPCNativeSet with only nsISupports in them.

> (And that assumes |mInterfaceCount > 0|.)

In case you were wondering, this is a safe assumption: all objects exposed to script implement at least nsISupports.
Comment 11 Blake Kaplan (:mrbkap) (please use needinfo!) 2011-12-02 06:36:42 PST
Comment on attachment 578453 [details] [diff] [review]
patch v3

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

If you want, I think you can nuke the if statement in XPCNativeSet::SizeOfIncludingThis, but it's correct either way.
Comment 12 Nicholas Nethercote [:njn] 2011-12-05 17:35:17 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a5174695f86
Comment 13 Ed Morley [:emorley] 2011-12-06 10:06:19 PST
https://hg.mozilla.org/mozilla-central/rev/8a5174695f86

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