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