1052626 - Add memory reporter for nsXPCWrappedJS

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Andrew McCreight [:mccr8]]

The DMD log in bug 1051051 has around 975,000 bytes of nsXPCWrappedJS in it under various stacks.  I think this is due to leaking thousands of SpecialPowers objects.  nsXPCWrappedJS also seems to own some XPTCallStub data through its nsAutoXPTCStub parent class.  This is another 184,185 bytes in the same DMD log.

Every root nsXPCWrappedJS is added to the wrapped JS map, so we should be able to measure these in JSObject2WrappedJSMap::SizeOfIncludingThis().  The map just includes the root nsXPCWrappedJS, so we'll have to measure everything in the chain as well.

There's currently no SizeOf method on nsXPCWrappedJS, so I think nobody else measures them.

Comment 1

[Andrew McCreight [:mccr8]]

In a browser with nothing except about:memory open, this is about 70KB.  It would be easy to measure nsXPCWrappedJSClass in the same way, but the total number of classes is going to be much smaller, and you aren't really going to leak a bunch of them in the same way, so I think it isn't particularly worth measuring.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Outline and constify JSObject2WrappedJSMap::SizeOfIncludingThis.

Just a minor cleanup.

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Report the total size of nsXPCWrappedJS.

Comment 4

[Andrew McCreight [:mccr8]]

Comment on attachment 8472005 [details] [diff] [review]
part 2 - Report the total size of nsXPCWrappedJS.

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

Let me know if you have any comments on this.  I'll probably request more formal review from bholley and froydnj.

Comment 5

[Nicholas Nethercote [inactive]]

Comment on attachment 8472005 [details] [diff] [review]
part 2 - Report the total size of nsXPCWrappedJS.

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

Looks fine.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +2797,5 @@
>      {
>          return NS_ERROR_FAILURE;
>      }
>  
>      size_t xpconnect = xpcrt->SizeOfIncludingThis(JSMallocSizeOf);

While you're here, can you rename this variable as |xpconnectRuntime|? I think this names dates back to when xpconnect usage was reported as a single number.

::: js/xpconnect/src/XPCWrappedJS.cpp
@@ +558,5 @@
> +{
> +    // mJSObject is a JS pointer, so don't measure the object.
> +    // mClass is not uniquely owned by this WrappedJS. Measure it in IID2WrappedJSClassMap.
> +    // mRoot is not measured because it is either |this| or a callee.
> +    // mOuter is rare and probably not uniquely owned by this.

Nice comment.

::: xpcom/glue/nsXPTCUtils.h
@@ +40,5 @@
> +  {
> +    if (mXPTCStub) {
> +      return NS_SizeOfIncludingThisXPTCallStub(mXPTCStub, aMallocSizeOf);
> +    }
> +    return 0;

I'd use ?: for this.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Report the total size of nsXPCWrappedJS.

try is closed, so I'll have to push this later, but this should be okay.

bholley for the XPConnect stuff, froydnj for the XPCOM stuff.

Bobby, for memory reporters, it is important that we report each object only once.  So this patch iterates over the XPCWJS roots in the table, then goes through all of the linked WJS.  I think that should be okay.  The existing memory reporter for XPCJSRuntime reports the size of the table itself, but not any of the contents.

Comment 7

[Andrew McCreight [:mccr8]]

Comment on attachment 8472005 [details] [diff] [review]
part 2 - Report the total size of nsXPCWrappedJS.

Thanks for the feedback, Nick.  I addressed the comments in the new version of my patch.

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8472565 [details] [diff] [review]
part 2 - Report the total size of nsXPCWrappedJS.

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

I think this is OK.  ISTR that we gave up on keeping xptcall straight C a while back...

Comment 9

[Andrew McCreight [:mccr8]]

The files end in .cpp so I figured it would be okay.

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Andrew McCreight [:mccr8] from comment #9)
> The files end in .cpp so I figured it would be okay.

You never know!  (I meant the API exposed by the .h files, but I see that they include JS header files, so it's probably a losing game at this point.)

Comment 11

[Andrew McCreight [:mccr8]]

green L64 debug try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6a53376d32a4

Comment 12

[Bobby Holley (:bholley)]

Comment on attachment 8472565 [details] [diff] [review]
part 2 - Report the total size of nsXPCWrappedJS.

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

Sorry for the long delay - this always required just a bit more focus and research than I had. r=bholley with the comments updated and the sizeof(pointer) issue fixed.

::: js/xpconnect/src/XPCWrappedJS.cpp
@@ +557,5 @@
> +nsXPCWrappedJS::SizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf) const
> +{
> +    // mJSObject is a JS pointer, so don't measure the object.
> +    // mClass is not uniquely owned by this WrappedJS. Measure it in IID2WrappedJSClassMap.
> +    // mRoot is not measured because it is either |this| or a callee.

I think "callee" is not the right word here.

@@ +563,5 @@
> +    size_t n = mallocSizeOf(this);
> +    n += nsAutoXPTCStub::SizeOfExcludingThis(mallocSizeOf);
> +
> +    // Wrappers form a linked list via the mNext field, so include them all
> +    // in the measurement.

Make a note that only root wrappers are stored in the map, and so everything will be measured exactly once.

::: xpcom/reflect/xptcall/xptcall.cpp
@@ +72,5 @@
> +                                  mozilla::MallocSizeOf aMallocSizeOf)
> +{
> +    // We could cast aStub to nsXPTCStubBase, but that class doesn't seem to own anything,
> +    // so just measure the size of the object itself.
> +    return aMallocSizeOf(aStub);

Well, this will always just return the size of the pointer right?

I think we should do the static cast and dereference it (unless you can just do aMallocSizeOf(nsXPTCStubBase) directly?).

The thing that threw me for a loop was my memory of the massive custom-built vtable for these things, but I guess there's only one of those per-process, so it probably isn't significant.

Comment 13

[Andrew McCreight [:mccr8]]

(In reply to (PTO 8/22 - 9/1) from comment #12)
> Sorry for the long delay - this always required just a bit more focus and
> research than I had.
No problem.  It always takes me a bit to remember what is happening when I looked at things involving XPCWrappedJS. :)

> I think "callee" is not the right word here.
Yeah, that was a little awkward.  I changed it.

> Make a note that only root wrappers are stored in the map, and so everything
> will be measured exactly once.
Done.

> Well, this will always just return the size of the pointer right?
>
> I think we should do the static cast and dereference it (unless you can just
> do aMallocSizeOf(nsXPTCStubBase) directly?).
malloc_size_of(x) isn't sizeof(x).  It asks the allocator to return the exact size of the allocated block for the pointer passed in.  In contrast to sizeof, which is statically computed, and as you say, returns the size of x itself rather than what x points to.

> The thing that threw me for a loop was my memory of the massive custom-built
> vtable for these things, but I guess there's only one of those per-process,
> so it probably isn't significant.
Hmm, yeah I'm not sure.

I'll land when the tree is open again.

Comment 14

[Andrew McCreight [:mccr8]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/66ee1f1268d9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ac76d8e0d077

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/66ee1f1268d9
https://hg.mozilla.org/mozilla-central/rev/ac76d8e0d077