847248 - Reduce use of nsFixedSizeAllocator

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Nicholas Nethercote [inactive]]

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 [inactive]]

Attachment: (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 [inactive]]

Attachment: (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 [inactive]]

Attachment: (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 [inactive]]

Attachment: (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 [:bzbarsky]]

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 [:bzbarsky]]

Comment on attachment 720505 [details] [diff] [review]
(part 2) - Use a smaller chunk size in TimerEventAllocator.

r=me

Comment 7

[Boris Zbarsky [:bzbarsky]]

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 [inactive]]

Attachment: (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 [:bzbarsky]]

Comment on attachment 720576 [details] [diff] [review]
(part 4) - Remove all uses of nsFixedSizeAllocator from rdf/.

r=me

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 721044 [details] [diff] [review]
(part 5) - Remove nsFixedSizeAllocator from nsXULTemplateBuilder.

Going to pass this one to enn.

Comment 11

[Nicholas Nethercote [inactive]]

> (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 [inactive]]

Attachment: (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 [inactive]]

Attachment: (part 7) - Remove nsFixedSizeAllocator from nsXBLService.

And another one.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 721059 [details] [diff] [review]
(part 7) - Remove nsFixedSizeAllocator from nsXBLService.

r=me

Comment 15

[Nicholas Nethercote [inactive]]

Attachment: (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 [inactive]]

Attachment: (part 10) - Rewrite TimerEventAllocator and remove nsFixedSizeAllocator.

Comment 17

[Boris Zbarsky [:bzbarsky]]

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 [inactive]]

> 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 [inactive]]

Attachment: (part 10) - Rewrite TimerEventAllocator and remove nsFixedSizeAllocator.

Updated version for super-review.

Comment 20

[Nicholas Nethercote [inactive]]

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 [:bzbarsky]]

> 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 [inactive]]

> 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 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/47ada7ee45de
https://hg.mozilla.org/mozilla-central/rev/f3ad021e88f0

Comment 24

[Guilherme Lima]

Only the first 2 parts landed, right?

Comment 25

[Nicholas Nethercote [inactive]]

Attachment: (part 8) - Remove nsFixedSizeAllocator from xbl/.

Oh, I forgot to upload this patch.

Comment 26

[Nicholas Nethercote [inactive]]

Parts 3, 4, 5:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e353ac8efaa
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b136fd785b9
https://hg.mozilla.org/integration/mozilla-inbound/rev/723ecd18f898

Comment 27

[Nicholas Nethercote [inactive]]

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 [:bzbarsky]]

Comment on attachment 722809 [details] [diff] [review]
(part 8) - Remove nsFixedSizeAllocator from xbl/.

r=me

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9e353ac8efaa
https://hg.mozilla.org/mozilla-central/rev/8b136fd785b9
https://hg.mozilla.org/mozilla-central/rev/723ecd18f898
https://hg.mozilla.org/mozilla-central/rev/ed1bfd0c2af6
https://hg.mozilla.org/mozilla-central/rev/6ad1095ef858

Comment 30

[Nicholas Nethercote [inactive]]

Parts 8 and 9:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b01a2d4c6d09
https://hg.mozilla.org/integration/mozilla-inbound/rev/917e4ba6a846

Comment 31

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b01a2d4c6d09
https://hg.mozilla.org/mozilla-central/rev/917e4ba6a846

Comment 32

[Benjamin Smedberg]

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

[(no longer active)]

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 [inactive]]

> 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

[(no longer active)]

(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 [inactive]]

Part 10:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cd5d96f223c

That's the last patch.

Comment 37

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5cd5d96f223c

Comment 38

[[e-mail]]

(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 [:bzbarsky]]

If the structure can be memmoved and memcopied, probably none.