Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:112932
MIME-Version: 1.0
References: <CABTDVhCtBn7T0YyB+55Mxxz1UCYi7ov_Se2nX-Uxh3EmK4XkdA@mail.gmail.com>
 <CABTDVhC_0DUxjSc+axXuOCyt=Vch2mFc0c9F4Hc=Qf8LPzYRXA@mail.gmail.com>
 <CABTDVhA2mitRV6ScDs4Q=zGiyA8tzUeSX1srKMRF44iFbj1Z-w@mail.gmail.com> <CAF+90c-wzapKOw+gbGHreiq=J64o50Xsys4iRvYt9V8Mimf3TA@mail.gmail.com>
In-Reply-To: <CAF+90c-wzapKOw+gbGHreiq=J64o50Xsys4iRvYt9V8Mimf3TA@mail.gmail.com>
Date: Wed, 20 Jan 2021 02:48:05 +0900
Message-ID: <CABTDVhCGC-ykWwFn7GKmAQ=ACjZMq3oVx0oTG9qYUX0mQvRCTQ@mail.gmail.com>
To: internals@lists.php.net
Content-Type: multipart/alternative; boundary="00000000000076540505b9447148"
Subject: Re: [PHP-DEV] Re: [RFC] Object-scope RNG implementation
From: zeriyoshi@gmail.com (Go Kudo)

--00000000000076540505b9447148
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

> 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 >=3D 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 =3D true): int`
`rng_next64(RNG\RNGInterface $rng, bool $unsigned =3D 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 =3D $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=E5=B9=B41=E6=9C=8820=E6=97=A5(=E6=B0=B4) 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 t=
he
>> 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() =3D=3D=3D 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=E5=B9=B41=E6=9C=8817=E6=97=A5(=E6=97=A5) 20:02 Go Kudo <zeriyoshi@g=
mail.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. U=
se
>> > 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 mak=
e
> 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 rang=
e
> 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 t=
o
> 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 the=
n
> the other). Would this make sense to you?
>
> Regards,
> Nikita
>

--00000000000076540505b9447148--