Last Comment Bug 847248 - Reduce use of nsFixedSizeAllocator
: Reduce use of nsFixedSizeAllocator
Status: RESOLVED FIXED
[MemShrink:P3]
:
Product: Core
Classification: Components
Component: General (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla22
Assigned To: Nicholas Nethercote [:njn] (on vacation until July 11)
:
Mentors:
Depends on: 847210 847236
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-03 18:23 PST by Nicholas Nethercote [:njn] (on vacation until July 11)
Modified: 2013-04-17 08:03 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
(part 1) - Improve documentation and reduce slop potential of nsFixedSizeAllocator. (8.89 KB, patch)
2013-03-03 18:47 PST, Nicholas Nethercote [:njn] (on vacation until July 11)
bzbarsky: review+
Details | Diff | Review
(part 2) - Use a smaller chunk size in TimerEventAllocator. (981 bytes, patch)
2013-03-03 18:53 PST, Nicholas Nethercote [:njn] (on vacation until July 11)
bzbarsky: review+
Details | Diff | Review
(part 3) - Remove the nsFixedSizeAllocator from nsTreeContentView. (7.06 KB, patch)
2013-03-03 19:00 PST, Nicholas Nethercote [:njn] (on vacation until July 11)
bzbarsky: review+
Details | Diff | Review
(part 4) - Remove all uses of nsFixedSizeAllocator from rdf/. (38.05 KB, patch)
2013-03-04 01:17 PST, Nicholas Nethercote [:njn] (on vacation until July 11)
bzbarsky: review+
Details | Diff | Review
(part 5) - Remove nsFixedSizeAllocator from nsXULTemplateBuilder. (20.31 KB, patch)
2013-03-04 19:57 PST, Nicholas Nethercote [:njn] (on vacation until July 11)
enndeakin: review+
Details | Diff | Review
(part 6) - Remove nsFixedSizeAllocator from MemoryElement. (16.89 KB, patch)
2013-03-04 21:39 PST, Nicholas Nethercote [:njn] (on vacation until July 11)
enndeakin: review+
Details | Diff | Review
(part 7) - Remove nsFixedSizeAllocator from nsXBLService. (6.84 KB, patch)
2013-03-04 21:54 PST, Nicholas Nethercote [:njn] (on vacation until July 11)
bzbarsky: review+
Details | Diff | Review
(part 9) - Remove nsFixedSizeAllocator from parser/htmlparser/. (19.16 KB, patch)
2013-03-05 16:41 PST, Nicholas Nethercote [:njn] (on vacation until July 11)
hsivonen: review+
Details | Diff | Review
(part 10) - Rewrite TimerEventAllocator and remove nsFixedSizeAllocator. (15.79 KB, patch)
2013-03-05 21:57 PST, Nicholas Nethercote [:njn] (on vacation until July 11)
bzbarsky: review+
Details | Diff | Review
(part 10) - Rewrite TimerEventAllocator and remove nsFixedSizeAllocator. (16.64 KB, patch)
2013-03-06 14:46 PST, Nicholas Nethercote [:njn] (on vacation until July 11)
ehsan: review+
benjamin: superreview+
Details | Diff | Review
(part 8) - Remove nsFixedSizeAllocator from xbl/. (15.09 KB, patch)
2013-03-08 08:11 PST, Nicholas Nethercote [:njn] (on vacation until July 11)
bzbarsky: review+
Details | Diff | Review

Description Nicholas Nethercote [:njn] (on vacation until July 11) 2013-03-03 18:23:49 PST
Most of the uses of nsFixedSizeAllocator add nothing but complexity to the code base.  They should be removed.  (Bug 847210 and bug 847236 are already open to remove two of the existing cases.)  TimerEventAllocator is an exception, but the sizes of the chunks it allocates are much bigger than necessary.

Better explanation of the circumstances when nsFixedSizeAllocator is useful would also be a good thing.
Comment 1 Nicholas Nethercote [:njn] (on vacation until July 11) 2013-03-03 18:47:45 PST
Created attachment 720503 [details] [diff] [review]
(part 1) - Improve documentation and reduce slop potential of nsFixedSizeAllocator.

This patch:

- Extends and cleans up the existing comments for nsFixedSizeAllocator.

- Excludes the |bucketSpace| from the chunk size passed to PL_InitArenaPool.
  When a power-of-two is passed to nsFixedSizeAllocator::Init(), the underlying
  plarena will now see the same size, and no slop will occur.  (Note that most
  of the existing nsFixedSizeAllocators do not yet pass in a power-of-two
  |aChunkSize|.  I'll fix that subsequently.)
Comment 2 Nicholas Nethercote [:njn] (on vacation until July 11) 2013-03-03 18:53:19 PST
Created attachment 720505 [details] [diff] [review]
(part 2) - Use a smaller chunk size in TimerEventAllocator.

TimerEventAllocator uses a chunk size of 1024 * sizeof(nsTimerEvent).  On
64-bit platforms this is 32 KiB, and on 32-bit platforms it is (I think) 16
KiB.

With 150 tabs open (http://gregor-wagner.com/tmp/mem) I couldn't get above 200
simultaneously live timers.  A starting chunk size that allows 1024 is
excessive, especially given that this is memory that isn't freed until
shutdown.  So I reduced it to 4096 bytes, which allows 127 timers per chunk on
64-bit bit, and 255 timers per chunk on 32-bit.
Comment 3 Nicholas Nethercote [:njn] (on vacation until July 11) 2013-03-03 19:00:06 PST
Created attachment 720506 [details] [diff] [review]
(part 3) - Remove the nsFixedSizeAllocator from nsTreeContentView.

This case is funny/sad.  The chunk size passed to the nsFixedSizeAllocator is
only 16 bytes, which means that we'll fit exactly one Row per chunk.  So the
nsFixedSizeAllocator is genuinely just a wrapper around malloc/new that
complicates things and wastes space (due to the chunk overhead).

This patch just rips out that nsFixedSizeAllocator.
Comment 4 Nicholas Nethercote [:njn] (on vacation until July 11) 2013-03-04 01:17:20 PST
Created attachment 720576 [details] [diff] [review]
(part 4) - Remove all uses of nsFixedSizeAllocator from rdf/.

This patch removes nsFixedSizeAllocator from rdf/, removing the Create/Destroy
methods in numerous classes and replacing calls to them with with calls to
vanilla new/delete.  It seems extremely unlikely this will affect performance
noticeably;  besides, AFAIK the RDF stuff is very obscure.

About the only non-mechanical thing in the patch is that I had to move some
hash table destruction code out of Assertion::Destroy into ~Assertion.

Hmm, I just realized that I haven't taken advantage of the fact that |new| is
infallible.  I could remove some post-|new| null checks if you think it's
worthwhile.
Comment 5 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-03-04 19:28:34 PST
Comment on attachment 720503 [details] [diff] [review]
(part 1) - Improve documentation and reduce slop potential of nsFixedSizeAllocator.

>+     *   chunk will be used to by the nsFixedSizeAllocator (or the underlying
>+     *   plarena) itself.

s/to by/by/

s/plarena/PLArena/.

r=me
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-03-04 19:29:07 PST
Comment on attachment 720505 [details] [diff] [review]
(part 2) - Use a smaller chunk size in TimerEventAllocator.

r=me
Comment 7 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-03-04 19:31:52 PST
Comment on attachment 720506 [details] [diff] [review]
(part 3) - Remove the nsFixedSizeAllocator from nsTreeContentView.

r=me, though worth filing a followup to make mRows into an nsTArray<nsAutoPtr<Row> >
Comment 8 Nicholas Nethercote [:njn] (on vacation until July 11) 2013-03-04 19:57:26 PST
Created attachment 721044 [details] [diff] [review]
(part 5) - Remove nsFixedSizeAllocator from nsXULTemplateBuilder.

This patch removes nsFixedSizeAllocator from nsXULTemplateBuilder.  Unlike the
previous patch, I kept the Create/Destroy methods because the Destroy() method
takes an argument.

This code appears to not be hot -- it didn't trigger when I started the browser
and viewed a couple of sites.  And the chunk size of 256 is small enough that
it's unlikely to have any performance/fragmentation advantages even if the code
was hotter.
Comment 9 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-03-04 21:04:26 PST
Comment on attachment 720576 [details] [diff] [review]
(part 4) - Remove all uses of nsFixedSizeAllocator from rdf/.

r=me
Comment 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-03-04 21:05:09 PST
Comment on attachment 721044 [details] [diff] [review]
(part 5) - Remove nsFixedSizeAllocator from nsXULTemplateBuilder.

Going to pass this one to enn.
Comment 11 Nicholas Nethercote [:njn] (on vacation until July 11) 2013-03-04 21:16:11 PST
> (part 3) - Remove the nsFixedSizeAllocator from nsTreeContentView.
> 
> r=me, though worth filing a followup to make mRows into an
> nsTArray<nsAutoPtr<Row> >

I filed bug 847790.
Comment 12 Nicholas Nethercote [:njn] (on vacation until July 11) 2013-03-04 21:39:47 PST
Created attachment 721056 [details] [diff] [review]
(part 6) - Remove nsFixedSizeAllocator from MemoryElement.

This patch removes nsFixedSizeAllocator from MemoryElement.  This time I got
rid of the Create/Destroy methods.  The relevant destructors are all virtual so
it should be fine.

Like the last patch, the chunk size is only 256, so the fixed-size allocator is
unlikely to be having any performance effect.
Comment 13 Nicholas Nethercote [:njn] (on vacation until July 11) 2013-03-04 21:54:29 PST
Created attachment 721059 [details] [diff] [review]
(part 7) - Remove nsFixedSizeAllocator from nsXBLService.

And another one.
Comment 14 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-03-04 22:11:09 PST
Comment on attachment 721059 [details] [diff] [review]
(part 7) - Remove nsFixedSizeAllocator from nsXBLService.

r=me
Comment 15 Nicholas Nethercote [:njn] (on vacation until July 11) 2013-03-05 16:41:23 PST
Created attachment 721533 [details] [diff] [review]
(part 9) - Remove nsFixedSizeAllocator from parser/htmlparser/.

The old HTML parser appears to be barely used.  This patch removes its use of
nsFixedSizeAllocator in a minimally-invasive fashion, by replacing
nsFixedSizeAllocator with a new nsDummyAllocator class which trivally wraps
malloc/free.

I could do a more extensive patch that gets rid of nsDummyAllocator, but it
doesn't seem worthwhile.
Comment 16 Nicholas Nethercote [:njn] (on vacation until July 11) 2013-03-05 21:57:28 PST
Created attachment 721562 [details] [diff] [review]
(part 10) - Rewrite TimerEventAllocator and remove nsFixedSizeAllocator.
Comment 17 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-03-06 06:34:47 PST
Comment on attachment 721562 [details] [diff] [review]
(part 10) - Rewrite TimerEventAllocator and remove nsFixedSizeAllocator.

>   void* Alloc(size_t aSize)

Worth asserting that aSize >= sizeof(void*), since we assume in Free that all our allocated chunks are at least that size.

r=me with that.
Comment 18 Nicholas Nethercote [:njn] (on vacation until July 11) 2013-03-06 14:06:01 PST
> Worth asserting that aSize >= sizeof(void*), since we assume in Free that
> all our allocated chunks are at least that size.

The real assumption is that |aSize == sizeof(nsTimerEvent)|, which is stronger.  I assert that in |nsTimerEvent::operator new|, but |TimerEventAllocator::Alloc| would be a better place for it.  I'll do that.
Comment 19 Nicholas Nethercote [:njn] (on vacation until July 11) 2013-03-06 14:46:04 PST
Created attachment 721905 [details] [diff] [review]
(part 10) - Rewrite TimerEventAllocator and remove nsFixedSizeAllocator.

Updated version for super-review.
Comment 20 Nicholas Nethercote [:njn] (on vacation until July 11) 2013-03-06 14:55:52 PST
Comment on attachment 721905 [details] [diff] [review]
(part 10) - Rewrite TimerEventAllocator and remove nsFixedSizeAllocator.

bsmedberg, I'm asking for super-review on the removal of nsFixedSizeAllocator.

It's a crappy allocator.  It's a mild extension to PLArenaPool -- it can handle freeing and subsequent recycling of objects.  It causes slop bytes by making non-power-of-two requests, but fortunately it's not used in places that allocate a lot of memory.

However, in bug 847210 it was causing us to hold onto several MBs of nsNodeInfo structs forever.  In bug 847236 it made the ETCI recycle pool much more complex, and required it to be flushed empty each time it became full.  In all but one of the cases in this bug, it had no beneficial effect but made the code substantially more complex -- vanilla malloc/free or new/delete was much simpler.  (I've removed ~700 lines of code as a result.)

The one case where it's genuinely helpful is in TimerEventAllocator, where the lack of contention with the main malloc lock is a performance win.  So I just changed TimerEventAllocator to be a one-off custom allocator, rather than using nsFixedSizeAllocator.  It was much simpler that way because it only has to handle a single size of object.

Like nsRecyclingAllocator, which I killed off in bug 715770, nsFixedSizeAllocator might have been useful in pre-jemalloc days, but no longer.  It's an attractive nuisance that needs to go!
Comment 21 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-03-06 17:38:03 PST
> The real assumption is that |aSize == sizeof(nsTimerEvent)|, which is stronger. 

What guarantees sizeof(nsTimerEvent) >= sizeof(void*)?  I know it is right now, but in general seems like a good thing to assert.  Statically, perhaps, since that's statically assertable.
Comment 22 Nicholas Nethercote [:njn] (on vacation until July 11) 2013-03-06 20:06:15 PST
> What guarantees sizeof(nsTimerEvent) >= sizeof(void*)?  I know it is right
> now, but in general seems like a good thing to assert.

Fair enough.  I'll add that too.
Comment 24 Guilherme Lima 2013-03-07 13:41:26 PST
Only the first 2 parts landed, right?
Comment 25 Nicholas Nethercote [:njn] (on vacation until July 11) 2013-03-08 08:11:24 PST
Created attachment 722809 [details] [diff] [review]
(part 8) - Remove nsFixedSizeAllocator from xbl/.

Oh, I forgot to upload this patch.
Comment 27 Nicholas Nethercote [:njn] (on vacation until July 11) 2013-03-11 15:05:03 PDT
Parts 6 and 7:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed1bfd0c2af6
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ad1095ef858
Comment 28 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-03-12 11:37:40 PDT
Comment on attachment 722809 [details] [diff] [review]
(part 8) - Remove nsFixedSizeAllocator from xbl/.

r=me
Comment 30 Nicholas Nethercote [:njn] (on vacation until July 11) 2013-03-12 19:56:20 PDT
Parts 8 and 9:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b01a2d4c6d09
https://hg.mozilla.org/integration/mozilla-inbound/rev/917e4ba6a846
Comment 32 Benjamin Smedberg [:bsmedberg] 2013-03-15 09:25:16 PDT
Comment on attachment 721905 [details] [diff] [review]
(part 10) - Rewrite TimerEventAllocator and remove nsFixedSizeAllocator.

I'm going to mark sr+ on this, but I think ehsan should at least rubberstamp the timer allocator bits since he has touched that code recently.
Comment 33 :Ehsan Akhgari (out sick) 2013-03-16 09:28:29 PDT
Comment on attachment 721905 [details] [diff] [review]
(part 10) - Rewrite TimerEventAllocator and remove nsFixedSizeAllocator.

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

::: xpcom/threads/nsTimerImpl.cpp
@@ +68,5 @@
> +// "freed" nsTimerEvents aren't truly freed, they're just put onto a free-list
> +// for later recycling.  So the amount of memory consumed will always be equal
> +// to the high-water mark consumption.  But nsTimerEvents are small and it's
> +// unusual to have more than a few hundred of them, so this shouldn't be a
> +// problem in practice.

It would be interesting to hook this up to the memory reporter infrastructure so that we can verify this in practice.

@@ +88,4 @@
>        mMonitor("TimerEventAllocator")
>    {
> +    PL_InitArenaPool(&mPool, "TimerEventPool", 4096, /* align = */ 0);
> +    mFirstFree = nullptr;

No need to re-assign null here.
Comment 34 Nicholas Nethercote [:njn] (on vacation until July 11) 2013-03-16 13:44:53 PDT
> It would be interesting to hook this up to the memory reporter
> infrastructure so that we can verify this in practice.

I did some ad hoc measurement while running MemBench (http://gregor-wagner.com/tmp/mem) which opens 150 tabs at once, and 202 was the maximum number of timers live at once.  And in DMD profiles I've never seen more than 32 KiB, which was the old chunk size.  So I don't think it's worth hooking up for something that rarely goes above 4 or 8 KiB.
Comment 35 :Ehsan Akhgari (out sick) 2013-03-16 14:09:49 PDT
(In reply to comment #34)
> > It would be interesting to hook this up to the memory reporter
> > infrastructure so that we can verify this in practice.
> 
> I did some ad hoc measurement while running MemBench
> (http://gregor-wagner.com/tmp/mem) which opens 150 tabs at once, and 202 was
> the maximum number of timers live at once.  And in DMD profiles I've never seen
> more than 32 KiB, which was the old chunk size.  So I don't think it's worth
> hooking up for something that rarely goes above 4 or 8 KiB.

Fair enough!
Comment 36 Nicholas Nethercote [:njn] (on vacation until July 11) 2013-03-17 18:59:21 PDT
Part 10:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cd5d96f223c

That's the last patch.
Comment 37 Ed Morley [:emorley] 2013-03-18 13:17:06 PDT
https://hg.mozilla.org/mozilla-central/rev/5cd5d96f223c
Comment 38 neil@parkwaycc.co.uk 2013-04-17 01:50:16 PDT
(In reply to Boris Zbarsky from comment #7)
> worth filing a followup to make mRows into an nsTArray<nsAutoPtr<Row> >

Out of interest, what are the pros and cons against nsTArray<Row> ?
Comment 39 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-04-17 08:03:59 PDT
If the structure can be memmoved and memcopied, probably none.

Note You need to log in before you can comment on or make changes to this bug.