Thanks for looking at this.
On Fri, 1 Feb 2019 at 11:45, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>
> I only have cosmetic suggestions for this patch. For one thing, I think
> the .c file should be in src/port and its header should be in
> src/include/port/, right beside the likes of pg_bswap.h and pg_crc32c.h.
I've moved the code into src/port and renamed the file to pg_bitutils.c
> For another, I think the arrangement of all those "ifdef
> HAVE_THIS_OR_THAT" in the bitutils.c file is a bit hard to read. I'd
> lay them out like this:
I've made this change too, although when doing it I realised that I
had forgotten to include the check for CPUID. It's possible that does
not exist but POPCNT does, I guess. This has made the #ifs a bit more
complex.
> Finally, the part in bitmapset.c is repetitive on the #ifdefs; I'd just
> put at the top of the file something like
Yeah, agreed. Much neater that way.
> Other than those minor changes, I think we should just get this pushed
> and see what the buildfarm thinks. In the words of a famous PG hacker:
> if a platform ain't in the buildfarm, we don't support it.
I also made a number of other changes to the patch.
1. The patch now only uses the -mpopcnt CFLAG for pg_bitutils.c. I
thought this was important so we don't expose that flag in pg_config
and possibly end up building extension with popcnt instructions, which
might not be portable to other older hardware.
2. Wrote a new pg_popcnt function that accepts an array of bytes and a
size variable. This seems useful for the bloomfilter use.
There are still various number_of_ones[] arrays around the codebase.
These exist in tsgistidx.c, _intbig_gist.c and _ltree_gist.c. It
would be nice to get rid of those too, but one of the usages in each
of those 3 files requires XORing with another bit array before
counting the bits. I thought about maybe writing a pop_count_xor()
function that accepts 2 byte arrays and a length parameter, but it
seems a bit special case, so I didn't.
Another thing I wasn't sure of was if I should just have
bms_num_members() just call pg_popcount(). It might be worth
benchmarking to see what's faster. My thinking is that pg_popcount
will inline the pg_popcount64() call so it would mean a single
function call rather than one for each bitmapword in the set.
I've compiled and run make check-world on Linux with GCC7.3 and
clang6.0. I've also tested on MSVC to ensure I didn't break windows.
It would be good to get a few more people to compile it and run the
tests.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services