Hook xptiWorkingSet up to about:memory

RESOLVED FIXED in mozilla7

Status

()

Core
XPCOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: khuey, Assigned: khuey)

Tracking

(Blocks: 1 bug)

Trunk
mozilla7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Created attachment 539975 [details] [diff] [review]
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.
Attachment #539975 - Flags: review?(nnethercote)
Assignee: nobody → khuey
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.
Attachment #539975 - Flags: review?(nnethercote) → review+
(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!
http://hg.mozilla.org/integration/mozilla-inbound/rev/de3b70528526
Whiteboard: [inbound]
Pushed:
http://hg.mozilla.org/mozilla-central/rev/de3b70528526
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla7
Version: unspecified → Trunk
FWIW, I see the same amount of xpti-working-set in both the chrome and content processes in Fennec.  Does that sound right?
Yes, it should be exactly the same.
You need to log in before you can comment on or make changes to this bug.