Thread: [BackendXidGetPid] only access allProcs when xid matches

[BackendXidGetPid] only access allProcs when xid matches

From
Junwang Zhao
Date:
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

Attachment

Re: [BackendXidGetPid] only access allProcs when xid matches

From
Ashutosh Bapat
Date:
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



Re: [BackendXidGetPid] only access allProcs when xid matches

From
Junwang Zhao
Date:
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

Attachment

Re: [BackendXidGetPid] only access allProcs when xid matches

From
Ashutosh Bapat
Date:
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



Re: [BackendXidGetPid] only access allProcs when xid matches

From
Junwang Zhao
Date:
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



Re: [BackendXidGetPid] only access allProcs when xid matches

From
Ashutosh Bapat
Date:
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



Re: [BackendXidGetPid] only access allProcs when xid matches

From
Michael Paquier
Date:
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

Attachment

Re: [BackendXidGetPid] only access allProcs when xid matches

From
Junwang Zhao
Date:
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

Attachment

Re: [BackendXidGetPid] only access allProcs when xid matches

From
Ashutosh Bapat
Date:
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

Attachment

Re: [BackendXidGetPid] only access allProcs when xid matches

From
Junwang Zhao
Date:
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



Re: [BackendXidGetPid] only access allProcs when xid matches

From
Ashutosh Bapat
Date:
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



Re: [BackendXidGetPid] only access allProcs when xid matches

From
Michael Paquier
Date:
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

Attachment