Closed Bug 946781 Opened 6 years ago Closed 6 years ago

need a memory reporter for mozJSComponentLoader

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: froydnj, Assigned: froydnj)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 2 obsolete files)

Running DMD on a fresh browser on x86-64 Linux, I see:

Unreported: 58 blocks in stack trace record 1 of 23,847
 712,704 bytes (501,584 requested / 211,120 slop)
 1.60% of the heap (1.60% cumulative);  4.41% of unreported (4.41% cumulative)
 Allocated at
   replace_malloc (/home/froydnj/src/mozilla-central-official/memory/replace/dmd/DMD.cpp:1247) 0xca21e6cb
   moz_xmalloc (/home/froydnj/src/mozilla-central-official/memory/mozalloc/mozalloc.cpp:53) 0xca20f46a
   Array (/opt/build/froydnj/build-mc/dom/bindings/../../dist/include/mozilla/Array.h:20) 0xc57548a9
   xpc::CreateGlobalObject(JSContext*, JSClass const*, nsIPrincipal*, JS::CompartmentOptions&) (/home/froydnj/src/mozilla-central-official/js/xpconnect/src/nsXPConnect.cpp:466) 0xc5b0f838
   XPCWrappedNative::WrapNewGlobal(xpcObjectHelper&, nsIPrincipal*, bool, JS::CompartmentOptions&, XPCWrappedNative**) (/home/froydnj/src/mozilla-central-official/js/xpconnect/src/XPCWrappedNative.cpp:181) 0xc5b1ce2f
   nsXPConnect::InitClassesWithNewWrappedGlobal(JSContext*, nsISupports*, nsIPrincipal*, unsigned int, JS::CompartmentOptions&, nsIXPConnectJSObjectHolder**) (/home/froydnj/src/mozilla-central-official/js/xpconnect/src/nsXPConnect.cpp:496) 0xc5b1d3b9
   mozJSComponentLoader::PrepareObjectForLocation(JSCLContextHelper&, nsIFile*, nsIURI*, bool, bool*) (/home/froydnj/src/mozilla-central-official/js/xpconnect/loader/mozJSComponentLoader.cpp:633) 0xc5b367f3
   mozJSComponentLoader::ObjectForLocation(nsIFile*, nsIURI*, JSObject**, char**, bool, JS::MutableHandle<JS::Value>) (/home/froydnj/src/mozilla-central-official/js/xpconnect/loader/mozJSComponentLoader.cpp:726) 0xc5b36eee
   nsTHashtable<nsBaseHashtableET<nsCStringHashKey, mozJSComponentLoader::ModuleEntry*> >::RemoveEntry(nsACString_internal const&) (/opt/build/froydnj/build-mc/js/xpconnect/loader/../../../dist/include/nsTHashtable.h:186) 0xc5b38480
   mozJSComponentLoader::Import(nsACString_internal const&, JS::Value const&, JSContext*, unsigned char, JS::Value*) (/home/froydnj/src/mozilla-central-official/js/xpconnect/loader/mozJSComponentLoader.cpp:1088) 0xc5b39c5f
   nsXPCComponents_Utils::Import(nsACString_internal const&, JS::Value const&, JSContext*, unsigned char, JS::Value*) (/home/froydnj/src/mozilla-central-official/js/xpconnect/src/XPCComponents.cpp:2826) 0xc5acabb8
   NS_InvokeByIndex (/home/froydnj/src/mozilla-central-official/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:164) 0xc511084b
   CallMethodHelper::Call() (/home/froydnj/src/mozilla-central-official/js/xpconnect/src/XPCWrappedNative.cpp:1909) 0xc5b281e7
   XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (/home/froydnj/src/mozilla-central-official/js/xpconnect/src/XPCWrappedNative.cpp:1873) 0xc5b226fe
   XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) (/home/froydnj/src/mozilla-central-official/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1301) 0xc5b22d39
   js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (/home/froydnj/src/mozilla-central-official/js/src/jscntxtinlines.h:221) 0xc6f8acf9
   js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (/home/froydnj/src/mozilla-central-official/js/src/vm/Interpreter.cpp:463) 0xc6f74eb0
   Interpret (/home/froydnj/src/mozilla-central-official/js/src/vm/Interpreter.cpp:2505) 0xc6f68105
   RunScript (/home/froydnj/src/mozilla-central-official/js/src/vm/Interpreter.cpp:420) 0xc6f740fe
   js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::AbstractFramePtr, JS::Value*) (/home/froydnj/src/mozilla-central-official/js/src/vm/Interpreter.cpp:614) 0xc6f74296
   ~Rooted (/opt/build/froydnj/build-mc/js/src/../../dist/include/js/RootingAPI.h:757) 0xc6f7473c
   JS_ExecuteScript(JSContext*, JSObject*, JSScript*, JS::Value*) (/home/froydnj/src/mozilla-central-official/js/src/jsapi.cpp:4754) 0xc6e3c27e
   ~Rooted (/opt/build/froydnj/build-mc/js/src/../../dist/include/js/RootingAPI.h:757) 0xc6e3c4bf
   mozJSComponentLoader::ObjectForLocation(nsIFile*, nsIURI*, JSObject**, char**, bool, JS::MutableHandle<JS::Value>) (/home/froydnj/src/mozilla-central-official/js/xpconnect/loader/mozJSComponentLoader.cpp:977) 0xc5b37a86

Unreported: 21 blocks in stack trace record 4 of 23,847
 258,048 bytes (181,608 requested / 76,440 slop)
 0.58% of the heap (4.65% cumulative);  1.60% of unreported (12.83% cumulative)
 Allocated at
   replace_malloc (/home/froydnj/src/mozilla-central-official/memory/replace/dmd/DMD.cpp:1247) 0xca21e6cb
   moz_xmalloc (/home/froydnj/src/mozilla-central-official/memory/mozalloc/mozalloc.cpp:53) 0xca20f46a
   Array (/opt/build/froydnj/build-mc/dom/bindings/../../dist/include/mozilla/Array.h:20) 0xc57548a9
   xpc::CreateGlobalObject(JSContext*, JSClass const*, nsIPrincipal*, JS::CompartmentOptions&) (/home/froydnj/src/mozilla-central-official/js/xpconnect/src/nsXPConnect.cpp:466) 0xc5b0f838
   XPCWrappedNative::WrapNewGlobal(xpcObjectHelper&, nsIPrincipal*, bool, JS::CompartmentOptions&, XPCWrappedNative**) (/home/froydnj/src/mozilla-central-official/js/xpconnect/src/XPCWrappedNative.cpp:181) 0xc5b1ce2f
   nsXPConnect::InitClassesWithNewWrappedGlobal(JSContext*, nsISupports*, nsIPrincipal*, unsigned int, JS::CompartmentOptions&, nsIXPConnectJSObjectHolder**) (/home/froydnj/src/mozilla-central-official/js/xpconnect/src/nsXPConnect.cpp:496) 0xc5b1d3b9
   mozJSComponentLoader::PrepareObjectForLocation(JSCLContextHelper&, nsIFile*, nsIURI*, bool, bool*) (/home/froydnj/src/mozilla-central-official/js/xpconnect/loader/mozJSComponentLoader.cpp:633) 0xc5b367f3
   mozJSComponentLoader::ObjectForLocation(nsIFile*, nsIURI*, JSObject**, char**, bool, JS::MutableHandle<JS::Value>) (/home/froydnj/src/mozilla-central-official/js/xpconnect/loader/mozJSComponentLoader.cpp:726) 0xc5b36eee
   nsTHashtable<nsBaseHashtableET<nsCStringHashKey, mozJSComponentLoader::ModuleEntry*> >::RemoveEntry(nsACString_internal const&) (/opt/build/froydnj/build-mc/js/xpconnect/loader/../../../dist/include/nsTHashtable.h:186) 0xc5b38480
   mozJSComponentLoader::Import(nsACString_internal const&, JS::Value const&, JSContext*, unsigned char, JS::Value*) (/home/froydnj/src/mozilla-central-official/js/xpconnect/loader/mozJSComponentLoader.cpp:1088) 0xc5b39c5f
   nsXPCComponents_Utils::Import(nsACString_internal const&, JS::Value const&, JSContext*, unsigned char, JS::Value*) (/home/froydnj/src/mozilla-central-official/js/xpconnect/src/XPCComponents.cpp:2826) 0xc5acabb8
   NS_InvokeByIndex (/home/froydnj/src/mozilla-central-official/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:164) 0xc511084b
   CallMethodHelper::Call() (/home/froydnj/src/mozilla-central-official/js/xpconnect/src/XPCWrappedNative.cpp:1909) 0xc5b281e7
   XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (/home/froydnj/src/mozilla-central-official/js/xpconnect/src/XPCWrappedNative.cpp:1873) 0xc5b226fe
   XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) (/home/froydnj/src/mozilla-central-official/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1301) 0xc5b22d39
   js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (/home/froydnj/src/mozilla-central-official/js/src/jscntxtinlines.h:221) 0xc6f8acf9
   js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (/home/froydnj/src/mozilla-central-official/js/src/vm/Interpreter.cpp:463) 0xc6f74eb0
   Interpret (/home/froydnj/src/mozilla-central-official/js/src/vm/Interpreter.cpp:2505) 0xc6f68105
   RunScript (/home/froydnj/src/mozilla-central-official/js/src/vm/Interpreter.cpp:420) 0xc6f740fe
   js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (/home/froydnj/src/mozilla-central-official/js/src/vm/Interpreter.cpp:484) 0xc6f74dec
   js_fun_call(JSContext*, unsigned int, JS::Value*) (/home/froydnj/src/mozilla-central-official/js/src/jsfun.cpp:893) 0xc6e3d26a
   js_fun_apply(JSContext*, unsigned int, JS::Value*) (/home/froydnj/src/mozilla-central-official/js/src/jsfun.cpp:931) 0xc6e3db34
   js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (/home/froydnj/src/mozilla-central-official/js/src/jscntxtinlines.h:221) 0xc6f8acf9
   js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (/home/froydnj/src/mozilla-central-official/js/src/vm/Interpreter.cpp:463) 0xc6f74eb0

...and a couple other instances further down.  These all add up to a significant fraction of the heap for a freshly-started browser, and we should be tracking them in about:memory.

(Making them take up less memory would be great, too...)
Isn't that just the proto and iface cache?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #1)
> Isn't that just the proto and iface cache?

Yes.

But it doesn't look like any of mozJSComponentLoader is being tracked, AFAICS.  And tracking the proto/iface cache would require setting up something for the loader, so...
Trivial patch; I thought it a good idea to separate out the runtime bits from
the component loader bits.
Attachment #8343288 - Flags: review?(bobbyholley+bmo)
This patch is where all the work is.  bholley for the xpconnect bits,
njn for the memory reporting bits.

Tested with DMD, which reports no overlapping blocks.  The only
ProtoAndIfaceCache bits that don't get reported now are from sandboxes
(and workers...), and I can't see a good way to measure those right
offhand.  Feedback welcome on ideas of what to do there.
Attachment #8343290 - Flags: review?(n.nethercote)
Attachment #8343290 - Flags: review?(bobbyholley+bmo)
Attachment #8343288 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 8343290 [details] [diff] [review]
part 2 - add memory used by mozJSComponentLoader to about:memory

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

::: js/xpconnect/loader/mozJSComponentLoader.cpp
@@ +1391,5 @@
> +    size_t amount = aMallocSizeOf(this);
> +
> +    amount += aMallocSizeOf(location);
> +    if (js::GetObjectClass(obj)->flags & JSCLASS_DOM_GLOBAL &&
> +        mozilla::dom::HasProtoAndIfaceArray(obj)) {

It seems pretty weird to me to be counting this here. Shouldn't we be counting this whenever we count all of the other per-{compartment,global} data like the XPCWrappedNativeScope?

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +2724,5 @@
> +    size_t jsComponentLoaderSize = 0;
> +
> +    if (loader) {
> +        jsComponentLoaderSize = loader->SizeOfIncludingThis(JSMallocSizeOf);
> +    }

No braces for single-line consequents in XPConnect.

But really, I'd just right this as:

size_t jsComponentLoaderSize = loader ? loader->SizeOfIncludingThis(JSMallocSizeOf) : 0;
Attachment #8343290 - Flags: review?(bobbyholley+bmo) → review-
Comment on attachment 8343290 [details] [diff] [review]
part 2 - add memory used by mozJSComponentLoader to about:memory

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

::: js/xpconnect/loader/mozJSComponentLoader.cpp
@@ +585,5 @@
> +                                                    const nsAutoPtr<ModuleEntry>& aData,
> +                                                    MallocSizeOf aMallocSizeOf, void*)
> +{
> +    return aKey.SizeOfExcludingThisIfUnshared(aMallocSizeOf) +
> +        aData->SizeOfIncludingThis(aMallocSizeOf);

Not sure if this should be IncludingThis or ExcludingThis.

@@ +591,5 @@
> +
> +size_t
> +mozJSComponentLoader::SizeOfIncludingThis(MallocSizeOf aMallocSizeOf)
> +{
> +    size_t amount = aMallocSizeOf(this);

I usually use |n| for the name of this variable.
Attachment #8343290 - Flags: review?(n.nethercote)
(In reply to Bobby Holley (:bholley) from comment #5)
> Comment on attachment 8343290 [details] [diff] [review]
> part 2 - add memory used by mozJSComponentLoader to about:memory
> 
> Review of attachment 8343290 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/xpconnect/loader/mozJSComponentLoader.cpp
> @@ +1391,5 @@
> > +    size_t amount = aMallocSizeOf(this);
> > +
> > +    amount += aMallocSizeOf(location);
> > +    if (js::GetObjectClass(obj)->flags & JSCLASS_DOM_GLOBAL &&
> > +        mozilla::dom::HasProtoAndIfaceArray(obj)) {
> 
> It seems pretty weird to me to be counting this here. Shouldn't we be
> counting this whenever we count all of the other per-{compartment,global}
> data like the XPCWrappedNativeScope?

Oh, hm, probably!  So this:

http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/xpcprivate.h#1210

is the actual object that we need to be measuring with this?  Do we need to do some sort of uniqueness check here for B2G's One Global To Rule Them All scheme?
Flags: needinfo?(bobbyholley+bmo)
(In reply to Nathan Froyd (:froydnj) from comment #7)
> (In reply to Bobby Holley (:bholley) from comment #5)
> > Comment on attachment 8343290 [details] [diff] [review]
> > part 2 - add memory used by mozJSComponentLoader to about:memory
> > 
> > Review of attachment 8343290 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: js/xpconnect/loader/mozJSComponentLoader.cpp
> > @@ +1391,5 @@
> > > +    size_t amount = aMallocSizeOf(this);
> > > +
> > > +    amount += aMallocSizeOf(location);
> > > +    if (js::GetObjectClass(obj)->flags & JSCLASS_DOM_GLOBAL &&
> > > +        mozilla::dom::HasProtoAndIfaceArray(obj)) {
> > 
> > It seems pretty weird to me to be counting this here. Shouldn't we be
> > counting this whenever we count all of the other per-{compartment,global}
> > data like the XPCWrappedNativeScope?
> 
> Oh, hm, probably!  So this:
> 
> http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/xpcprivate.
> h#1210
> 
> is the actual object that we need to be measuring with this?  Do we need to
> do some sort of uniqueness check here for B2G's One Global To Rule Them All
> scheme?

Hm, measuring this and not the loader says that we're double-counting the cache: once for nsGlobalWindow and once for the XPConnect scope.  We don't get double-counting if we measure in the loader.
(In reply to Nathan Froyd (:froydnj) from comment #7)
> (In reply to Bobby Holley (:bholley) from comment #5)
> > Comment on attachment 8343290 [details] [diff] [review]
> > part 2 - add memory used by mozJSComponentLoader to about:memory
> > 
> > Review of attachment 8343290 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: js/xpconnect/loader/mozJSComponentLoader.cpp
> > @@ +1391,5 @@
> > > +    size_t amount = aMallocSizeOf(this);
> > > +
> > > +    amount += aMallocSizeOf(location);
> > > +    if (js::GetObjectClass(obj)->flags & JSCLASS_DOM_GLOBAL &&
> > > +        mozilla::dom::HasProtoAndIfaceArray(obj)) {
> > 
> > It seems pretty weird to me to be counting this here. Shouldn't we be
> > counting this whenever we count all of the other per-{compartment,global}
> > data like the XPCWrappedNativeScope?
> 
> Oh, hm, probably!  So this:
> 
> http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/xpcprivate.
> h#1210
> 
> is the actual object that we need to be measuring with this?  Do we need to
> do some sort of uniqueness check here for B2G's One Global To Rule Them All
> scheme?

There is exactly one XPCWrappedNativeScope per compartment (module some JSD weirdness). And there is exactly one CompartmentPrivate per compartment (period). So we should put any per-global memory reporting on one of those. CompartmentPrivate seems to have one already.

(In reply to Nathan Froyd (:froydnj) from comment #8)
> Hm, measuring this and not the loader says that we're double-counting the
> cache: once for nsGlobalWindow and once for the XPConnect scope.  We don't
> get double-counting if we measure in the loader.

We should not be measuring it on nsGlobalWindow.
Flags: needinfo?(bobbyholley+bmo)
Slightly modified part 1, re-asking for review.  I think it's valuable to
have things split up as much as possible in about:memory, hence the extra
categories here.
Attachment #8343288 - Attachment is obsolete: true
Attachment #8343880 - Flags: review?(bobbyholley+bmo)
We don't measure the global object cache memory anymore; those bits have
been moved to parts 3 - 5.
Attachment #8343290 - Attachment is obsolete: true
Attachment #8343881 - Flags: review?(n.nethercote)
Attachment #8343881 - Flags: review?(bobbyholley+bmo)
In the spirit of fine-grained categories, I'd like to measure the scope
memory usage separate from the proto and iface cache memory usage.  We
already have a separate line-item for the proto and iface cache from bug
922094, so I don't think this is a controversial idea.  No functional
changes intended.

This patch prepares for that by changing the interface used by
XPCWrappedNativeScope for measuring memory.
Attachment #8343883 - Flags: review?(bobbyholley+bmo)
bholley says we should be measuring global object proto and iface cache
usage in xpconnect.  Sounds plausible enough, so let's not measure it
in nsGlobalWindow.  As this is a pretty straightforward backout of
bug 922094, I'm going to r+ this myself; anybody who objects can point
out flaws in the backout.
Attachment #8343885 - Flags: review+
Finally, let's measure the proto and iface cache memory and attribute it
to XPConnect.
Attachment #8343886 - Flags: review?(n.nethercote)
Attachment #8343886 - Flags: review?(bobbyholley+bmo)
Comment on attachment 8343880 [details] [diff] [review]
part 1 - split explicit/jsconnect into its constituent parts

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

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

Splitting things up is fine, but can you please move the measurement-taking back to here, to maintain the two-step "measure then report" structure of this function?  Thanks.
Attachment #8343880 - Flags: review+
Comment on attachment 8343881 [details] [diff] [review]
part 2 - add memory used by mozJSComponentLoader to about:memory

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

Do you have some example numbers from the new reports?

::: js/xpconnect/loader/mozJSComponentLoader.cpp
@@ +575,5 @@
> +                                                   ModuleEntry* const& aData,
> +                                                   MallocSizeOf aMallocSizeOf, void*)
> +{
> +    return aKey.SizeOfExcludingThisIfUnshared(aMallocSizeOf) +
> +        aData->SizeOfIncludingThis(aMallocSizeOf);

Ah, the data is |ModuleEntry*| so IncludingThis is appropriate.

@@ +595,5 @@
> +
> +    amount += mModules.SizeOfExcludingThis(DataEntrySizeOfExcludingThis, aMallocSizeOf, nullptr);
> +    amount += mImports.SizeOfExcludingThis(ClassEntrySizeOfExcludingThis, aMallocSizeOf, nullptr);
> +    amount += mInProgressImports.SizeOfExcludingThis(DataEntrySizeOfExcludingThis, aMallocSizeOf, nullptr);
> +    amount += mThisObjects.SizeOfExcludingThis(nullptr, aMallocSizeOf, nullptr);

The nullptr 3rd args here can all be removed.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +2725,5 @@
>                   "Memory used by XPConnect scopes.");
>  
> +    mozJSComponentLoader* loader = mozJSComponentLoader::Get();
> +    size_t jsComponentLoaderSize = loader ? 0 :
> +        loader->SizeOfIncludingThis(JSMallocSizeOf);

Please switch the 2nd and 3rd args of the ternary operator :)

Also, as in the previous patch, please move this up in order to keep the measurement and reporting stages separate.
Attachment #8343881 - Flags: review?(n.nethercote) → review+
Comment on attachment 8343883 [details] [diff] [review]
part 3 - prepare for measuring multiple things from XPCWrappedNativeScope

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

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ -2722,3 @@
>  
>      REPORT_BYTES(NS_LITERAL_CSTRING("explicit/xpconnect/scopes"),
> -                 KIND_HEAD, scopes,

KIND_HEAD?  That should be fixed in whichever earlier patch introduced it, to avoid having a non-compiling revision in the tree :)

::: js/xpconnect/src/xpcprivate.h
@@ +1143,5 @@
>      void
>      DebugDump(int16_t depth);
>  
> +    struct ScopeSizeInfo {
> +        ScopeSizeInfo() : mScopeAndMapSize(0) {}

One trick you could do (if you want) is to have a mMallocSizeOf member, so you only have to pass one argument to all the relevant functions.

@@ +1149,5 @@
> +        size_t mScopeAndMapSize;
> +    };
> +
> +    static void
> +    SizeOfAllScopesIncludingThis(mozilla::MallocSizeOf mallocSizeOf,

A convention I've started using is to use an "Add" prefix on the names of functions that increment sizes.  (Some parts of the code have a mixture of incrementing and non-incrementing SizeOf functions, and this convention really helps make things clear.)  So this should be AddSizeOfAllScopesIncludingThis().

@@ +1153,5 @@
> +    SizeOfAllScopesIncludingThis(mozilla::MallocSizeOf mallocSizeOf,
> +                                 ScopeSizeInfo* scopeSizeInfo);
> +
> +    void
> +    SizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf,

And this should be AddSizeOfIncludingThis().
Attachment #8343883 - Flags: review+
Comment on attachment 8343886 [details] [diff] [review]
part 5 - measure the proto and iface cache from within xpconnect

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

Thanks for doing this.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +2723,5 @@
>  
>      REPORT_BYTES(NS_LITERAL_CSTRING("explicit/xpconnect/scopes"),
>                   KIND_HEAP, sizeInfo.mScopeAndMapSize,
>                   "Memory used by XPConnect scopes.");
> +    REPORT_BYTES(NS_LITERAL_CSTRING("explicit/xpconnect/proto-iface-cache"),

Is the "xpconnect" sub-tree the best place for this?  Previously it was in the "dom" sub-tree.  Also, all the caches are summed to this single value whereas they used to be distinguished by window.  Maybe that's not a problem.

I don't have firm opinions here, just some things worth thinking about.
Attachment #8343886 - Flags: review?(n.nethercote) → review+
> There is exactly one XPCWrappedNativeScope per compartment (module some JSD
> weirdness). And there is exactly one CompartmentPrivate per compartment
> (period). So we should put any per-global memory reporting on one of those.
> CompartmentPrivate seems to have one already.

I'm not sure what's best for this case... but plenty of other per-global stuff gets reported elsewhere, e.g. all the DOM stuff.  And doing it here forces the cache sizes to be summed into a single number.
Example numbers from a single-window new session, close-to-new profile:

├───3.07 MB (04.09%) -- xpconnect
│   ├──2.06 MB (02.75%) ── proto-iface-cache
│   ├──0.75 MB (01.01%) ── scopes
│   └──0.25 MB (00.34%) -- (2 tiny)
│      ├──0.25 MB (00.34%) ── runtime
│      └──0.00 MB (00.00%) ── js-component-loader

About 75MB total memory usage (according to about:memory).

Example numbers from a old session with a number of tabs and windows (looks like ~200ish tabs, 98%-ish of them not yet restored):

├───11.31 MB (04.24%) -- xpconnect
│   ├───7.14 MB (02.67%) ── proto-iface-cache
│   ├───2.86 MB (01.07%) ── scopes
│   └───1.32 MB (00.49%) -- (2 tiny)
│       ├──1.32 MB (00.49%) ── runtime
│       └──0.00 MB (00.00%) ── js-component-loader

About 266MB total memory usage (according to about:memory)

I wonder if the proto-iface cache really needs to be an array; I can't imagine that many members are really going to be hit in your typical scope.  Some webpages, sure, but not all of them.  Going for a balanced tree or something to reduce memory usage in the average case might be worth investigating.
Attachment #8343880 - Flags: review?(bobbyholley+bmo) → review+
Attachment #8343881 - Flags: review?(bobbyholley+bmo) → review+
Attachment #8343883 - Flags: review?(bobbyholley+bmo) → review+
Attachment #8343886 - Flags: review?(bobbyholley+bmo) → review+
(In reply to Nathan Froyd (:froydnj) from comment #20)
> Example numbers from a single-window new session, close-to-new profile:
> │   ├──2.06 MB (02.75%) ── proto-iface-cache
memory usage (according to about:memory).

> Example numbers from a old session with a number of tabs and windows (looks
> like ~200ish tabs, 98%-ish of them not yet restored):
> │   ├───7.14 MB (02.67%) ── proto-iface-cache

> I wonder if the proto-iface cache really needs to be an array; I can't
> imagine that many members are really going to be hit in your typical scope. 
> Some webpages, sure, but not all of them.  Going for a balanced tree or
> something to reduce memory usage in the average case might be worth
> investigating.

Yeah, that does seem to be a somewhat unfortunate amount of memory to be using for this. Is the perf of lookups here ever critical? It seems like we would benefit from a denser representation...
Flags: needinfo?(bzbarsky)
The perf of lookups here is critical for the performance of JS-wrapping.  One of the big speedups of WebIDL bindings over XPConnect ones is that we made the lookup of the prototype object not take approximately forever.

That said, some sort of balanced tree might not be too bad.  It would still be way better than the xpconnect setup.
Flags: needinfo?(bzbarsky)
Also, it should be simple to test the performance here: something like createElement or new Event or some such ought to be worst-cases for wrapping performance.
OK - Nathan, can you file a bug and mark it for memshrink triage?
Flags: needinfo?(nfroyd)
(In reply to Bobby Holley (:bholley) from comment #24)
> OK - Nathan, can you file a bug and mark it for memshrink triage?

Done: bug 948445.
Flags: needinfo?(nfroyd)
Assignee: nobody → nfroyd
You need to log in before you can comment on or make changes to this bug.