Re: ProcArrayGroupClearXid() compare-exchange style

[Amit Kapila]

On Tue, Oct 15, 2019 at 9:23 AM Noah Misch  wrote:
>
> ProcArrayGroupClearXid() has this:
>
> while (true)
> {
> nextidx = 
> pg_atomic_read_u32(>procArrayGroupFirst);
>
> ...
>
> if 
> (pg_atomic_compare_exchange_u32(>procArrayGroupFirst,
>   
>  ,
>   
>  (uint32) proc->pgprocno))
> break;
> }
>
> This, from UnpinBuffer(), is our more-typical style:
>
> old_buf_state = pg_atomic_read_u32(>state);
> for (;;)
> {
> ...
>
> if (pg_atomic_compare_exchange_u32(>state, 
> _buf_state,
>   
>  buf_state))
> break;
> }
>
> That is, we typically put the pg_atomic_read_u32() outside the loop.  After
> the first iteration, it is redundant with the side effect of
> pg_atomic_compare_exchange_u32().  I haven't checked whether this materially
> improves performance, but, for style, I would like to change it in HEAD.
>

+1.  I am not sure if it would improve performance as this whole
optimization was to reduce the number of attempts to acquire LWLock,
but definitely, it makes the code consistent.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

ProcArrayGroupClearXid() compare-exchange style

[Noah Misch]

ProcArrayGroupClearXid() has this:

while (true)
{
nextidx = pg_atomic_read_u32(>procArrayGroupFirst);

...

if 
(pg_atomic_compare_exchange_u32(>procArrayGroupFirst,

   ,

   (uint32) proc->pgprocno))
break;
}

This, from UnpinBuffer(), is our more-typical style:

old_buf_state = pg_atomic_read_u32(>state);
for (;;)
{
...

if (pg_atomic_compare_exchange_u32(>state, 
_buf_state,

   buf_state))
break;
}

That is, we typically put the pg_atomic_read_u32() outside the loop.  After
the first iteration, it is redundant with the side effect of
pg_atomic_compare_exchange_u32().  I haven't checked whether this materially
improves performance, but, for style, I would like to change it in HEAD.
diff --git a/src/backend/storage/ipc/procarray.c 
b/src/backend/storage/ipc/procarray.c
index 8abcfdf..3da5307 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -490,15 +490,15 @@ ProcArrayGroupClearXid(PGPROC *proc, TransactionId 
latestXid)
/* We should definitely have an XID to clear. */
Assert(TransactionIdIsValid(allPgXact[proc->pgprocno].xid));
 
/* Add ourselves to the list of processes needing a group XID clear. */
proc->procArrayGroupMember = true;
proc->procArrayGroupMemberXid = latestXid;
+   nextidx = pg_atomic_read_u32(>procArrayGroupFirst);
while (true)
{
-   nextidx = pg_atomic_read_u32(>procArrayGroupFirst);
pg_atomic_write_u32(>procArrayGroupNext, nextidx);
 
if 
(pg_atomic_compare_exchange_u32(>procArrayGroupFirst,

   ,

   (uint32) proc->pgprocno))
break;