Re: [PATCH] bufmgr: tighten LWLock:BufferMapping on InvalidateBuffer

Hi,

On 2026-08-24 22:01:43 +0100, Alexandre Felipe wrote:
> There are no algorithmic changes. The changes are about
>
> 0002 having two distinct paths for LW_EXCLUSIVE and LW_SHARED,
> the rest is diminishing returns. And also added LWLockReleaseLast,
> added wrappers as inline functions in lwlock.h that compiles the
> current code base replacing the calls
> LWLockAcquire(lock, LW_EXCLUSIVE | LW_SHARED) by the
> LWLockAcquire(Exclusive|Shared)(lock)
>
>
>
> 0003 reading the mode from the LWLock state and saving space and time
> storing it in the held lwlocks array.
> 0004 well, bring LWLock(Acquire/Release)X external functions that take
> mode as a parameter (a baseline).
> 0005 Inlined LWLockAttemptLock and folded the loop, placing a single
> LWLockAttemptLock at the top of the loop followed by the common logic
> (previously at the end bottom of the function), then enqueue and continue
> or wait.

> From 7e2fe005525d59c4def5ac0f1060d3d4bfa43351 Mon Sep 17 00:00:00 2001
> From: Alexandre Felipe <o(dot)alexandre(dot)felipe(at)gmail(dot)com>
> Date: Mon, 17 Aug 2026 18:19:43 +0100
> Subject: [PATCH 1/5] Benchmark
>
> This adds a module for benchmarking LWLocks in a tight loop.

I don't believe that's a particulary interesting test. You don't normally take
a lock to then not do anything when covered by the lock.

> From 139f5d67266c94f96f3e88b056c297307efacb32 Mon Sep 17 00:00:00 2001
> From: Alexandre Felipe <o(dot)alexandre(dot)felipe(at)gmail(dot)com>
> Date: Mon, 17 Aug 2026 18:41:39 +0100
> Subject: [PATCH 2/5] LWLock fast paths
>
> inline LWLockAcquire(LWLock* , LWLockMode)
> +- extern LWLockAcquireShared(LWLock*)
> | +- fast LWLockAcquireCommon(LWLock*, LW_SHARED)
> | +- fast LWLockAttemptLock(LWLock*, LW_SHARED)
> | +- ... slow path
> +- extern LWLockACquireExclusive(LWLock*)
> +- fast LWLockAcquireCommon(LWLock*, LW_EXCLUSIVE)
> +- fast LWLockAttemptLock(LWLock*, LW_EXCLUSIVE)
> +- ... slow path
>
> LWLock tracking only stores (LWLock*), the mode can be inferred at
> any time from the LWLock state. since at any time either the number
> of exclusive locks or the number of shared locks on a LWLock must
> be zero. If a lock is currently held, one of them will be non-zero.

> Is reading the mode from the lock state in the hot path a concern?

Yes, that sounds like a bad idea. The lock state on a somewhat contended lock
tends to bounce very heavily between cores / nodes. Pulling it into shared
state before the atomic operation is a bad idea.

I think that's a clear no-go.

> This patch provides a LWLockRelease(LWLock, LWLockMode) that uses
> an architecture similar to LWLockAcquire.
>
> inline LWLockReleaseMode(LWLock* , LWLockMode)
> +- extern LWLockReleaseShared(LWLock*)
> | +- if(lock is at the top of held_lwlocks)
> | | fast LWLockReleaseCommon(LWLock*, LW_SHARED)
> | +- ... slow path
> +- extern LWLockReleaseExclusive(LWLock*)
> +- if(lock is at the top of held_lwlocks)
> | fast LWLockReleaseCommon(LWLock*, LW_EXCLUSIVE)
> +- ... slow path
>
> Is the top of the stack check in the hot path a concern?
> This patch provides a LWLockReleaseLast(LWLock*, LWLockMode),
> that only checks for the existence of a lock.

I doubt the gain of eliding am-i-on-the-top-of-the-stack check is worth it.
To prove that it is you would really need to show in a more realistic scenario
that the gain is worth the interface complexity.

Greetings,

Andres Freund