On Wed, Oct 8, 2014 at 9:35 AM, Andres Freund <andres@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
