Re: [RFC] [Draft] Add RNG extension and deprecate mt_srand()

On Tue, May 18, 2021 at 6:19 PM Go Kudo <zeriyoshi@gmail.com> wrote: > Hello internals. > > I have created a draft of the RFC. > > https://wiki.php.net/rfc/rng_extension > Hello Go Kudo, First of all, thank you for the (re)work. I think the API looks better, but I still have some remarks/questions (roughly in order of appearance, with some overlap): - How many bytes is Source::next() supposed to generate? Is it even intended to be called directly (vs `Randomizer::generateBytes(int $length)`)? - The stubs in "Proposal" should show __construct() signatures for the classes implementing Source. - About the MT19937PHP variant: The PHP manual for mt_srand() describes the MT_RAND_PHP mode like this: "Uses an *incorrect* Mersenne Twister implementation which was used as the default up till PHP 7.1.0. This mode is available for backward compatibility."; do we really need/want to port it to this new extension? - I think you said in the past that XorShift128Plus should be the preferred/default implementation, and MT19937 is only provided for compatibility/migration? But if so, that's not clear at all in the RFC. - PlatformProvided feels like the "odd one out" here: its constructor [assuming same as previous RFC] doesn't take a seed, and it isn't serializable; I understand why, but the main point of the RFC is "RNG reproducibility", which this class cannot achieve. Also, we already have random_bytes()/random_int(), and you're proposing to change shuffle()/str_shuffle()/array_rand()'s internal RNG from mt_rand()-like to random_bytes()-like, so what reason would there be to use this class? only generateFloat()? but couldn't that be a new global function random_float()? All in all, wouldn't it be simpler to drop PlatformProvided, and have all Source implementations take a seed on construction and be serializable? - Technical, about Randomizer::generateFloat(): Is the returned float guaranteed to be finite (i.e. never NAN nor +/-INF)? And may it ever return -0.0 (negative zero)? and even if not, is 0.0 twice as likely as other floats? Also, no generateFloat(float $min, float $max) variant like for generateInt()? - **About main usage of the API:** To get e.g. $n (say 3) pseudo-random integers between $min (say 0) and $max (say 100) using the "best" implementation (which we have to know/guess is XorShift128+, isn't it?), we must: *first* choose a $seed (time(), maybe?), *then* do `$source = new XorShift128Plus($seed); *and* $rng = new Randomizer($source);` [even if $source (and $seed) could be inlined, that's still several "steps"], *then* we can finally call $rng->generateInt($min, $max); $n times. Is that correct? If so, it may make sense from the implementer side, but from the user side it doesn't look like the most friendly. Also, that lets the possibility of constructing a second Randomizer using the same $source (or $rng->getSource()), which could be undesirable for reproducibility, isn't it? Wouldn't it be better to "merge" Randomizer into the interface/classes? Actually I see that's the case in your other project ext/orng ( https://github.com/zeriyoshi/php-ext-orng/blob/master/rng/rnginterface.stub.php and https://github.com/zeriyoshi/php-ext-orng/blob/master/rng/xorshift128plus.stub.php ), why the different approach for ext/rng? Maybe even better (and already suggested), have Randomizer constructor take an *optional* algorithm (string, default XorShift128Plus::class?) and an *optional* seed (int, default time()?) and construct the source internally (and don't expose it)? So we could do just `$rng = new Randomizer(); to rely on the defaults (then call $rng->generateInt($min, $max);` $n times). - Finally, I think the vote should be split into 3 parts: the new classes, the deprecations (which itself could be split between [mt_]srand and [mt_]rand), and the change of internal RNG for shuffle/str_shuffle/array_rand. Best regards, -- Guilliam Xavier