Обсуждение: [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.
--
Regards
Junwang Zhao
Вложения
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
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
Вложения
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
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
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
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
Вложения
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
Вложения
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
Вложения
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
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
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