On Wed, Jul 20, 2022 at 4:52 PM Tom Lane <t...@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.
From 19a3b2aa631a5291023dcf7185f5cee474fae6b5 Mon Sep 17 00:00:00 2001 From: Thomas Munro <thomas.mu...@gmail.com> Date: Tue, 19 Jul 2022 17:47:10 +1200 Subject: [PATCH v2] Remove fls function. Commit 4f658dc8 provided the traditional BSD fls() function so it could be used in several places. Later we added a bunch of similar sorts of things in pg_bitutils.h. Let's drop fls.c and the configure probe, and reuse the newer code. Reviewed-by: David Rowley <dgrowle...@gmail.com> Reviewed-by: Tom Lane <t...@sss.pgh.pa.us> Discussion: https://postgr.es/m/CA%2BhUKG%2B7dSX1XF8yFGmYk-%3D48dbjH2kmzZj16XvhbrWP-9BzRg%40mail.gmail.com --- configure | 13 ------ configure.ac | 1 - src/backend/access/hash/hashutil.c | 2 +- src/backend/optimizer/path/allpaths.c | 5 +- src/backend/optimizer/prep/prepunion.c | 2 +- src/backend/utils/mmgr/dsa.c | 4 +- src/include/pg_config.h.in | 3 -- src/include/port.h | 4 -- src/port/fls.c | 64 -------------------------- src/tools/msvc/Mkvcbuild.pm | 2 +- src/tools/msvc/Solution.pm | 1 - 11 files changed, 9 insertions(+), 92 deletions(-) delete mode 100644 src/port/fls.c diff --git a/configure b/configure index 59fa82b8d7..33a425562f 100755 --- a/configure +++ b/configure @@ -16771,19 +16771,6 @@ esac fi -ac_fn_c_check_func "$LINENO" "fls" "ac_cv_func_fls" -if test "x$ac_cv_func_fls" = xyes; then : - $as_echo "#define HAVE_FLS 1" >>confdefs.h - -else - case " $LIBOBJS " in - *" fls.$ac_objext "* ) ;; - *) LIBOBJS="$LIBOBJS fls.$ac_objext" - ;; -esac - -fi - ac_fn_c_check_func "$LINENO" "getopt" "ac_cv_func_getopt" if test "x$ac_cv_func_getopt" = xyes; then : $as_echo "#define HAVE_GETOPT 1" >>confdefs.h diff --git a/configure.ac b/configure.ac index 612dabf698..5295cb5806 100644 --- a/configure.ac +++ b/configure.ac @@ -1911,7 +1911,6 @@ fi AC_REPLACE_FUNCS(m4_normalize([ dlopen explicit_bzero - fls getopt getpeereid getrusage diff --git a/src/backend/access/hash/hashutil.c b/src/backend/access/hash/hashutil.c index fe37bc47cb..32822dbb6b 100644 --- a/src/backend/access/hash/hashutil.c +++ b/src/backend/access/hash/hashutil.c @@ -436,7 +436,7 @@ _hash_get_oldblock_from_newbucket(Relation rel, Bucket new_bucket) * started. Masking the most significant bit of new bucket would give us * old bucket. */ - mask = (((uint32) 1) << (fls(new_bucket) - 1)) - 1; + mask = (((uint32) 1) << pg_leftmost_one_pos32(new_bucket)) - 1; old_bucket = new_bucket & mask; metabuf = _hash_getbuf(rel, HASH_METAPAGE, HASH_READ, LH_META_PAGE); diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c index 028d9e1680..63f0f6b79c 100644 --- a/src/backend/optimizer/path/allpaths.c +++ b/src/backend/optimizer/path/allpaths.c @@ -47,6 +47,7 @@ #include "parser/parsetree.h" #include "partitioning/partbounds.h" #include "partitioning/partprune.h" +#include "port/pg_bitutils.h" #include "rewrite/rewriteManip.h" #include "utils/lsyscache.h" @@ -1491,7 +1492,7 @@ add_paths_to_append_rel(PlannerInfo *root, RelOptInfo *rel, if (enable_parallel_append) { parallel_workers = Max(parallel_workers, - fls(list_length(live_childrels))); + pg_leftmost_one_pos32(list_length(live_childrels)) + 1); parallel_workers = Min(parallel_workers, max_parallel_workers_per_gather); } @@ -1542,7 +1543,7 @@ add_paths_to_append_rel(PlannerInfo *root, RelOptInfo *rel, * the planned number of parallel workers. */ parallel_workers = Max(parallel_workers, - fls(list_length(live_childrels))); + pg_leftmost_one_pos32(list_length(live_childrels)) + 1); parallel_workers = Min(parallel_workers, max_parallel_workers_per_gather); Assert(parallel_workers > 0); diff --git a/src/backend/optimizer/prep/prepunion.c b/src/backend/optimizer/prep/prepunion.c index 2214920dea..043181b586 100644 --- a/src/backend/optimizer/prep/prepunion.c +++ b/src/backend/optimizer/prep/prepunion.c @@ -675,7 +675,7 @@ generate_union_paths(SetOperationStmt *op, PlannerInfo *root, if (enable_parallel_append) { parallel_workers = Max(parallel_workers, - fls(list_length(partial_pathlist))); + pg_leftmost_one_pos32(list_length(partial_pathlist)) + 1); parallel_workers = Min(parallel_workers, max_parallel_workers_per_gather); } diff --git a/src/backend/utils/mmgr/dsa.c b/src/backend/utils/mmgr/dsa.c index b6cb8fa13d..8fa943a798 100644 --- a/src/backend/utils/mmgr/dsa.c +++ b/src/backend/utils/mmgr/dsa.c @@ -51,6 +51,7 @@ #include "postgres.h" #include "port/atomics.h" +#include "port/pg_bitutils.h" #include "storage/dsm.h" #include "storage/ipc.h" #include "storage/lwlock.h" @@ -137,7 +138,8 @@ typedef size_t dsa_segment_index; * free pages? There is no point in looking in segments in lower bins; they * definitely can't service a request for n free pages. */ -#define contiguous_pages_to_segment_bin(n) Min(fls(n), DSA_NUM_SEGMENT_BINS - 1) +#define contiguous_pages_to_segment_bin(n) \ + Min(n == 0 ? 0 : pg_leftmost_one_pos32(n) + 1, DSA_NUM_SEGMENT_BINS - 1) /* Macros for access to locks. */ #define DSA_AREA_LOCK(area) (&area->control->lock) diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in index 529fb84a86..86d0b1941b 100644 --- a/src/include/pg_config.h.in +++ b/src/include/pg_config.h.in @@ -191,9 +191,6 @@ /* Define to 1 if you have the `fdatasync' function. */ #undef HAVE_FDATASYNC -/* Define to 1 if you have the `fls' function. */ -#undef HAVE_FLS - /* Define to 1 if fseeko (and presumably ftello) exists and is declared. */ #undef HAVE_FSEEKO diff --git a/src/include/port.h b/src/include/port.h index e647f62b77..d39b04141f 100644 --- a/src/include/port.h +++ b/src/include/port.h @@ -366,10 +366,6 @@ extern int gettimeofday(struct timeval *tp, struct timezone *tzp); #define pgoff_t off_t #endif -#ifndef HAVE_FLS -extern int fls(int mask); -#endif - #ifndef HAVE_GETPEEREID /* On Windows, Perl might have incompatible definitions of uid_t and gid_t. */ #ifndef PLPERL_HAVE_UID_GID diff --git a/src/port/fls.c b/src/port/fls.c deleted file mode 100644 index 19b4221826..0000000000 --- a/src/port/fls.c +++ /dev/null @@ -1,64 +0,0 @@ -/*------------------------------------------------------------------------- - * - * fls.c - * finds the last (most significant) bit that is set - * - * Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group - * - * - * IDENTIFICATION - * src/port/fls.c - * - * This file was taken from FreeBSD to provide an implementation of fls() - * for platforms that lack it. Note that the operating system's version may - * be substantially more efficient than ours, since some platforms have an - * assembly instruction that does exactly this. - * - * The FreeBSD copyright terms follow. - */ - -/*- - * Copyright (c) 1990, 1993 - * The Regents of the University of California. All rights reserved. - * - * Redistribution and use in source and binary forms, with or without - * modification, are permitted provided that the following conditions - * are met: - * 1. Redistributions of source code must retain the above copyright - * notice, this list of conditions and the following disclaimer. - * 2. Redistributions in binary form must reproduce the above copyright - * notice, this list of conditions and the following disclaimer in the - * documentation and/or other materials provided with the distribution. - * 4. Neither the name of the University nor the names of its contributors - * may be used to endorse or promote products derived from this software - * without specific prior written permission. - * - * THIS SOFTWARE IS PROVIDED BY THE REGENTS AND CONTRIBUTORS ``AS IS'' AND - * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE - * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE - * ARE DISCLAIMED. IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE - * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL - * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS - * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) - * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT - * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY - * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF - * SUCH DAMAGE. - */ - -#include "c.h" - -/* - * Find Last Set bit - */ -int -fls(int mask) -{ - int bit; - - if (mask == 0) - return (0); - for (bit = 1; mask != 1; bit++) - mask = (unsigned int) mask >> 1; - return (bit); -} diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm index cc7a908d10..c935f776e5 100644 --- a/src/tools/msvc/Mkvcbuild.pm +++ b/src/tools/msvc/Mkvcbuild.pm @@ -99,7 +99,7 @@ sub mkvcbuild $solution = CreateSolution($vsVersion, $config); our @pgportfiles = qw( - chklocale.c explicit_bzero.c fls.c fdatasync.c + chklocale.c explicit_bzero.c fdatasync.c getpeereid.c getrusage.c inet_aton.c getaddrinfo.c gettimeofday.c inet_net_ntop.c kill.c open.c snprintf.c strlcat.c strlcpy.c dirmod.c noblock.c path.c diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm index 3ddcd024a7..23296527ae 100644 --- a/src/tools/msvc/Solution.pm +++ b/src/tools/msvc/Solution.pm @@ -258,7 +258,6 @@ sub GenerateFiles HAVE_EXECINFO_H => undef, HAVE_EXPLICIT_BZERO => undef, HAVE_FDATASYNC => 1, - HAVE_FLS => undef, HAVE_FSEEKO => 1, HAVE_FUNCNAME__FUNC => undef, HAVE_FUNCNAME__FUNCTION => 1, -- 2.36.1
