Closed Bug 807883 Opened 12 years ago Closed 11 years ago

Add the PL_SizeOfArenaPoolExcludingPool function

Categories

(NSPR :: NSPR, enhancement, P2)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: n.nethercote, Assigned: n.nethercote)

Details

Attachments

(3 files, 2 obsolete files)

In several places in Gecko we have duplicated code that measures the size of a PLArena using an nsMallocSizeOfFun.  It would be nice to have proper functions for this to avoid the duplication.
This patch adds the new functions to NSPR, and also shows how they are used in
Gecko.  (Obviously this patch would need to be split in two for landing, but 
I've included both parts in case it's instructive.)
Attachment #677667 - Flags: review?(wtc)
Assignee: wtc → n.nethercote
wtc: review ping.  I'd like to know if this has any chance of making it into NSPR, or if I'll have to do it instead within Gecko.  Thanks.
Severity: normal → enhancement
OS: Linux → All
Priority: -- → P2
Hardware: x86_64 → All
Target Milestone: --- → 4.9.5
bsmith, you said that getting changes into NSPR wasn't that hard.  This bug suggests otherwise -- it's a very small patch that's been entirely ignored for six weeks.  At this point I'm contemplating doing this somewhere in Gecko, rather than NSPR, even though it won't really fit anywhere well.
Comment on attachment 677667 [details] [diff] [review]
Add PL_SizeOfArenaPool{In,Ex}cludingPool.

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

r=wtc. Sorry about the delay in reviewing this patch.

This patch seems fine. I suggest that we provide only PL_SizeOfArenaPoolExcludingPool.
It would be helpful to document what the new functions can be used for. It seems like
an esoteric, advanced feature.

Why does the mallocSizeOf function need to be passed in? Why can't NSPR figure out
the right function to use?

::: nsprpub/lib/ds/plarena.c
@@ +349,5 @@
> +    PLArenaPool *pool, PLMallocSizeOfFun mallocSizeOf)
> +{
> +    return mallocSizeOf(pool) +
> +           PL_SizeOfArenaPoolExcludingPool(pool, mallocSizeOf);
> +}

For a minimal API, it seems that only one of these two functions
needs to be provided because their difference is mallocSizeOf(pool),
which the caller can call himself.

I would expect that the inclusive function, PL_SizeOfArenaPoolIncludingPool,
would be more useful, but in your patch you only use
PL_SizeOfArenaPoolExcludingPool. Why is that? Is it because the
PLArenaPool structure itself may not be heap allocated?
Attachment #677667 - Flags: review?(wtc) → review+
> This patch seems fine. I suggest that we provide only
> PL_SizeOfArenaPoolExcludingPool.
> It would be helpful to document what the new functions can be used for. It
> seems like
> an esoteric, advanced feature.
> 
> Why does the mallocSizeOf function need to be passed in? Why can't NSPR
> figure out the right function to use?

This API reflects the needs of Firefox's memory reporting infrastructure, where the mallocSizeOf function (a) measures the size, and (b) does some additional, Firefox-specific stuff (specifically, it ties in with our infrastructure that ensures that no heap blocks are measured twice).


> For a minimal API, it seems that only one of these two functions
> needs to be provided because their difference is mallocSizeOf(pool),
> which the caller can call himself.
>
> I would expect that the inclusive function, PL_SizeOfArenaPoolIncludingPool,
> would be more useful, but in your patch you only use
> PL_SizeOfArenaPoolExcludingPool. Why is that? Is it because the
> PLArenaPool structure itself may not be heap allocated?

Yes.  Within Firefox we have lots of functions like this, and in practice the IncludingThis and ExcludingThis version are both used frequently.  The IncludingThis function should be used if the pool is allocated by itself on the heap.  The ExcludingThis function should be used if the pool is allocated on the stack, or its a member in a struct/object that is allocated on the heap.  I can improve the comment to explain this.

If you really want to keep the API minimal, then the ExcludingPool function is the one that's needed, because the IncludingPool assumes that the pool is heap-allocated -- as you say, the caller can do mallocSizeOf(pool) themselves if necessary.  But having both functions makes the API nicer.  I can live with either;  please let me know which you prefer and I'll update the patch.  Thanks!
Comment on attachment 677667 [details] [diff] [review]
Add PL_SizeOfArenaPool{In,Ex}cludingPool.

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

::: modules/libpref/src/prefapi.cpp
@@ -820,4 @@
>      for (struct CallbackNode* node = gCallbacks; node; node = node->next) {
>          n += aMallocSizeOf(node);
>          n += aMallocSizeOf(node->domain);
>      }

Looking at this closer, I am not convinced that we need to add this
function to NSPR. It is an esoteric feature. The function replaces
a simple loop, that is very similar to this for loop on the
gCallbacks list.

Are you trying to avoid iterating over the PLArenas in a PLArenaPool?
(I.e., you consider that as NSPR implementation detail.)

Or do you want to create a function for a loop that is used in multiple
files? Would xpcom be a good home for this function?

http://mxr.mozilla.org/mozilla-central/search?string=first.next
Nicolas: please review the NSPR patch. I made some minor changes.

1. I checked the NSPR public headers. Most function pointer typedefs
use the "Fn" or "FN" suffix. So I named the typedef "PLMallocSizeFn".
Note that I removed "Of" from the typedef name (because the example
functions malloc_size and malloc_usable_size don't have "of" in their
names), but I kept "Of" in the argument name.

2. I only added PL_SizeOfArenaPoolExcludingPool. I believe the
PLArenaPool API discourages people from allocating the PLArenaPool
structure from the heap separately.

NOTE: do not check in the Mozilla-side patch until I have pushed
a new NSPR update to mozilla-central.

This MXR query shows two other Mozilla files could use the new
function:
http://mxr.mozilla.org/mozilla-central/search?string=first.next

Finally, when you check in the Mozilla-side patch, you need to
update mozilla/configure.in to require NSPR version 4.9.6.
Attachment #677667 - Attachment is obsolete: true
Attachment #712271 - Flags: review?(n.nethercote)
Attachment #712271 - Flags: checked-in+
I forgot to paste the NSPR CVS commit log:

Checking in plarena.c;
/cvsroot/mozilla/nsprpub/lib/ds/plarena.c,v  <--  plarena.c
new revision: 3.21; previous revision: 3.20
done
Checking in plarenas.h;
/cvsroot/mozilla/nsprpub/lib/ds/plarenas.h,v  <--  plarenas.h
new revision: 3.11; previous revision: 3.10
done
Checking in plds.def;
/cvsroot/mozilla/nsprpub/lib/ds/plds.def,v  <--  plds.def
new revision: 1.8; previous revision: 1.7
done
Status: NEW → ASSIGNED
Target Milestone: 4.9.5 → 4.9.6
NSPR_4_9_6_BETA1 pushed to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4ce303cca2f
Whiteboard: [leave open]
Attached patch Mozilla patchSplinter Review
Please check this in when NSPR_4_9_6_BETA1 has made it to
mozilla-central.
Attachment #712310 - Flags: review?(n.nethercote)
Attached patch Supplemental NSPR patch (obsolete) — Splinter Review
1. Declare the local variable |arena| as a const pointer. I noticed
this when I compared the NSPR code with the Mozilla code it replaces.

2. Change "return" back to "measure" in the comments for PLMallocSizeFn.
Originally I used "return" because I thought the size of a heap block
should be readily available but "measure" implies extra effort is needed
to get the size. But I saw you use "measure" in the comments for the
Mozilla code. Perhaps the mallocSizeOf function you are using really needs
to measure something that's not readily available to the memory allocator?
Attachment #712311 - Flags: review?(n.nethercote)
Comment on attachment 712271 [details] [diff] [review]
Add PL_SizeOfArenaPoolExcludingPool.

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

::: mozilla/nsprpub/lib/ds/plarena.c
@@ +349,5 @@
> +    PLArenaPool *pool, PLMallocSizeFn mallocSizeOf)
> +{
> +    /*
> +     * The first PLArena is within |pool|, so don't measure it.  Subsequent
> +     * PLArenas are separate and must measured.

"must be measured" -- you might as well fix that in the supplementary patch.
Attachment #712271 - Flags: review?(n.nethercote) → review+
Attachment #712311 - Flags: review?(n.nethercote) → review+
Attachment #712310 - Flags: review?(n.nethercote) → review+
Comment on attachment 712310 [details] [diff] [review]
Mozilla patch

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

My Firefox test build had a compilation error.

::: layout/base/nsPresArena.cpp
@@ +405,5 @@
>    size_t SizeOfIncludingThisFromMalloc(nsMallocSizeOfFun aMallocSizeOf) const
>    {
>      size_t n = aMallocSizeOf(this);
> +    n += PL_SizeOfArenaPoolExcludingPool(&mPool,
> +                                         aMallocSizeOf);

Since this is inside a const method, &mPool is a
const PLArenaPool * pointer, so I will need to change
the function prototype of PL_SizeOfArenaPoolExcludingPool.

Please don't check in the Mozilla-side patch until
I have pushed a new NSPR update (NSPR_4_9_6_BETA2).
I will do that on Monday or Tuesday, after my Firefox
build completes successfully.
Patch checked in on the NSPR trunk (NSPR 4.9.6).

Checking in plarena.c;
/cvsroot/mozilla/nsprpub/lib/ds/plarena.c,v  <--  plarena.c
new revision: 3.22; previous revision: 3.21
done
Checking in plarenas.h;
/cvsroot/mozilla/nsprpub/lib/ds/plarenas.h,v  <--  plarenas.h
new revision: 3.12; previous revision: 3.11
done
Attachment #712311 - Attachment is obsolete: true
Attachment #712448 - Flags: checked-in+
NSPR_4_9_6_BETA2 pushed to mozilla-inbound to add 'const' to the 'pool'
input parameter of PL_SizeOfArenaPoolExcludingPool:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a6bef63fdc0
Mozilla patch (attachment 712310 [details] [diff] [review]) pushed to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0acac77dd920

Nicolas: do you think we should also measure the PLArenas in the
arena_freelist?

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/lib/ds/plarena.c&rev=3.20&mark=20#20
Summary: Add PL_ArenaSizeOf{In,Ex}cludingThis functions → Add the PL_SizeOfArenaPoolExcludingPool function
Whiteboard: [leave open]
> Nicholas: do you think we should also measure the PLArenas in the
> arena_freelist?

That's a global free list that is shared between all PLArenaPools, right?  So there's nothing sensible that can be done with it when measuring a single PLArenaPool.  I guess we could add another function that measures just the free list, but it hasn't shown up as a big deal in any of the Firefox memory profiles I've looked at.

(BTW, I don't particularly like having a shared free list.  A while back I was scratching my head for some time because one PLArenaPool was getting chunks of completely the wrong size... it turned out it was because the free list recycles chunks without much regard for their size.)
https://hg.mozilla.org/mozilla-central/rev/0acac77dd920
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
(In reply to Nicholas Nethercote [:njn] from comment #19)
> I guess we could add another function that measures just the
> free list, but it hasn't shown up as a big deal in any of the
> Firefox memory profiles I've looked at.

Yes, this is what I meant to ask you. We don't need to add that
function if you don't need to measure the free list.
> That's a global free list that is shared between all PLArenaPools, right? 
> So there's nothing sensible that can be done with it when measuring a single
> PLArenaPool.  I guess we could add another function that measures just the
> free list, but it hasn't shown up as a big deal in any of the Firefox memory
> profiles I've looked at.

I just did a direct measurements that confirmed that the free list doesn't get very big in Firefox -- the only sizes I saw were zero (most of the time) and 4 KiB (the rest of the time.)

I see now that arenas are only put on the free list when PL_FreeArenaPool() is called, and that they are actually freed when PL_FinishArenaPool() is called.  So that's good.
You need to log in before you can comment on or make changes to this bug.