Re: Re: [RFC] Object-scope RNG implementation

> but I don't think rng_rand() should support calling without arguments. This is a backwards-compatibility leftover in mt_rand() and we should not carry it over into a new API. While this is true, the current implementation of MT in PHP relies on global state, and I believe that having a compatible implementation that can eliminate this is a good option. If PHP supports some parallel implementation in the future, and mt_srand() and mt_rand() are deprecated, it may be useful as an alternative method. > You should probably mention that the classes can be serialized in the RFC. I had forgotten about this one. I've added it. Thanks. > Naming I think this needs to be discussed further. The name RNG\RNGInterface is a compromise at this point, and is mainly based on the PSR conventions. However, PSR is only a userland convention and I am not sure if the core should take this into account. However, we also believe that the name is easy to understand, although it is somewhat redundant. > 64-bit I've been thinking for a while about what to do since then, and I think your suggestion is probably the most appropriate. That is, remove the RNG64Interface and the next64() method, and use the rng_range() function to generate random numbers beyond 32 bits. However, there is a concern. RNG implementations will no longer be able to return native 64-bit random numbers for seed values. Given the class name XorShift128Plus , users might expect to return the number 6233086606872742541 when 12345 is the seed value. On the other hand, it is possible to pseudo-generate 64-bit random numbers by bit-shifting even for 32-bit RNGs. I thought of using this to include next64() in the RNGInterface itself, but then the problem of userland implementation comes into play. Originally, the next64() method should always throw an exception if PHP_INT_SIZE >= 8, but if you allow userland implementation, it is implementation dependent. To solve these problems, how about adding a function like the following? It doesn't seem like a very good idea, but... rng_next(RNG\RNGInterface $rng, bool $unsigned = true): int rng_next64(RNG\RNGInterface $rng, bool $unsigned = true): int These will do a bit shift and return an unsigned integer if $unsigned is true. Otherwise, they return the value as generated by the RNG. Of course, if rng_next64() is called in a 32bit environment, it will throw a ValueError exception. In this case, however, the interface is no longer an interface. Perhaps it should be an abstract class, like this ```php abstract class AbstractRNG { abstract protected function next(): int; abstract protected function next64(): int; } ``` It may also be useful to have a trait like this ```php trait RNG64Support { abstract function next(): int; protected function next64(): int { $ret = $this->next(); return $ret << 32 | $this->ret(); } } ``` It's pretty "exotic" for a core implementation. :o Does anyone have any good ideas? After all this time, I'm not good at English, and I apologize if this is inappropriate. Regards, Go Kudo 2021年1月20日(水) 1:19 Nikita Popov <nikita.ppv@gmail.com>: > On Mon, Jan 18, 2021 at 5:29 PM Go Kudo <zeriyoshi@gmail.com> wrote: > >> RFC and implementation updated. >> >> https://wiki.php.net/rfc/object_scope_prng >> https://github.com/php/php-src/pull/6568 >> >> > MT19937 returns a signed 32bit number as a result of the >> > next() >> method; >> the mt_rand() function returns a bit-shifted unsigned 31bit number, so the >> results are not identical. >> >> The rng_range() function has been renamed to >> rng_rand() and returns an >> unsigned 31-bit integer like the mt_rand() function if only >> the $rng >> argument is received. >> >> ```php >> mt_srand(1234); >> mt_rand() === rng_rand(new \RNG\MT19937(1234)); // true >> ``` >> >> This allows to completely replace the mt_rand() function >> with the >> RNG\MT19937. >> >> I said I wanted to initiate a vote, but there seems to be too much >> discussion missing for that. >> I'd like to wait and see for a while. >> >> Regards, >> Go Kudo >> >> 2021年1月17日(日) 20:02 Go Kudo <zeriyoshi@gmail.com>: >> >> > Updated the RFC and fixed the implementation. >> > Also made some additions to the RFC about when this feature might be >> > useful. >> > >> > RFC: https://wiki.php.net/rfc/object_scope_prng >> > Implementation PR: >> > https://github.com/php/php-src/pull/6568 (All CI >> > passed) >> > >> > The main points are as follows: >> > >> > - The implementation now meets all requirements. (maybe) >> > - Implemented MT19937, which is compatible with PHP standard MT. The >> > consistency of these is ensured by tests. >> > - The implementation is now what we used to call TypeII. >> > - All RNG implementations can now be inherited like regular classes. Use >> > faster calls only if the class is an internal class. >> > - The implementation has been heavily modified and the quality of the >> code >> > has been improved. >> > >> > Currently, there are a few concerns: >> > >> > - MT19937 returns a signed 32bit number as a result of the >> > next() >> > method; the mt_rand() function returns a bit-shifted unsigned 31bit >> number, >> > so the results are not identical. >> > - You can get the equivalent result with `$rng->next() << 1 & >> > PHP_INT_MAX, but maybe you should have a >> > nextUnsigned31()` method. >> > - Currently, the test uses $rng->next() << 1 & >> > PHP_INT_MAX. >> > - The implementation of MT19937 is redundant. This can be merged. >> > >> > Anyway, I think the code is finally ready for review. >> > Once that is done, we will start voting on RFCs. >> > >> > Regards, >> > Go Kudo >> > > Overall I like the proposal. Some notes: > > * I know you only just changed it, but I don't think rng_rand() should > support calling without arguments. This is a backwards-compatibility > leftover in mt_rand() and we should not carry it over into a new API. > > * You should probably mention that the classes can be serialized in the > RFC. Maybe you want to write out all the methods so it's also visible how > the constructors for the classes look like. > > * Naming: RNG\RNGInterface seems a bit redundant. Of course, we can't make > it RNG\Interface. RNG\Generator would be an option. I'm not sure about > this. Maybe RNG\RNGInterface is fine. > > * 64-bit: I looked over your implementation, and I think your approach to > handling 64-bits is correct. You only call next64() if the requested range > is larger than 32-bit, and that can only happen on 64-bit systems, thus > guaranteeing consistency of results (where they can be consistent at all).. > What I'm unsure about is the way this gets exposed to userland via next() > and next64() methods. I think fetching of 64-bit values is mainly > interesting as an internal optimization, and not so much important to be > part of the RNGInterface API. The user is intended to get numbers via > rng_rand(), not by directly calling next(). A possibility here would be to > drop the RNG64Interface and next64() method, have next() always return > 32-bit, but retain the internal next64 API. For this to make sense, we'd > need two subsequent internal next() calls to return the same data as a > single next64() call (i.e. store the result and first return one half then > the other). Would this make sense to you? > > Regards, > Nikita >