Hello Dean,

First, I have a thing against erand48.

Yeah, that's probably a fair point. However, all the existing pgbench random functions are using it, so I think it's fair enough for permute() to do the same (and actually 2^48 is pretty huge). Switching to a 64-bit PRNG might not be a bad idea, but I think that's something we'd want to do across the board, and so I think it should be out of scope for this patch.

But less likely to pass, whereas here we have an internal function that we can set as we want.

Also, there is a 64 bits seed provided to the function which instantly ignores 16 of them, which looks pretty silly to me.

Also, the function is named everywhere erand48 with its hardcoded int16[3] state, which makes a poor abstraction.

At least, I suggest that two 48-bits prng could be initialized with parts of the seed and used in different places, eg for r & m.

Also, the seed could be used to adjust the rotation, maybe.

I'm really at odds with FULL SHIFT 1, because it means that up to 1/256 of
values are kept out of STEERING. [...]

Ah, that's a good point. Something else that also concerned me there was that it might lead to 2 consecutive full shifts with nothing in between, which would lead to less uniform randomness (like the Irwin-Hall distribution). I just did a quick test without the first full shift, and the results do appear to be better,

Indeed, it makes sense to me.

so removing that looks like a good idea.

Third, I think that the rotate code can be simplified, in particular the ?: should be avoided because it may induce branches quite damaging to processor performance.

Yeah, I wondered about that. Perhaps there's a "trick" that can be
used to simplify it. Pre-computing the number of bits in the mask
would probably help.

See pg_popcount64().

--
Fabien.


Reply via email to