ProcArrayGroupClearXid() compare-exchange style
ProcArrayGroupClearXid() has this:
while (true)
{
nextidx = pg_atomic_read_u32(&procglobal->procArrayGroupFirst);
...
if (pg_atomic_compare_exchange_u32(&procglobal->procArrayGroupFirst,
&nextidx,
(uint32) proc->pgprocno))
break;
}
This, from UnpinBuffer(), is our more-typical style:
old_buf_state = pg_atomic_read_u32(&buf->state);
for (;;)
{
...
if (pg_atomic_compare_exchange_u32(&buf->state, &old_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.
Attachments:
compare-exchange-style-v1.patchtext/plain; charset=us-asciiDownload
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(&procglobal->procArrayGroupFirst);
while (true)
{
- nextidx = pg_atomic_read_u32(&procglobal->procArrayGroupFirst);
pg_atomic_write_u32(&proc->procArrayGroupNext, nextidx);
if (pg_atomic_compare_exchange_u32(&procglobal->procArrayGroupFirst,
&nextidx,
(uint32) proc->pgprocno))
break;
On Tue, Oct 15, 2019 at 9:23 AM Noah Misch <noah@leadboat.com> wrote:
ProcArrayGroupClearXid() has this:
while (true)
{
nextidx = pg_atomic_read_u32(&procglobal->procArrayGroupFirst);...
if (pg_atomic_compare_exchange_u32(&procglobal->procArrayGroupFirst,
&nextidx,
(uint32) proc->pgprocno))
break;
}This, from UnpinBuffer(), is our more-typical style:
old_buf_state = pg_atomic_read_u32(&buf->state);
for (;;)
{
...if (pg_atomic_compare_exchange_u32(&buf->state, &old_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