Обсуждение: Remove fls(), use pg_bitutils.h facilities instead?

Hi,

Back in commit 4f658dc8 we gained src/port/fls.c.  As anticipated by
its commit message, we later finished up with something better in
src/include/port/pg_bitutils.h.  fls() ("find last set") is an
off-by-one cousin of pg_leftmost_one_pos32().  I don't know why ffs()
("find first set", the rightmost variant) made it into POSIX while
fls() did not, other than perhaps its more amusing name.  fls() is
present on *BSD, Macs and maybe more, but not everywhere, hence the
configure test.  Let's just do it with pg_bitutils.h instead, and drop
some cruft?  Open to better ideas on whether we need a new function,
or there is some way to use the existing facilities directly without
worrying about undefined behaviour for 0, etc.

Noticed while looking for configure stuff to cull.  Mentioning
separately because this isn't a simple
no-longer-needed-crutch-for-prestandard-system case like the others in
a nearby thread.

On Wed, 20 Jul 2022 at 16:21, Thomas Munro <thomas.munro@gmail.com> wrote:
> Back in commit 4f658dc8 we gained src/port/fls.c.  As anticipated by
> its commit message, we later finished up with something better in
> src/include/port/pg_bitutils.h.  fls() ("find last set") is an
> off-by-one cousin of pg_leftmost_one_pos32().

Seems like a good idea to me.

One thing I noticed was that pg_leftmost_one_pos32() expects a uint32
but the new function passes it an int. Is it worth mentioning that's
ok in a comment somewhere or maybe adding an explicit cast?

David

Thomas Munro <thomas.munro@gmail.com> writes:
> Back in commit 4f658dc8 we gained src/port/fls.c.  As anticipated by
> its commit message, we later finished up with something better in
> src/include/port/pg_bitutils.h.  fls() ("find last set") is an
> off-by-one cousin of pg_leftmost_one_pos32().  I don't know why ffs()
> ("find first set", the rightmost variant) made it into POSIX while
> fls() did not, other than perhaps its more amusing name.  fls() is
> present on *BSD, Macs and maybe more, but not everywhere, hence the
> configure test.  Let's just do it with pg_bitutils.h instead, and drop
> some cruft?  Open to better ideas on whether we need a new function,

I think we could probably just drop fls() entirely.  It doesn't look
to me like any of the existing callers expect a zero argument, so they
could be converted to use pg_leftmost_one_pos32() pretty trivially.
I don't see that fls() is buying us anything that is worth requiring
readers to know yet another nonstandard function.

            regards, tom lane

On Wed, Jul 20, 2022 at 4:52 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I think we could probably just drop fls() entirely.  It doesn't look
> to me like any of the existing callers expect a zero argument, so they
> could be converted to use pg_leftmost_one_pos32() pretty trivially.
> I don't see that fls() is buying us anything that is worth requiring
> readers to know yet another nonstandard function.

That was not true for the case in contiguous_pages_to_segment_bin(),
in dsa.c.  If it's just one place like that (and, hrrm, curiously
there is an open issue about binning quality on my to do list...),
then perhaps we should just open code it there.  The attached doesn't
trigger the assertion that work != 0 in a simple make check.

On Wed, Jul 20, 2022 at 5:26 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> On Wed, Jul 20, 2022 at 4:52 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > I think we could probably just drop fls() entirely.  It doesn't look
> > to me like any of the existing callers expect a zero argument, so they
> > could be converted to use pg_leftmost_one_pos32() pretty trivially.
> > I don't see that fls() is buying us anything that is worth requiring
> > readers to know yet another nonstandard function.
>
> That was not true for the case in contiguous_pages_to_segment_bin(),
> in dsa.c.  If it's just one place like that (and, hrrm, curiously
> there is an open issue about binning quality on my to do list...),
> then perhaps we should just open code it there.  The attached doesn't
> trigger the assertion that work != 0 in a simple make check.

That double eval macro wasn't nice.  This time with a static inline function.

Thomas Munro <thomas.munro@gmail.com> writes:
> On Wed, Jul 20, 2022 at 4:52 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I think we could probably just drop fls() entirely.  It doesn't look
>> to me like any of the existing callers expect a zero argument, so they
>> could be converted to use pg_leftmost_one_pos32() pretty trivially.

> That was not true for the case in contiguous_pages_to_segment_bin(),
> in dsa.c.  If it's just one place like that (and, hrrm, curiously
> there is an open issue about binning quality on my to do list...),

How is it sane to ask for a segment bin for zero pages?  Seems like
something should have short-circuited such a case well before here.

            regards, tom lane

Thomas Munro <thomas.munro@gmail.com> writes:
> That double eval macro wasn't nice.  This time with a static inline function.

Seems like passing a size_t to pg_leftmost_one_pos32 isn't great.
It was just as wrong before (if the caller-supplied argument is
indeed a size_t), but no time like the present to fix it.

We could have pg_bitutils.h #define pg_leftmost_one_pos_size_t
as the appropriate one of pg_leftmost_one_pos32/64, perhaps.

            regards, tom lane

On Thu, Jul 21, 2022 at 1:34 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> How is it sane to ask for a segment bin for zero pages?  Seems like
> something should have short-circuited such a case well before here.

It's intended.  There are two ways you can arrive here with n == 0:

* There's a special case in execParallel.c that creates a DSA segment
"in-place" with initial size dsa_minimum_size().  That's because we
don't know yet if we have any executor nodes that need a DSA segment
(Parallel Hash, Parallel Bitmap Heap Scan), so we create one with the
minimum amount of space other than the DSA control meta-data, so you
get an in-place segment 0 with 0 usable pages.  As soon as someone
tries to allocate one byte, the first external DSM segment will be
created.

* A full segment can be re-binned into slot 0.

On Thu, Jul 21, 2022 at 1:48 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Seems like passing a size_t to pg_leftmost_one_pos32 isn't great.
> It was just as wrong before (if the caller-supplied argument is
> indeed a size_t), but no time like the present to fix it.
>
> We could have pg_bitutils.h #define pg_leftmost_one_pos_size_t
> as the appropriate one of pg_leftmost_one_pos32/64, perhaps.

Yeah.

Thomas Munro <thomas.munro@gmail.com> writes:
> On Thu, Jul 21, 2022 at 1:34 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> How is it sane to ask for a segment bin for zero pages?  Seems like
>> something should have short-circuited such a case well before here.

> It's intended.  There are two ways you can arrive here with n == 0:

OK.

>> We could have pg_bitutils.h #define pg_leftmost_one_pos_size_t
>> as the appropriate one of pg_leftmost_one_pos32/64, perhaps.

> Yeah.

Patches look good to me.

            regards, tom lane