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 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.

> > +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.

> 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.

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 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.

> 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|.)

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.

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 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.