Closed
Bug 946781
Opened 11 years ago
Closed 11 years ago
need a memory reporter for mozJSComponentLoader
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: froydnj, Assigned: froydnj)
References
(Blocks 1 open bug)
Details
Attachments
(5 files, 2 obsolete files)
(deleted),
patch
|
bholley
:
review+
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bholley
:
review+
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bholley
:
review+
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bholley
:
review+
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
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?
Assignee | ||
Comment 2•11 years ago
|
||
(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...
Assignee | ||
Comment 3•11 years ago
|
||
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)
Assignee | ||
Comment 4•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #8343288 -
Flags: review?(bobbyholley+bmo) → review+
Comment 5•11 years ago
|
||
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 6•11 years ago
|
||
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)
Assignee | ||
Comment 7•11 years ago
|
||
(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)
Assignee | ||
Comment 8•11 years ago
|
||
(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.
Comment 9•11 years ago
|
||
(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)
Assignee | ||
Comment 10•11 years ago
|
||
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)
Assignee | ||
Comment 11•11 years ago
|
||
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)
Assignee | ||
Comment 12•11 years ago
|
||
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)
Assignee | ||
Comment 13•11 years ago
|
||
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+
Assignee | ||
Comment 14•11 years ago
|
||
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 15•11 years ago
|
||
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 16•11 years ago
|
||
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 17•11 years ago
|
||
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 18•11 years ago
|
||
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+
Comment 19•11 years ago
|
||
> 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.
Assignee | ||
Comment 20•11 years ago
|
||
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.
Updated•11 years ago
|
Attachment #8343880 -
Flags: review?(bobbyholley+bmo) → review+
Updated•11 years ago
|
Attachment #8343881 -
Flags: review?(bobbyholley+bmo) → review+
Updated•11 years ago
|
Attachment #8343883 -
Flags: review?(bobbyholley+bmo) → review+
Updated•11 years ago
|
Attachment #8343886 -
Flags: review?(bobbyholley+bmo) → review+
Comment 21•11 years ago
|
||
(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)
Comment 22•11 years ago
|
||
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)
Comment 23•11 years ago
|
||
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.
Comment 24•11 years ago
|
||
OK - Nathan, can you file a bug and mark it for memshrink triage?
Flags: needinfo?(nfroyd)
Assignee | ||
Comment 25•11 years ago
|
||
(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 | ||
Comment 26•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/529a6786d705
https://hg.mozilla.org/integration/mozilla-inbound/rev/1949b601ca45
https://hg.mozilla.org/integration/mozilla-inbound/rev/223cd199faa2
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9149f2fc278
Flags: in-testsuite-
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → nfroyd
Comment 27•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/529a6786d705
https://hg.mozilla.org/mozilla-central/rev/1949b601ca45
https://hg.mozilla.org/mozilla-central/rev/223cd199faa2
https://hg.mozilla.org/mozilla-central/rev/f9149f2fc278
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in
before you can comment on or make changes to this bug.
Description
•