Closed
Bug 1085740
Opened 10 years ago
Closed 10 years ago
Reduce heap churn caused by TempAllocator
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(1 file)
6.82 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
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.
![]() |
Assignee | |
Comment 1•10 years ago
|
||
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.)
Attachment #8508373 -
Flags: review?(jdemooij)
Comment 2•10 years ago
|
||
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.
Attachment #8508373 -
Flags: review?(jdemooij) → review+
![]() |
Assignee | |
Comment 3•10 years ago
|
||
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.
![]() |
Assignee | |
Comment 4•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/352cdd69b5c6
Comment 5•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/352cdd69b5c6
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
![]() |
Assignee | |
Updated•10 years ago
|
Depends on: cumulative-heap-profiling
You need to log in
before you can comment on or make changes to this bug.
Description
•