1085740 - Reduce heap churn caused by TempAllocator

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [:njn] (On Long Service Leave)]

We allocate a *lot* of short-lived memory via TempAllocator. E.g. hundreds of
MiBs (cumulatively) just at browser start-up.

And TempAllocator is odd. It uses LifoAllocs that typically have 4 KiB or 8 KiB
chunks. But it also requires that there is at least 16 KiB of ballast at all
times. Furthermore, because getOrCreateChunk() factors in the BumpChunk header
size, when we request 16 KiB of ballast we end up getting 32 KiB chunks.

What this means is that if you create a new LifoAlloc with 4 KiB chunk size and
then create a TempAllocator that uses it, you'll get:

- a 4 KiB chunk at the start, which is created for the very first allocation;

- a 32 KiB chunk immediately afterwards, which is created as ballast after the
  first allocation;

- additional 32 KiB chunks after that, if any the preceding chunks fill up
  enough that there is less than 16 KiB spare.

And in practice, many (~75%) of the TempAllocator's don't allocate even fill up
their first chunk. So this is all pretty inefficient.

Comment 1

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Attachment: Reduce heap churn caused by TempAllocator

My patch changes things so that, where possible, TempAllocator have a chunk
size of 32 KiB and a ballast of 16 KiB. (There are some cases where a
TempAllocator uses a pre-existing LifoAlloc that uses 4 or 8 KiB chunks. These
account for only a small fraction of TempAllocator allocations, and I've left
them unchanged.)

This means all chunks are 32 KiB (except for oversized ones, as usual), and we
only allocate one of them to begin with. In the very common case where the
TempAllocator has a peak usage of 16 KiB or less, this avoids the 4 KiB chunk
allocation.

When I start the browser and load Gmail, TechCrunch and CNN, this reduces the
number of allocations done by LifoAlloc by ~10, and also reduces their
cumulative size by ~10%.

Another possibility would be to use 16 KiB chunks and 12 KiB of ballast. Then
the reduction in allocations would be ~8% (slightly less because in larger
TempAllocators we have to allocate more chunks) but the reduction in their
cumulative size would be ~30% (much higher because many TempAllocators allocate
less than 4 KiB). But I'm not sure how best to evaluate how much ballast is
necessary. Also, since these are short-lived, it's not clear if reducing the
average size of the allocations is an actual win. (In contrast, reducing the
number of allocations can only be a good thing.)

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8508373 [details] [diff] [review]
Reduce heap churn caused by TempAllocator

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

Thanks for cleaning this up!

::: js/src/jit/IonAllocPolicy.h
@@ +28,5 @@
> +    // KiB. And with a ballast of 16 KiB, a chunk size of 32 KiB works well,
> +    // because TempAllocators with a peak allocation size of less than 16 KiB
> +    // (which is most of them) only have to allocate a single chunk.
> +    static const size_t BallastSize;            // 16 KiB
> +    static const size_t PreferredLifoChunkSize; // 32 KiB

I think you can do:

    static const size_t BallastSize = 16 * 1024;

Because size_t is an integral type. See also TypeZone::TYPE_LIFO_ALLOC_PRIMARY_CHUNK_SIZE in jsinfer.h where we do this as well.

Comment 3

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Thank you for the fast review.

> > +    static const size_t BallastSize;            // 16 KiB
> > +    static const size_t PreferredLifoChunkSize; // 32 KiB
> 
> I think you can do:
> 
>     static const size_t BallastSize = 16 * 1024;
> 
> Because size_t is an integral type. See also
> TypeZone::TYPE_LIFO_ALLOC_PRIMARY_CHUNK_SIZE in jsinfer.h where we do this
> as well.

I thought that too, but if I do it for BallastSize I get this link error with clang/gold on Linux:

> /home/njn/moz/mi5/js/src/jit/IonAllocPolicy.h:68: error: undefined reference to 'js::jit::TempAllocator::BallastSize'

and for PreferredLifoChunkSize I get this link error:

> /home/njn/moz/mi5/js/src/vm/MallocProvider.h:161: error: undefined reference to 'js::jit::TempAllocator::PreferredLifoChunkSize'

I think TYPE_LIFO_ALLOC_PRIMARY_CHUNK_SIZE works because it is only referred to from .cpp files.

Comment 4

[Nicholas Nethercote [:njn] (On Long Service Leave)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/352cdd69b5c6

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/352cdd69b5c6