Re: Addressing buffer private reference count scalability issue

Hi,

On 2026-03-08 16:09:07 +0000, Alexandre Felipe wrote:
> 2.
>
> Refactoring reference counting: Before starting to change code and
> potentially breaking things I considered prudent to isolate it to limit the
> damage. This code was part of a 8k+ LOC file.

Not allowing, at least the fast paths, to be inlined will make the most common
cases measurably slower, I've tried that.

> 3.
>
> Using simplehash: Simply replacing the HTAB for a simplehash, and adding
> a new set of macros SH_ENTRY_EMPTY, SH_MAKE_EMPTY, SH_MAKE_IN_USE.

Yea, we'll need to that. Peter Geoghegan has a patch for it as well.

> Here I assume that the buffer buffer sequence is independent enough from the
> array size, so I use the buffer as the hash key directly, omitting a hash
> function call.

I doubt that that's good enough. The hash table code relies on the bits being
well mixed, but you won't get that with buffer ids.

> 4.
>
> Compact PrivateRefCountEntry: The original implementation used a 4-byte
> key and 8-byte value. Reference count uses 32 bits, and it is unreasonable
> to expect one backend to pin the same buffer 1 billion times. The lock mode
> uses 32 bits but can only assume 4 values. So I packed them in one single
> uint32, giving 30 bits for count and 2 bits for lock mode. This makes the
> entries 8-byte long, on 64-bit CPUs this represents more than a 1/3
> reduction in memory. This makes the array aligned with the 64-bit words,
> copying one entry can be completed in one instruction, and every entry will
> be aligned.
> 5.

I'm rather sceptical that the overhead of needing to shift and mask is worth
it. I suspect we'll also want to add more state for each entry (e.g. I think
it may be worth getting rid of the io-in-progress resowner).

> REFCOUNT_ARRAY_ENTRIES=0: since the simplehash is basically some array
> lookup, it is worth trying to remove it completely and keep only the hash.
> For small values we would be trading a few branches for a buffer % SIZE,
> for the use case of prefetch where pin/unpin in a FIFO fashion, it will
> save an 8-entry array lookup, and some extra data moves.

I doubt that that's ok, in the vast majority of access we will have 0-2
buffers pinned. And even when we have pinned more buffers, it's *exceedingly*
common to access the same entry repeatedly (e.g. pin, lock, unlock, unpin),
adding a few cycles to each of those repeated accesses will quickly show up.

> From 56bfdd6d7296397a7b3f0b282e239fdc86d6580d Mon Sep 17 00:00:00 2001
> From: Alexandre Felipe <o(dot)alexandre(dot)felipe(at)gmail(dot)com>
> Date: Fri, 6 Mar 2026 17:15:44 +0000
> Subject: [PATCH 4/5] Compact PrivateRefCountEntry
>
> The previous implementation used an 8-bite (64-bit) entry to store
> a uint32 count and an uint32 lock mode. That is fine when we store
> the data separate from the key (buffer). But in the simplehash
> {key, value} are stored together, so each entry is 12-bytes.
> This is somewhat awkward as we have to either pad the entry to 16-bytes,
> or the access will be an alternating aligned/misaligned addreses.
>
> However, we are probably not going to need even 16-bits for the count
> and 2-bits is enough for the lock mode. So we can easily pack these
> int to a single uint32.

I wouldn't want to rely on a 16bit pin counter anyway.

> Incrementing/decrementing the count continue the same, just using
> 4 instead of 1, lock mode access will require one or two additional
> bitwise operations.
>
> No bit-shifts are required.

I don't know how that last sentence can be true, given that:

> -struct PrivateRefCountEntry
> +#define PRIVATEREFCOUNT_LOCKMODE_MASK 0x3
> +#define ONE_PRIVATE_REFERENCE 4 /* 1 << 2 */
> +
> +#define PrivateRefCountGetLockMode(d) ((BufferLockMode)((d) & PRIVATEREFCOUNT_LOCKMODE_MASK))
> +#define PrivateRefCountSetLockMode(d, m) ((d) = ((d) & ~PRIVATEREFCOUNT_LOCKMODE_MASK) | (m))
> +#define PrivateRefCountGetRefCount(d) ((int32)((d) >> 2))
> +#define PrivateRefCountIsZero(d) ((d) < ONE_PRIVATE_REFERENCE)

Involves shifts at least in PrivateRefCountGetRefCount()..

Greetings,

Andres Freund