[BackendXidGetPid] only access allProcs when xid matches

Started by Junwang Zhaoover 2 years ago12 messages

zhjwpku@gmail.com

over 2 years ago

1 attachment(s)

In function `BackendXidGetPid`, when looping every proc's
TransactionId, there is no need to access its PGPROC since there
is shared memory access: `arrayP->pgprocnos[index]`.

Though the compiler can optimize this kind of inefficiency, I
believe we should ship with better code.

--
Regards
Junwang Zhao

Attachments:

0001-BackendXidGetPid-only-access-allProcs-when-xid-match.patchapplication/octet-stream; name=0001-BackendXidGetPid-only-access-allProcs-when-xid-match.patchDownload

From 35645e4f9118afdf2652137bf65fbacc0eaf5269 Mon Sep 17 00:00:00 2001
From: Zhao Junwang <zhjwpku@gmail.com>
Date: Wed, 9 Aug 2023 11:43:21 +0800
Subject: [PATCH] [BackendXidGetPid] only access allProcs when xid matches

In function `BackendXidGetPid`, when looping every proc's
TransactionId, there is no need to access its PGPROC since there
is shared memory access: `arrayP->pgprocnos[index]`.

Though the compiler can optimize this kind of inefficiency, I
believe we should ship with better code.

Signed-off-by: Zhao Junwang <zhjwpku@gmail.com>
---
 src/backend/storage/ipc/procarray.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index 2a3da49b8f..b8e26acad8 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -3205,11 +3205,9 @@ BackendXidGetPid(TransactionId xid)
 
 	for (index = 0; index < arrayP->numProcs; index++)
 	{
-		int			pgprocno = arrayP->pgprocnos[index];
-		PGPROC	   *proc = &allProcs[pgprocno];
-
 		if (other_xids[index] == xid)
 		{
+			PGPROC	   *proc = &allProcs[arrayP->pgprocnos[index]];
 			result = proc->pid;
 			break;
 		}
-- 
2.41.0

Ashutosh Bapat

ashutosh.bapat.oss@gmail.com

over 2 years ago

In reply to: Junwang Zhao (#1)

Re: [BackendXidGetPid] only access allProcs when xid matches

On Wed, Aug 9, 2023 at 9:30 AM Junwang Zhao <zhjwpku@gmail.com> wrote:

In function `BackendXidGetPid`, when looping every proc's
TransactionId, there is no need to access its PGPROC since there
is shared memory access: `arrayP->pgprocnos[index]`.

Though the compiler can optimize this kind of inefficiency, I
believe we should ship with better code.

Looks good to me. However, I would just move the variable declaration
with their assignments inside the if () rather than combing the
expressions. It more readable that way.

--
Best Wishes,
Ashutosh Bapat

Junwang Zhao

zhjwpku@gmail.com

over 2 years ago

In reply to: Ashutosh Bapat (#2)

1 attachment(s)

Re: [BackendXidGetPid] only access allProcs when xid matches

On Wed, Aug 9, 2023 at 10:46 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Wed, Aug 9, 2023 at 9:30 AM Junwang Zhao <zhjwpku@gmail.com> wrote:

In function `BackendXidGetPid`, when looping every proc's
TransactionId, there is no need to access its PGPROC since there
is shared memory access: `arrayP->pgprocnos[index]`.

Though the compiler can optimize this kind of inefficiency, I
believe we should ship with better code.

Looks good to me. However, I would just move the variable declaration
with their assignments inside the if () rather than combing the
expressions. It more readable that way.

yeah, make sense, also checked elsewhere using the original style,
attachment file
keep that style, thanks ;)

--
Best Wishes,
Ashutosh Bapat

--
Regards
Junwang Zhao

Attachments:

v2-0001-BackendXidGetPid-only-access-allProcs-when-xid-ma.patchapplication/octet-stream; name=v2-0001-BackendXidGetPid-only-access-allProcs-when-xid-ma.patchDownload

From 3b50691183d110de3688f2c61ab31e3d55222abe Mon Sep 17 00:00:00 2001
From: Zhao Junwang <zhjwpku@gmail.com>
Date: Wed, 9 Aug 2023 11:43:21 +0800
Subject: [PATCH v2] [BackendXidGetPid] only access allProcs when xid matches

In function `BackendXidGetPid`, when looping every proc's
TransactionId, there is no need to access its PGPROC since there
is shared memory access: `arrayP->pgprocnos[index]`.

Though the compiler can optimize this kind of inefficiency, I
believe we should ship with better code.

Signed-off-by: Zhao Junwang <zhjwpku@gmail.com>
---
 src/backend/storage/ipc/procarray.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index 2a3da49b8f..3361709e64 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -3205,11 +3205,10 @@ BackendXidGetPid(TransactionId xid)
 
 	for (index = 0; index < arrayP->numProcs; index++)
 	{
-		int			pgprocno = arrayP->pgprocnos[index];
-		PGPROC	   *proc = &allProcs[pgprocno];
-
 		if (other_xids[index] == xid)
 		{
+			int			pgprocno = arrayP->pgprocnos[index];
+			PGPROC	   *proc = &allProcs[pgprocno];
 			result = proc->pid;
 			break;
 		}
-- 
2.41.0

Ashutosh Bapat

ashutosh.bapat.oss@gmail.com

over 2 years ago

In reply to: Junwang Zhao (#3)

Re: [BackendXidGetPid] only access allProcs when xid matches

Please add this to commitfest so that it's not forgotten.

On Wed, Aug 9, 2023 at 8:37 PM Junwang Zhao <zhjwpku@gmail.com> wrote:

On Wed, Aug 9, 2023 at 10:46 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Wed, Aug 9, 2023 at 9:30 AM Junwang Zhao <zhjwpku@gmail.com> wrote:

In function `BackendXidGetPid`, when looping every proc's
TransactionId, there is no need to access its PGPROC since there
is shared memory access: `arrayP->pgprocnos[index]`.

Though the compiler can optimize this kind of inefficiency, I
believe we should ship with better code.

Looks good to me. However, I would just move the variable declaration
with their assignments inside the if () rather than combing the
expressions. It more readable that way.

yeah, make sense, also checked elsewhere using the original style,
attachment file
keep that style, thanks ;)

--
Best Wishes,
Ashutosh Bapat

--
Regards
Junwang Zhao

--
Best Wishes,
Ashutosh Bapat

Junwang Zhao

zhjwpku@gmail.com

over 2 years ago

In reply to: Ashutosh Bapat (#4)

Re: [BackendXidGetPid] only access allProcs when xid matches

On Thu, Aug 10, 2023 at 4:11 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

Please add this to commitfest so that it's not forgotten.

Added [1]https://commitfest.postgresql.org/44/4495/, thanks

[1]: https://commitfest.postgresql.org/44/4495/

On Wed, Aug 9, 2023 at 8:37 PM Junwang Zhao <zhjwpku@gmail.com> wrote:

On Wed, Aug 9, 2023 at 10:46 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Wed, Aug 9, 2023 at 9:30 AM Junwang Zhao <zhjwpku@gmail.com> wrote:

In function `BackendXidGetPid`, when looping every proc's
TransactionId, there is no need to access its PGPROC since there
is shared memory access: `arrayP->pgprocnos[index]`.

Though the compiler can optimize this kind of inefficiency, I
believe we should ship with better code.

Looks good to me. However, I would just move the variable declaration
with their assignments inside the if () rather than combing the
expressions. It more readable that way.

yeah, make sense, also checked elsewhere using the original style,
attachment file
keep that style, thanks ;)

--
Best Wishes,
Ashutosh Bapat

--
Regards
Junwang Zhao

--
Best Wishes,
Ashutosh Bapat

--
Regards
Junwang Zhao

Ashutosh Bapat

ashutosh.bapat.oss@gmail.com

over 2 years ago

In reply to: Junwang Zhao (#5)

Re: [BackendXidGetPid] only access allProcs when xid matches

Hi Junwang,
We leave a line blank after variable declaration as in the attached patch.

Otherwise the patch looks good to me.

The function modified by the patch is only used by extension
pgrowlocks. Given that the function will be invoked as many times as
the number of locked rows in the relation, the patch may show some
improvement and thus be more compelling. One way to measure
performance is to create a table with millions of rows, SELECT all
rows with FOR SHARE/UPDATE clause. Then run pgrowlock() on that
relation. This will invoke the given function a million times. That
way we might be able to catch some miniscule improvement per row.

If the performance is measurable, we can mark the CF entry as ready
for committer.

--
Best Wishes,
Ashutosh Bapat

Show quoted text

On Thu, Aug 10, 2023 at 1:48 PM Junwang Zhao <zhjwpku@gmail.com> wrote:

On Thu, Aug 10, 2023 at 4:11 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

Please add this to commitfest so that it's not forgotten.

Added [1], thanks

[1]: https://commitfest.postgresql.org/44/4495/

On Wed, Aug 9, 2023 at 8:37 PM Junwang Zhao <zhjwpku@gmail.com> wrote:

On Wed, Aug 9, 2023 at 10:46 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Wed, Aug 9, 2023 at 9:30 AM Junwang Zhao <zhjwpku@gmail.com> wrote:

In function `BackendXidGetPid`, when looping every proc's
TransactionId, there is no need to access its PGPROC since there
is shared memory access: `arrayP->pgprocnos[index]`.

Though the compiler can optimize this kind of inefficiency, I
believe we should ship with better code.

Looks good to me. However, I would just move the variable declaration
with their assignments inside the if () rather than combing the
expressions. It more readable that way.

yeah, make sense, also checked elsewhere using the original style,
attachment file
keep that style, thanks ;)

--
Best Wishes,
Ashutosh Bapat

--
Regards
Junwang Zhao

--
Best Wishes,
Ashutosh Bapat

--
Regards
Junwang Zhao

Michael Paquier

michael@paquier.xyz

over 2 years ago

In reply to: Ashutosh Bapat (#6)

Re: [BackendXidGetPid] only access allProcs when xid matches

On Thu, Sep 07, 2023 at 01:22:07PM +0530, Ashutosh Bapat wrote:

Otherwise the patch looks good to me.

The function modified by the patch is only used by extension
pgrowlocks. Given that the function will be invoked as many times as
the number of locked rows in the relation, the patch may show some
improvement and thus be more compelling. One way to measure
performance is to create a table with millions of rows, SELECT all
rows with FOR SHARE/UPDATE clause. Then run pgrowlock() on that
relation. This will invoke the given function a million times. That
way we might be able to catch some miniscule improvement per row.

If the performance is measurable, we can mark the CF entry as ready
for committer.

So, is the difference measurable? Assuming that your compiler does
not optimize that, my guess is no because the cycles are going to be
eaten by the other system calls in pgrowlocks. You could increase the
number of proc entries and force a large loop around
BackendXidGetPid() to see a difference of runtime, but that's going
to require a lot more proc entries to see any kind of difference
outside the scope of a usual ~1% (somewhat magic number!) noise with
such micro benchmarks.

-        int            pgprocno = arrayP->pgprocnos[index];
-        PGPROC       *proc = &allProcs[pgprocno];
-
         if (other_xids[index] == xid)
         {
+            PGPROC       *proc = &allProcs[arrayP->pgprocnos[index]];
             result = proc->pid;
             break;

Saying that, moving the declarations into the inner loop is usually a
good practice, but I would just keep two variables instead of one for
the sake of readability. That's a nit, sure.
--
Michael

Junwang Zhao

zhjwpku@gmail.com

over 2 years ago

In reply to: Michael Paquier (#7)

1 attachment(s)

Re: [BackendXidGetPid] only access allProcs when xid matches

On Thu, Sep 7, 2023 at 4:04 PM Michael Paquier <michael@paquier.xyz> wrote:

On Thu, Sep 07, 2023 at 01:22:07PM +0530, Ashutosh Bapat wrote:

Otherwise the patch looks good to me.

The function modified by the patch is only used by extension
pgrowlocks. Given that the function will be invoked as many times as
the number of locked rows in the relation, the patch may show some
improvement and thus be more compelling. One way to measure
performance is to create a table with millions of rows, SELECT all
rows with FOR SHARE/UPDATE clause. Then run pgrowlock() on that
relation. This will invoke the given function a million times. That
way we might be able to catch some miniscule improvement per row.

If the performance is measurable, we can mark the CF entry as ready
for committer.

So, is the difference measurable? Assuming that your compiler does

I have checked my compiler, this patch give them same assembly code
as before.

not optimize that, my guess is no because the cycles are going to be
eaten by the other system calls in pgrowlocks. You could increase the
number of proc entries and force a large loop around
BackendXidGetPid() to see a difference of runtime, but that's going
to require a lot more proc entries to see any kind of difference
outside the scope of a usual ~1% (somewhat magic number!) noise with
such micro benchmarks.
-        int            pgprocno = arrayP->pgprocnos[index];
-        PGPROC       *proc = &allProcs[pgprocno];
-
if (other_xids[index] == xid)
{
+            PGPROC       *proc = &allProcs[arrayP->pgprocnos[index]];
result = proc->pid;
break;
Saying that, moving the declarations into the inner loop is usually a
good practice, but I would just keep two variables instead of one for
the sake of readability. That's a nit, sure.

I remember I split this into two lines in v2 patch. Whatever, I attached
a v3 with a suggestion from Ashutosh Bapat.

--
Michael

--
Regards
Junwang Zhao

Attachments:

v3-0001-BackendXidGetPid-only-access-allProcs-when-xid-ma.patchapplication/octet-stream; name=v3-0001-BackendXidGetPid-only-access-allProcs-when-xid-ma.patchDownload

From d3adf0be6ac338031223ded04baab2d05cbd0f24 Mon Sep 17 00:00:00 2001
From: Zhao Junwang <zhjwpku@gmail.com>
Date: Wed, 9 Aug 2023 11:43:21 +0800
Subject: [PATCH v3] [BackendXidGetPid] only access allProcs when xid matches

In function `BackendXidGetPid`, when looping every proc's
TransactionId, there is no need to access its PGPROC since there
is shared memory access: `arrayP->pgprocnos[index]`.

Though the compiler can optimize this kind of inefficiency, I
believe we should ship with better code.

Signed-off-by: Zhao Junwang <zhjwpku@gmail.com>
---
 src/backend/storage/ipc/procarray.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index 2a3da49b8f..016f08b5b4 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -3205,11 +3205,11 @@ BackendXidGetPid(TransactionId xid)
 
 	for (index = 0; index < arrayP->numProcs; index++)
 	{
-		int			pgprocno = arrayP->pgprocnos[index];
-		PGPROC	   *proc = &allProcs[pgprocno];
-
 		if (other_xids[index] == xid)
 		{
+			int			pgprocno = arrayP->pgprocnos[index];
+			PGPROC	   *proc = &allProcs[pgprocno];
+
 			result = proc->pid;
 			break;
 		}
-- 
2.41.0

Ashutosh Bapat

ashutosh.bapat.oss@gmail.com

over 2 years ago

In reply to: Ashutosh Bapat (#6)

1 attachment(s)

Re: [BackendXidGetPid] only access allProcs when xid matches

Forgot to attach the patch.

On Thu, Sep 7, 2023 at 1:22 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

Hi Junwang,
We leave a line blank after variable declaration as in the attached patch.

Otherwise the patch looks good to me.

The function modified by the patch is only used by extension
pgrowlocks. Given that the function will be invoked as many times as
the number of locked rows in the relation, the patch may show some
improvement and thus be more compelling. One way to measure
performance is to create a table with millions of rows, SELECT all
rows with FOR SHARE/UPDATE clause. Then run pgrowlock() on that
relation. This will invoke the given function a million times. That
way we might be able to catch some miniscule improvement per row.

If the performance is measurable, we can mark the CF entry as ready
for committer.

--
Best Wishes,
Ashutosh Bapat

On Thu, Aug 10, 2023 at 1:48 PM Junwang Zhao <zhjwpku@gmail.com> wrote:

On Thu, Aug 10, 2023 at 4:11 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

Please add this to commitfest so that it's not forgotten.

Added [1], thanks

[1]: https://commitfest.postgresql.org/44/4495/

On Wed, Aug 9, 2023 at 8:37 PM Junwang Zhao <zhjwpku@gmail.com> wrote:

On Wed, Aug 9, 2023 at 10:46 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Wed, Aug 9, 2023 at 9:30 AM Junwang Zhao <zhjwpku@gmail.com> wrote:

In function `BackendXidGetPid`, when looping every proc's
TransactionId, there is no need to access its PGPROC since there
is shared memory access: `arrayP->pgprocnos[index]`.

Though the compiler can optimize this kind of inefficiency, I
believe we should ship with better code.

Looks good to me. However, I would just move the variable declaration
with their assignments inside the if () rather than combing the
expressions. It more readable that way.

yeah, make sense, also checked elsewhere using the original style,
attachment file
keep that style, thanks ;)

--
Best Wishes,
Ashutosh Bapat

--
Regards
Junwang Zhao

--
Best Wishes,
Ashutosh Bapat

--
Regards
Junwang Zhao

--
Best Wishes,
Ashutosh Bapat

Attachments:

0001-only-access-allProcs-when-xid-matches-20230907.patchtext/x-patch; charset=US-ASCII; name=0001-only-access-allProcs-when-xid-matches-20230907.patchDownload

From 7ad91841dda0f49e7bcc44809b58230ed67d80f7 Mon Sep 17 00:00:00 2001
From: Zhao Junwang <zhjwpku@gmail.com>
Date: Wed, 9 Aug 2023 11:43:21 +0800
Subject: [PATCH] only access allProcs when xid matches

In function `BackendXidGetPid`, when looping every proc's
TransactionId, there is no need to access its PGPROC since there
is shared memory access: `arrayP->pgprocnos[index]`.

Though the compiler can optimize this kind of inefficiency, I
believe we should ship with better code.

Signed-off-by: Zhao Junwang <zhjwpku@gmail.com>
---
 src/backend/storage/ipc/procarray.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index bfbf7f903f..d93475b2bd 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -3174,11 +3174,11 @@ BackendXidGetPid(TransactionId xid)
 
 	for (index = 0; index < arrayP->numProcs; index++)
 	{
-		int			pgprocno = arrayP->pgprocnos[index];
-		PGPROC	   *proc = &allProcs[pgprocno];
-
 		if (other_xids[index] == xid)
 		{
+			int			pgprocno = arrayP->pgprocnos[index];
+			PGPROC	   *proc = &allProcs[pgprocno];
+
 			result = proc->pid;
 			break;
 		}
-- 
2.25.1

#10

Junwang Zhao

zhjwpku@gmail.com

over 2 years ago

In reply to: Ashutosh Bapat (#9)

Re: [BackendXidGetPid] only access allProcs when xid matches

On Thu, Sep 7, 2023 at 5:07 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

Forgot to attach the patch.

LGTM

Should I change the status to ready for committer now?

On Thu, Sep 7, 2023 at 1:22 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

Hi Junwang,
We leave a line blank after variable declaration as in the attached patch.

Otherwise the patch looks good to me.

The function modified by the patch is only used by extension
pgrowlocks. Given that the function will be invoked as many times as
the number of locked rows in the relation, the patch may show some
improvement and thus be more compelling. One way to measure
performance is to create a table with millions of rows, SELECT all
rows with FOR SHARE/UPDATE clause. Then run pgrowlock() on that
relation. This will invoke the given function a million times. That
way we might be able to catch some miniscule improvement per row.

If the performance is measurable, we can mark the CF entry as ready
for committer.

--
Best Wishes,
Ashutosh Bapat

On Thu, Aug 10, 2023 at 1:48 PM Junwang Zhao <zhjwpku@gmail.com> wrote:

On Thu, Aug 10, 2023 at 4:11 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

Please add this to commitfest so that it's not forgotten.

Added [1], thanks

[1]: https://commitfest.postgresql.org/44/4495/

On Wed, Aug 9, 2023 at 8:37 PM Junwang Zhao <zhjwpku@gmail.com> wrote:

On Wed, Aug 9, 2023 at 10:46 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Wed, Aug 9, 2023 at 9:30 AM Junwang Zhao <zhjwpku@gmail.com> wrote:

In function `BackendXidGetPid`, when looping every proc's
TransactionId, there is no need to access its PGPROC since there
is shared memory access: `arrayP->pgprocnos[index]`.

Though the compiler can optimize this kind of inefficiency, I
believe we should ship with better code.

Looks good to me. However, I would just move the variable declaration
with their assignments inside the if () rather than combing the
expressions. It more readable that way.

yeah, make sense, also checked elsewhere using the original style,
attachment file
keep that style, thanks ;)

--
Best Wishes,
Ashutosh Bapat

--
Regards
Junwang Zhao

--
Best Wishes,
Ashutosh Bapat

--
Regards
Junwang Zhao

--
Best Wishes,
Ashutosh Bapat

--
Regards
Junwang Zhao

#11

Ashutosh Bapat

ashutosh.bapat.oss@gmail.com

over 2 years ago

In reply to: Junwang Zhao (#10)

Re: [BackendXidGetPid] only access allProcs when xid matches

On Thu, Sep 7, 2023 at 3:05 PM Junwang Zhao <zhjwpku@gmail.com> wrote:

On Thu, Sep 7, 2023 at 5:07 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

Forgot to attach the patch.

LGTM

Should I change the status to ready for committer now?

I would still love to see some numbers. It's not that hard. Either
using my micro benchmark or Michael's. We may or may not see an
improvement. But at least we tried. But Michael feels otherwise,
changing it to RoC is fine.

--
Best Wishes,
Ashutosh Bapat

#12

Michael Paquier

michael@paquier.xyz

over 2 years ago

In reply to: Ashutosh Bapat (#11)

Re: [BackendXidGetPid] only access allProcs when xid matches

On Thu, Sep 07, 2023 at 04:34:20PM +0530, Ashutosh Bapat wrote:

I would still love to see some numbers. It's not that hard. Either
using my micro benchmark or Michael's. We may or may not see an
improvement. But at least we tried. But Michael feels otherwise,
changing it to RoC is fine.

I have hardcoded a small SQL function that calls BackendXidGetPid() in
a tight loop a few hundred millions of times, and I see no difference
in runtime between HEAD and the patch under -O2. The argument about
readability still applies for me, so applied.
--
Michael