1005836 - Wasted space in NodePool::Block allocations

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[(Away)]

xul!GCGraphBuilder::NoteChild+0x42:
63e3a112 6818800200      push    28018h
63e3a117 ff1510e9cb64    call    dword ptr [xul!_imp__moz_xmalloc (64cbe910)]

That's just a hair over 40 pages, so it uses 41. It's not a huge waste, but I do see this number frequently in crash reports, so I assume there are many of these things.

Comment 1

[Andrew McCreight [:mccr8]]

Yeah, it would be nice to save some space if possible.  EdgePool would also be worth taking a look at.  The purple buffer also does some stuff with blocks, but that got fixed up, and some static asserts were added to make sure we don't regress slop.

(Despite the terrible name, GCGraphBuilder is part of the cycle collector, which is in the XPCOM component.)

Comment 2

[Andrew McCreight [:mccr8]]

from the purple buffer code:

        // Try to match the size of a jemalloc bucket, to minimize slop bytes.
        // - On 32-bit platforms sizeof(nsPurpleBufferEntry) is 12, so mEntries
        //   is 16,380 bytes, which leaves 4 bytes for mNext.
        // - On 64-bit platforms sizeof(nsPurpleBufferEntry) is 24, so mEntries
        //   is 32,544 bytes, which leaves 8 bytes for mNext.
        nsPurpleBufferEntry mEntries[1365];

        Block() : mNext(nullptr) {
            // Ensure Block is the right size (see above).
            static_assert(
                sizeof(Block) == 16384 ||       // 32-bit
                sizeof(Block) == 32768,         // 64-bit
                "ill-sized nsPurpleBuffer::Block"
            );
        }

Comment 3

[Nathan Froyd [:froydnj]]

I think this is coming from NodePool, which says:

class NodePool
{
private:
    enum { BlockSize = 8 * 1024 }; // could be int template parameter

    struct Block {
        // We create and destroy Block using NS_Alloc/NS_Free rather
        // than new and delete to avoid calling its constructor and
        // destructor.
        Block()  { NS_NOTREACHED("should never be called"); }
        ~Block() { NS_NOTREACHED("should never be called"); }

        Block* mNext;
        PtrInfo mEntries[BlockSize + 1]; // +1 to store last child of last node
    };

PtrInfos are five words on 32-bit platforms, 4 words on 64-bit.  So we have (8 * 1024 + 1) * 5 * 4 + 4 bytes being allocated for a NodePool::Block, which is...0x28018 bytes.

It would be super nice to shrink PtrInfo on 32-bit platforms, but that looks...very hard.  So recalibrating NodePool::BlockSize looks like the best bet here?

Comment 4

[(Away)]

(In reply to Nathan Froyd (:froydnj) from comment #3)
> I think this is coming from NodePool, which says:

Yeah, there was some heavy inlining. NodePool is the one I was looking at.

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd (:froydnj) from comment #3)
> It would be super nice to shrink PtrInfo on 32-bit platforms, but that
> looks...very hard.  So recalibrating NodePool::BlockSize looks like the best
> bet here?

It looks like trying to make NodePool::Block consume 24 pages (96KB) would fit ~4K+ PtrInfo structures without any slop on 32-bit platforms.  As a bonus, we'd be allocating smaller chunks of memory than we would before.  We'd have a tiny bit of slop on 64-bit platforms, but that seems unavoidable given the structure sizes.

Comment 6

[Andrew McCreight [:mccr8]]

If you want to know how many of these there are in practice, you can look at telemetry and add up CYCLE_COLLECTOR_VISITED_GCED and CYCLE_COLLECTOR_VISITED_REF_COUNTED.

Comment 7

[Andrew McCreight [:mccr8]]

It looks like the median is about 14000.

Comment 8

[Nicholas Nethercote [inactive]]

Nice catch, dmajor.

So who wants to take this? mccr8?

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: Avoid slop NodePool::Block allocations

I just went with the dead simple approach of reducing BlockSize. On 32-bit,
Block now takes up 39.997 pages. On 64-bit it takes up 63.994 pages. I can live
with that level of unused space.

I did some testing to see how many of these we have live at once. During normal
browsing, the peaks are typically 1 or 2, and occasionally spike as high as 10
or so. A pathological case is to load about:memory and then hit "Measure" over
and over; that gives seemingly unlimited increases due to the orphan DOM nodes
accumulating. I got "explicit" up to 1 GiB and the Block count over 400 before
I got bored and stopped.

So in practice, this is often reducing the CC memory spike a couple of 10s of
KiBs. Not bad for a trivial patch, and probably worth taking on Tarako.

Comment 10

[Nicholas Nethercote [inactive]]

BTW, having three different classes called |Block| in one file is no fun. I'm strongly tempted to rename them |EBlock|, |NBlock| and |PBlock|...

Comment 11

[Andrew McCreight [:mccr8]]

Comment on attachment 8422165 [details] [diff] [review]
Avoid slop NodePool::Block allocations

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

Thanks.  A static assert like we have elsewhere in this file might be nice.

Comment 12

[Nicholas Nethercote [inactive]]

Landed with static assertions added.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e915549578d

Comment 13

[Nicholas Nethercote [inactive]]

This is a small win, but it's a trivial change, and so worthwhile for Tarako.

Comment 14

[Olli Pettay [:smaug][[e-mail]] (vacation-ish -> Aug 1)]

(In reply to Nicholas Nethercote [:njn] from comment #10)
> BTW, having three different classes called |Block| in one file is no fun.

Cycle collector has still some odd looking code from the initial landing.
It could be cleaned up a bit.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2e915549578d

Comment 16

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/b850a55fe660