Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:114794
MIME-Version: 1.0
References: <CABTDVhC_h-+pG54KuJu6g-CFQPqaMJ=PUF02FMjjENvP1dpfzg@mail.gmail.com>
 <CABTDVhDVE4JnAtLKCr8y+s_48LgNz_MM2G2OWscTkWLaL+wFTA@mail.gmail.com>
In-Reply-To: <CABTDVhDVE4JnAtLKCr8y+s_48LgNz_MM2G2OWscTkWLaL+wFTA@mail.gmail.com>
Date: Wed, 9 Jun 2021 12:07:38 +0200
Message-ID: <CAC5V6gh-ueUYUAUVx6RE4tuTObpGS4Sphhu0gKXRGYq7Te--cg@mail.gmail.com>
To: Go Kudo <zeriyoshi@gmail.com>
Cc: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="0000000000006f58f105c4527297"
Subject: Re: [PHP-DEV] Re: [RFC] Under Discussion: Add Random class and
 RandomNumberGenerator interface
From: guilliam.xavier@gmail.com (Guilliam Xavier)

--0000000000006f58f105c4527297
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hello Go Kudo,

On Tue, Jun 8, 2021 at 2:33 PM Go Kudo <zeriyoshi@gmail.com> wrote:

> Hello iinternals.
>
> There doesn't seem to be much mention of it.
> But, that may be because it has been discussed well in advance.
> Thank you for participating in the discussion.
>

I'm not sure you really addressed https://externals.io/message/114561#11468=
0
? especially this part:

"""
If you pick the second option, then you should consistently separate the
source for both extension-provided RNGs and user-provided ones. I don't
think it makes sense to have extension provided RNGs use a `new
Random(RANDOM_XORSHIFT128PLUS)` interface, while user-provided ones use
`new Random(new SomeRandomSource)`. Going down this route, it should be
`new Random(new XorShift128Plus)`. This is a pure design, which also means
that it's more complicated, and may make simple usages harder (though there
is certainly room for a Random::createDefault() here.)
"""

Now, that might sound like it would take us back to the "full OO" version
(with multiple classes), which was deemed too "user-*un*friendly" by
several people (including me :s),
*but* there are some new ideas to consider, e.g. (for the Random class):

- add a `public static function createDefault(): self` as suggested by
Nikita, or

- change `public function __construct` parameters to e.g.
  `(string $algo =3D XorShift128Plus::class, ?int $seed =3D null, mixed
...$extra_args)`
  (that would internally do a `new $algo($seed, ...$extra_args)` to store),
  to be called not as
  `new Random(new UserDefinedRNG($seed_or_null, $foo, $bar))` but as
  `new Random(UserDefinedRNG::class, $seed_or_null, $foo, $bar)`.

Any new opinions (or other ideas)?


> Now, if there is no particular discussion on this, I will try to start th=
e
> voting phase next week.
> Of course, I will contact you separately.
>
> However, I was looking back at the implementation and found only one poin=
t
> of concern.
>
> With the current implementation, the results of the following example wil=
l
> match.
>
> ```php
> $one =3D new Random();
> $one->nextInt();
> $two =3D clone $one;
>
> var_dump($one->nextInt() =3D=3D=3D $two->nextInt()); // true
> ```
>
> But, this is not true for user implementations.
>
> ```php
> class UserRNG implements RandomNumberGenerator
> {
>     protected int $current =3D 0;
>
>     public function generate(): int
>     {
>         return ++$this->current;
>     }
> }
>
> $rng =3D new UserRNG();
> $one =3D new Random($rng);
> $one->nextInt();
> $two =3D clone $one;
>
> var_dump($one->nextInt() =3D=3D=3D $two->nextInt()); // false
> ```
>
> This is because `$rng` is kept as a normal property and is managed by the
> standard PHP mechanism. In other words, it is passed by reference.
>
> This is not consistent with the built-in RNG behavior. However, I don't s=
ee
> a problem with this behavior.
> I feel that the standard PHP behavior is preferable to changing the
> userland behavior in a specific way.
>
> I would like to solicit opinions.
>

Couldn't the Random class simply implement `public function __clone():
void` (that would internally do like `$this->algo =3D clone $this->algo;`)?


>
> Regards,
> Go Kudo
>
> 2021=E5=B9=B46=E6=9C=881=E6=97=A5(=E7=81=AB) 23:28 Go Kudo <zeriyoshi@gma=
il.com>:
>
> > Hello internals.
> > Thanks for continuing to participate in the discussion.
> >
> > I've sorted out the proposal, and finished writing and implementing the
> > RFC.
> > (Funny, I know.) I think it's time to start a discussion.
> >
> > https://wiki.php.net/rfc/rng_extension
> > https://github.com/php/php-src/pull/7079
> >
> > The main changes since last time are as follows:
> >
> > - The ugly RANDOM_USER has been removed.
> > - RandomNumberGenerator interface has been added for user-defined RNGs.
> > - Random class is now final.
> > - Random class now accepts a RandomNumberGenerator interface other than
> > string as the first argument to the constructor.
> > - INI directive has been removed. In 32-bit environments, the result is
> > always truncated.
> >
> > What I'm struggling with now is the behavior when calling nextInt() in =
a
> > 32-bit environment using a 64-bit RNG. It currently returns a truncated
> > result, which means that the code loses compatibility with the result o=
f
> > running on a 64-bit machine.
> > I was also considering throwing an exception, but which would be
> > preferable?
>

Indeed, a decision has to be made. If you throw an exception, we wouldn't
have the "issue" of different results on 32- vs 64-bit machines, but
XorShift128+ and Secure would be unusable on a 32-bit machine, right?
How do other existing implementations handle this question?

>
> > I would like to answer a few unanswered questions.
> >
> > > What is bias?
> >
> > I' ve confirmed that the bias issue in mt_rand has already been fixed a=
nd
> > is no longer a problem.
> >
> > This implementation copies most of it from mt_rand. Therefore, this
> > problem does not exist.
> >
> > Therefore, an equivalent method to mt_getrandmax() is no longer provide=
d.
>

Great!


> >
> > > uint64 & right-shift
> >
> > This is a specification of the RNG implementation. PHP does not have
> > unsigned integers, but RNG handles unsigned integers (to be precise, ev=
en
> > the sign bit is random).
> >
> > As mentioned above, PHP does not have unsigned integers, which means th=
at
> > using the results generated by RNGs may cause compatibility problems in
> the
> > future. To avoid this, a 1-bit right shift is always required (similar =
to
> > mt_rand()).
>

Good to know, thanks.

Regards,

--=20
Guilliam Xavier

--0000000000006f58f105c4527297--