Re: [HACKERS] Fix performance degradation of contended LWLock on NUMA

Hi,

On 07/18/2017 01:20 PM, Sokolov Yura wrote:
> I'm sending rebased version with couple of one-line tweaks.
> (less skip_wait_list on shared lock, and don't update spin-stat on 
> aquiring)
> 

I have been running this patch on a 2S/28C/56T/256Gb w/ 2 x RAID10 SSD 
setup (1 to 375 clients on logged tables).

I'm seeing

-M prepared: Up to 11% improvement
-M prepared -S: No improvement, no regression ("noise")
-M prepared -N: Up to 12% improvement

for all runs the improvement shows up the closer you get to the number 
of CPU threads, or above. Although I'm not seeing the same improvements 
as you on very large client counts there are definitely improvements :)

Some comments:
==============

lwlock.c:
---------
+ * This race is avoiding by taking lock for wait list using CAS with a old
+ * state value, so it could succeed only if lock is still held. 
Necessary flag
+ * is set with same CAS.
+ *
+ * Inside LWLockConflictsWithVar wait list lock is reused to protect 
variable
+ * value. So first it is acquired to check variable value, then flags are
+ * set with second CAS before queueing to wait list to ensure lock were not
+ * released yet.
 * This race is avoided by taking a lock for the wait list using CAS 
with the old * state value, so it would only succeed if lock is still held. 
Necessary flag * is set using the same CAS. * * Inside LWLockConflictsWithVar the wait list lock is reused to 
protect the variable * value. First the lock is acquired to check the variable value, then 
flags are * set with a second CAS before queuing to the wait list in order to 
ensure that the lock was not * released yet.


+static void
+add_lwlock_stats_spin_stat(LWLock *lock, SpinDelayStatus *delayStatus)

Add a method description.

+            /*
+             * This barrier is never needed for correctness, and it is no-op
+             * on x86. But probably first iteration of cas loop in
+             * ProcArrayGroupClearXid will succeed oftener with it.
+             */

* "more often"

+static inline bool
+LWLockAttemptLockOrQueueSelf(LWLock *lock, LWLockMode mode, LWLockMode 
waitmode)

I'll leave it to the Committer to decide if this method is too big to be 
"inline".

+    /*
+     * We intentionally do not call finish_spin_delay here, cause loop above
+     * usually finished in queueing into wait list on contention, and doesn't
+     * reach spins_per_delay (and so, doesn't sleep inside of
+     * perform_spin_delay). Also, different LWLocks has very different
+     * contention pattern, and it is wrong to update spin-lock statistic based
+     * on LWLock contention.
+     */

/* * We intentionally do not call finish_spin_delay here, because the 
loop above * usually finished by queuing into the wait list on contention, and 
doesn't * reach spins_per_delay thereby doesn't sleep inside of * perform_spin_delay. Also, different LWLocks has very
different* contention pattern, and it is wrong to update spin-lock statistic based * on LWLock contention. */
 


s_lock.c:
---------
+        if (status->spins == 0)
+            /* but we didn't spin either, so ignore */
+            return;

Use { } for the if, or move the comment out of the nesting for readability.

Open questions:
---------------
* spins_per_delay as extern
* Calculation of skip_wait_list


You could run the patch through pgindent too.

Passes make check-world.

Status: Waiting on Author

Best regards, Jesper


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers