Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:112928
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>
In-Reply-To: <CABTDVhA2mitRV6ScDs4Q=zGiyA8tzUeSX1srKMRF44iFbj1Z-w@mail.gmail.com>
Date: Tue, 19 Jan 2021 17:19:23 +0100
Message-ID: <CAF+90c-wzapKOw+gbGHreiq=J64o50Xsys4iRvYt9V8Mimf3TA@mail.gmail.com>
To: Go Kudo <zeriyoshi@gmail.com>
Cc: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000987ad205b943340c"
Subject: Re: [PHP-DEV] Re: [RFC] Object-scope RNG implementation
From: nikita.ppv@gmail.com (Nikita Popov)

--000000000000987ad205b943340c
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

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()` metho=
d;
> the mt_rand() function returns a bit-shifted unsigned 31bit number, so th=
e
> results are not identical.
>
> The `rng_range()` function has been renamed to `rng_rand()` and returns a=
n
> 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@gm=
ail.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. Us=
e
> > 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

--000000000000987ad205b943340c--