Re: ResourceOwner refactoring

On 25/10/2023 21:07, Andres Freund wrote:
> It's not too awful to have it be in this order:
> 
> struct ResourceOwnerData {
>          ResourceOwner              parent;               /*     0     8 */
>          ResourceOwner              firstchild;           /*     8     8 */
>          ResourceOwner              nextchild;            /*    16     8 */
>          const char  *              name;                 /*    24     8 */
>          _Bool                      releasing;            /*    32     1 */
>          _Bool                      sorted;               /*    33     1 */
>          uint8                      narr;                 /*    34     1 */
>          uint8                      nlocks;               /*    35     1 */
>          uint32                     nhash;                /*    36     4 */
>          uint32                     capacity;             /*    40     4 */
>          uint32                     grow_at;              /*    44     4 */
>          ResourceElem               arr[32];              /*    48   512 */
>          /* --- cacheline 8 boundary (512 bytes) was 48 bytes ago --- */
>          ResourceElem *             hash;                 /*   560     8 */
>          LOCALLOCK *                locks[15];            /*   568   120 */
> 
>          /* size: 688, cachelines: 11, members: 14 */
>          /* last cacheline: 48 bytes */
> };
> 
> Requires rephrasing a few comments to document that the lenghts are separate
> from the array / hashtable / locks, but otherwise...

Hmm, let's move 'capacity' and 'grow_at' after 'arr', too. They are only 
needed together with 'hash'.

> This reliably shows a decent speed improvement in my stress test [1], on the
> order of 5%.
> 
> [1] pgbench running
>     DO $$ BEGIN FOR i IN 1..5000 LOOP PERFORM abalance FROM pgbench_accounts WHERE aid = 17;END LOOP;END;$$;

I'm seeing similar results, although there's enough noise in the test 
that I'm sure how real the would be across different tests.

> At that point, the first array entry fits into the first cacheline. If we were
> to move parent, firstchild, nextchild, name further down the struct, three
> array entries would be on the first line. Just using the array presumably is a
> very common case, so that might be worth it?
> 
> I got less clear performance results with this one, and it's also quite
> possible it could hurt, if resowners aren't actually "used", just
> released. Therefore it's probably not worth it for now.

You're assuming that the ResourceOwner struct begins at a cacheline 
boundary. That's not usually true, we don't try to cacheline-align it. 
So while it's helpful to avoid padding and to keep frequently-accessed 
fields close to each other, there's no benefit in keeping them at the 
beginning of the struct.

-- 
Heikki Linnakangas
Neon (https://neon.tech)