Re: testing HS/SR - 1 vs 2 performance - Mailing list pgsql-hackers

On Wed, 2010-04-21 at 15:20 +0300, Heikki Linnakangas wrote:
> Simon Riggs wrote:
> > On Sun, 2010-04-18 at 08:24 +0100, Simon Riggs wrote:
> >> On Sat, 2010-04-17 at 18:52 -0400, Tom Lane wrote:
> >>> Simon Riggs <simon@2ndQuadrant.com> writes:
> >>>> What I'm not clear on is why you've used a spinlock everywhere when only
> >>>> weak-memory thang CPUs are a problem. Why not have a weak-memory-protect
> >>>> macro that does does nada when the hardware already protects us? (i.e. a
> >>>> spinlock only for the hardware that needs it).
> >>> Well, we could certainly consider that, if we had enough places where
> >>> there was a demonstrable benefit from it.  I couldn't measure any real
> >>> slowdown from adding a spinlock in that sinval code, so I didn't propose
> >>> doing so at the time --- and I'm pretty dubious that this code is
> >>> sufficiently performance-critical to justify the work, either.
> >> OK, I'll put a spinlock around access to the head of the array.
> > 
> > v2 patch attached
> 
> The locking seems overly complex to me. 

> Looking at
> KnownAssignedXidsAdd(), isn't it possible for two processes to call it
> concurrently in exclusive_lock==false mode and get the same 'head'
> value, and step on each others toes? I guess KnownAssignedXidsAdd() is
> only called by the startup process, but it seems like an accident
> waiting to happen.

Not at all. That assumption is also used elsewhere, so it is safe to use
here.

> Spinlocks are fast, if you have to add an if-branch to decide whether to
> acquire it, I suspect you've lost any performance gain to be had
> already.

I think you're misreading the code. One caller already holds exclusive
lock, one does not. The if test is to determine whether to acquire the
lock or not.

> Let's keep it simple. And acquiring ProcArrayLock in exclusive
> mode while adding entries to the array seems OK to me as well. It only
> needs to be held very briefly, and getting this correct and keeping it
> simple is much more important at this point than squeezing out every
> possible CPU cycle from the system.

I don't understand what you're saying: you say I'm wasting a few cycles
on an if test and should change that, but at the same time you say I
shouldn't worry about a few cycles.

> Just require that the caller holds ProcArrayLock in exclusive mode when
> calling an operation that modifies the array, and in shared mode when
> reading it.

The point of the code you're discussing is to remove the exclusive lock
requests, not to save a few cycles. Spinlocks are fast, as you say.

Exclusive lock requests block under a heavy load of shared lock holders,
we know that already. It is worth removing the contention that can occur
by minimising the number of exclusive locks required. This patch shows
how and I don't see any reason from what you have said to avoid
committing it. I'm willing to hear some sound reasons, if any exist.

-- Simon Riggs           www.2ndQuadrant.com