664913 - Hook xptiWorkingSet up to about:memory

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

This is about a megabyte of memory on my machine.  Not enormous, and fixed for the life of the process, but doesn't hurt to report it anyways.

Comment 1

[Nicholas Nethercote [inactive]]

Comment on attachment 539975 [details] [diff] [review]
Patch

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

Looks fine.  1MB isn't much... is there any related memory usage -- other XPTArenas, maybe -- that could be incorporated?

::: xpcom/reflect/xptinfo/src/xptiWorkingSet.cpp
@@ +51,5 @@
> +}
> +
> +NS_MEMORY_REPORTER_IMPLEMENT(xptiWorkingSet,
> +                             "explicit/xpti-working-set",
> +                             MR_HEAP,

You're certain this is heap memory, I hope :)

@@ +52,5 @@
> +
> +NS_MEMORY_REPORTER_IMPLEMENT(xptiWorkingSet,
> +                             "explicit/xpti-working-set",
> +                             MR_HEAP,
> +                             "Memory used by the XPCOM typelib system",

Add a period after "system" for consistency with other descriptions.

::: xpcom/typelib/xpt/src/xpt_arena.c
@@ +339,5 @@
> +
> +XPT_PUBLIC_API(size_t)
> +XPT_SizeOfArena(XPTArena *arena)
> +{
> +    size_t s = sizeof(XPTArena);

I've been using 'n' elsewhere;  's' seems odd.  I'll let you decide whether to change.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #1)
> Comment on attachment 539975 [details] [diff] [review] [review]
> Patch
> 
> Review of attachment 539975 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> Looks fine.  1MB isn't much... is there any related memory usage -- other
> XPTArenas, maybe -- that could be incorporated?

Related?  Maybe, but not that I'm aware of.  This is the only non-transient XPTArena (we create others when reading in XPT files but they are destroyed after that finishes).

> ::: xpcom/reflect/xptinfo/src/xptiWorkingSet.cpp
> @@ +51,5 @@
> > +}
> > +
> > +NS_MEMORY_REPORTER_IMPLEMENT(xptiWorkingSet,
> > +                             "explicit/xpti-working-set",
> > +                             MR_HEAP,
> 
> You're certain this is heap memory, I hope :)

Yes.

> @@ +52,5 @@
> > +
> > +NS_MEMORY_REPORTER_IMPLEMENT(xptiWorkingSet,
> > +                             "explicit/xpti-working-set",
> > +                             MR_HEAP,
> > +                             "Memory used by the XPCOM typelib system",
> 
> Add a period after "system" for consistency with other descriptions.

Done.

> ::: xpcom/typelib/xpt/src/xpt_arena.c
> @@ +339,5 @@
> > +
> > +XPT_PUBLIC_API(size_t)
> > +XPT_SizeOfArena(XPTArena *arena)
> > +{
> > +    size_t s = sizeof(XPTArena);
> 
> I've been using 'n' elsewhere;  's' seems odd.  I'll let you decide whether
> to change.

Changed.

Thanks for the quick review!

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/de3b70528526

Comment 4

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/de3b70528526

Comment 5

[Nicholas Nethercote [inactive]]

FWIW, I see the same amount of xpti-working-set in both the chrome and content processes in Fennec.  Does that sound right?

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Yes, it should be exactly the same.