| From: | Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> | 
|---|---|
| To: | Thomas Munro <thomas(dot)munro(at)gmail(dot)com> | 
| Cc: | Hironobu SUZUKI <hironobu(at)interdb(dot)jp>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, David Steele <david(at)pgmasters(dot)net> | 
| Subject: | Re: pgbench - add pseudo-random permutation function | 
| Date: | 2019-07-23 07:44:09 | 
| Message-ID: | alpine.DEB.2.21.1907230730150.7144@lancre | 
| Views: | Whole Thread | Raw Message | Download mbox | Resend email | 
| Thread: | |
| Lists: | pgsql-hackers | 
Hello Thomas,
>>> Function nbits(), which was previously discussed, has been simplified by
>>> using the function pg_popcount64().
>
> Hi Fabien, Suzuki-san,
>
> I am not smart enough to commit this
I'm in no hurry:-)
> or judge its value for benchmarking,
I think that it is valuable given that we have non uniform random 
generators in pgbench.
I'm wondering about the modular_multiply manual implementation which adds 
quite a few lines of non trivial code. If int128 is available on all/most 
platforms, it could be removed and marked as not supported on these, 
although it would create an issue with non regression tests.
> but I have a few trivial comments on the language:
>
> +    It allows to mix the output of non uniform random functions so that
>
> "It allows the output of non-uniform random functions to be mixed so that"
Fixed.
> +    ensures that a perfect permutation is applied: there are no collisions
> +    nor holes in the output values.
>
> "neither collisions nor holes", or "no collisions or holes"
I choose the first.
> +    The function errors if size is not positive.
>
> "raises an error"
Fixed.
> + * 24 bits mega primes from https://primes.utm.edu/lists/small/millions/
>
> "24 bit mega primes"
Fixed.
> +/* length of n binary representation */
> +static int
> +nbits(uint64 n)
> +{
> +    /* set lower bits to 1 and count them */
> +    return pg_popcount64(compute_mask(n));
> +}
>
> I suppose you could use n == 0 ? 0 : pg_leftmost_one_pos64(n) + 1, and then...
It would create a branch, that I'm trying to avoid.
> +/* return smallest mask holding n  */
> +static uint64
> +compute_mask(uint64 n)
> +{
> +    n |= n >> 1;
> +    n |= n >> 2;
> +    n |= n >> 4;
> +    n |= n >> 8;
> +    n |= n >> 16;
> +    n |= n >> 32;
> +    return n;
> +}
>
> ... here you could use 1 << nbits(n)) - 1.  I have no idea if doing it
> that way around is better, it's just a thought and removes a few lines
> of bit-swizzling code.
This would create a infinite recursion as nbits currently uses 
compute_mask. The 6 bitfield operation above is pretty efficient, I'd let 
it at that.
Attached a v16.
-- 
Fabien.
| Attachment | Content-Type | Size | 
|---|---|---|
| pgbench-prp-func-16.patch | text/x-diff | 18.0 KB | 
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Michael Paquier | 2019-07-23 08:04:32 | Re: Race conditions with TAP test for syncrep | 
| Previous Message | Michael Paquier | 2019-07-23 07:38:01 | Re: Add parallelism and glibc dependent only options to reindexdb |