Reduce use of nsFixedSizeAllocator

RESOLVED FIXED in mozilla22

Status

()

Core
General
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla22
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P3])

Attachments

(10 attachments, 1 obsolete attachment)

8.89 KB, patch
Details | Diff | Splinter Review
981 bytes, patch
Details | Diff | Splinter Review
7.06 KB, patch
Details | Diff | Splinter Review
38.05 KB, patch
Details | Diff | Splinter Review
20.31 KB, patch
Neil Deakin (mostly unavailable until September)
: review+
Details | Diff | Splinter Review
16.89 KB, patch
Neil Deakin (mostly unavailable until September)
: review+
Details | Diff | Splinter Review
6.84 KB, patch
Details | Diff | Splinter Review
19.16 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
16.64 KB, patch
Ehsan
: review+
Benjamin Smedberg
: superreview+
Details | Diff | Splinter Review
15.09 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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.)
Attachment #720503 - Flags: review?(bzbarsky)
(Assignee)

Comment 2

5 years ago
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.
Attachment #720505 - Flags: review?(bzbarsky)
(Assignee)

Comment 3

5 years ago
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.
Attachment #720506 - Flags: review?(bzbarsky)
(Assignee)

Comment 4

5 years ago
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.
Attachment #720576 - Flags: review?(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
Attachment #720503 - Flags: review?(bzbarsky) → review+
Comment on attachment 720505 [details] [diff] [review]
(part 2) - Use a smaller chunk size in TimerEventAllocator.

r=me
Attachment #720505 - Flags: review?(bzbarsky) → review+
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> >
Attachment #720506 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 8

5 years ago
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.
Attachment #721044 - Flags: review?(bzbarsky)
Comment on attachment 720576 [details] [diff] [review]
(part 4) - Remove all uses of nsFixedSizeAllocator from rdf/.

r=me
Attachment #720576 - Flags: review?(bzbarsky) → review+
Comment on attachment 721044 [details] [diff] [review]
(part 5) - Remove nsFixedSizeAllocator from nsXULTemplateBuilder.

Going to pass this one to enn.
Attachment #721044 - Flags: review?(bzbarsky) → review?(enndeakin)
(Assignee)

Comment 11

5 years ago
> (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.
(Assignee)

Comment 12

5 years ago
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.
Attachment #721056 - Flags: review?(enndeakin)
(Assignee)

Comment 13

5 years ago
Created attachment 721059 [details] [diff] [review]
(part 7) - Remove nsFixedSizeAllocator from nsXBLService.

And another one.
Attachment #721059 - Flags: review?(bzbarsky)
Comment on attachment 721059 [details] [diff] [review]
(part 7) - Remove nsFixedSizeAllocator from nsXBLService.

r=me
Attachment #721059 - Flags: review?(bzbarsky) → review+
Attachment #721044 - Flags: review?(enndeakin) → review+
Attachment #721056 - Flags: review?(enndeakin) → review+
(Assignee)

Updated

5 years ago
Whiteboard: [MemShrink] → [MemShrink:P3]
(Assignee)

Comment 15

5 years ago
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.
Attachment #721533 - Flags: review?(hsivonen)
(Assignee)

Comment 16

5 years ago
Created attachment 721562 [details] [diff] [review]
(part 10) - Rewrite TimerEventAllocator and remove nsFixedSizeAllocator.
Attachment #721562 - Flags: review?(bzbarsky)
Attachment #721533 - Flags: review?(hsivonen) → review+
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.
Attachment #721562 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 18

5 years ago
> 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.
(Assignee)

Comment 19

5 years ago
Created attachment 721905 [details] [diff] [review]
(part 10) - Rewrite TimerEventAllocator and remove nsFixedSizeAllocator.

Updated version for super-review.
(Assignee)

Updated

5 years ago
Attachment #721562 - Attachment is obsolete: true
(Assignee)

Comment 20

5 years ago
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!
Attachment #721905 - Flags: superreview?(benjamin)
> 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.
(Assignee)

Comment 22

5 years ago
> 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.
https://hg.mozilla.org/mozilla-central/rev/47ada7ee45de
https://hg.mozilla.org/mozilla-central/rev/f3ad021e88f0
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22

Comment 24

5 years ago
Only the first 2 parts landed, right?

Updated

5 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

5 years ago
Whiteboard: [MemShrink:P3] → [MemShrink:P3][leave open]
(Assignee)

Comment 25

5 years ago
Created attachment 722809 [details] [diff] [review]
(part 8) - Remove nsFixedSizeAllocator from xbl/.

Oh, I forgot to upload this patch.
Attachment #722809 - Flags: review?(bzbarsky)
(Assignee)

Comment 26

5 years ago
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
(Assignee)

Comment 27

5 years ago
Parts 6 and 7:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed1bfd0c2af6
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ad1095ef858
Comment on attachment 722809 [details] [diff] [review]
(part 8) - Remove nsFixedSizeAllocator from xbl/.

r=me
Attachment #722809 - Flags: review?(bzbarsky) → review+
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
(Assignee)

Comment 30

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

Comment 32

5 years ago
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.
Attachment #721905 - Flags: superreview?(benjamin)
Attachment #721905 - Flags: superreview+
Attachment #721905 - Flags: review?(ehsan)
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.
Attachment #721905 - Flags: review?(ehsan) → review+
(Assignee)

Comment 34

5 years ago
> 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.
(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!
(Assignee)

Comment 36

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

That's the last patch.
Whiteboard: [MemShrink:P3][leave open] → [MemShrink:P3]
https://hg.mozilla.org/mozilla-central/rev/5cd5d96f223c
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
(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> ?
If the structure can be memmoved and memcopied, probably none.
You need to log in before you can comment on or make changes to this bug.