The default bug view has changed. See this FAQ.

Add memory reporter for XPConnect

RESOLVED FIXED in mozilla11

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

(Blocks: 1 bug)

unspecified
mozilla11
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
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.
Assignee: nobody → nnethercote
Blocks: 563700
Depends on: 676724
OS: Linux → All
Hardware: x86_64 → All
Summary: Add some → Add memory reporter for XPConnect
Whiteboard: [MemShrink]
(Assignee)

Comment 2

5 years ago
Created attachment 576389 [details] [diff] [review]
patch
Attachment #576389 - Flags: review?(mrbkap)
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.
Attachment #576389 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 4

5 years ago
> > +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.
(Assignee)

Comment 5

5 years ago
> 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.
(Assignee)

Comment 6

5 years ago
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.
Attachment #576389 - Attachment is obsolete: true
Attachment #577489 - Flags: review?(mrbkap)
(Assignee)

Updated

5 years ago
Whiteboard: [MemShrink] → [MemShrink:P2]
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.
Attachment #577489 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 8

5 years ago
> 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|.)
(Assignee)

Comment 9

5 years ago
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.
Attachment #577489 - Attachment is obsolete: true
Attachment #578453 - Flags: review?(mrbkap)
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.
Attachment #578453 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 12

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a5174695f86
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/8a5174695f86
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
You need to log in before you can comment on or make changes to this bug.