Re: Sort functions with specialized comparators

On Sun, Jan 5, 2025 at 1:15 AM Andrey M. Borodin <x4mmm(at)yandex-team(dot)ru> wrote:
>
> > On 4 Jan 2025, at 10:24, John Naylor <johncnaylorls(at)gmail(dot)com> wrote:
> >
> > v6-0001:
> >
> > +static int
> > +unique_cmp(const void *a, const void *b)
> > +{
> > + int32 aval = *((const int32 *) a);
> > + int32 bval = *((const int32 *) b);
> > +
> > + return pg_cmp_s32(aval, bval);
> > +}
> >
> > I'm not sure it makes sense to create a whole new function for this,
> > when the same patch removed:
> >
> > -int
> > -compASC(const void *a, const void *b)
> > -{
> > - return pg_cmp_s32(*(const int32 *) a, *(const int32 *) b);
> > -}
> >
> > ...which in effect the exact same thing.
> >
> > Otherwise seems close to committable.
>
> I thought about it, but decided to rename the routine.
> Here's a version 7 with compASC().

I had the right idea, but the wrong function -- HEAD already had a
suitable comp function, and one that works well with inlined
specialized sorts: isort_cmp(). We just need to remove the extra
argument. Like some other patches in this series, this does have the
side effect of removing the ability to skip quinique(), so that should
be benchmarked (I can do that this week unless you beat me to it). We
can specialize quinique too, as mentioned upthread, but I'm not sure
we need to.

> And, just in case, if we already have ASC, why not keep DESC too instead of newly invented cmp function :) PFA v8.

Those functions from common/int.h are probably not good when inlined
(see comment there). If we really want to keep the branch for asc/desc
out of the comparison, it makes more sense to add a function to
reverse the elements after sorting. That allows using the same cmp
function for everything, thus removing the apparent need for a global
wrapper around the static sort function.

I've done both ideas in v9, which also tries to improve patch
legibility by keeping things in the same place they were before. It
also removes the internal "n > 1" checks that you mentioned earlier --
after thinking about it that seems better than adding braces to one
macro to keep it functional.

--
John Naylor
Amazon Web Services