On Wed, Oct 8, 2014 at 9:35 AM, Andres Freund <and...@2ndquadrant.com> wrote: > 1) Convert PGPROC->lwWaitLink into a dlist. The old code was frail and > verbose. This also does: > * changes the logic in LWLockRelease() to release all shared lockers > when waking up any. This can yield some significant performance > improvements - and the fairness isn't really much worse than > before, > as we always allowed new shared lockers to jump the queue. > > * adds a memory pg_write_barrier() in the wakeup paths between > dequeuing and unsetting ->lwWaiting. That was always required on > weakly ordered machines, but f4077cda2 made it more urgent. I can > reproduce crashes without it.
I think it's a really bad idea to mix a refactoring change (like converting PGPROC->lwWaitLink into a dlist) with an attempted performance enhancement (like changing the rules for jumping the lock queue) and a bug fix (like adding pg_write_barrier where needed). I'd suggest that the last of those be done first, and perhaps back-patched. The current coding, using a hand-rolled list, touches shared memory fewer times. When many waiters are awoken at once, we clip them all out of the list at one go. Your revision moves them to a backend-private list one at a time, and then pops them off one at a time. The backend-private memory accesses don't seem like they matter much, but the shared memory accesses would be nice to avoid. Does LWLockUpdateVar's wake-up loop need a write barrier per iteration, or just one before the loop starts? How about commenting the pg_write_barrier() with the read-fence to which it pairs? + if(waiter->lwWaitMode == LW_EXCLUSIVE) Whitespace. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers