Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:114530
MIME-Version: 1.0
References: <CABTDVhAd8r_OPsdbzxrFeZTZEHuZjKyPfcYhNcdfdSZWbUJ-Lw@mail.gmail.com>
 <CAC5V6giS2UFjHgPaiRiiMQKYEPxwR_34_vG6EQtMepgq7ReF4g@mail.gmail.com>
In-Reply-To: <CAC5V6giS2UFjHgPaiRiiMQKYEPxwR_34_vG6EQtMepgq7ReF4g@mail.gmail.com>
Date: Thu, 20 May 2021 21:42:30 +0900
Message-ID: <CABTDVhA=eK1V_EnZDJL7iiRdp5CK6prieVRNnZ6r0aRLxUoatQ@mail.gmail.com>
To: Guilliam Xavier <guilliam.xavier@gmail.com>, PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="00000000000072766405c2c247ca"
Subject: Re: [PHP-DEV] [RFC] [Draft] Add RNG extension and deprecate mt_srand()
From: zeriyoshi@gmail.com (Go Kudo)

--00000000000072766405c2c247ca
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Sorry. I couldn't make it in time for my lunch break.

>  How many bytes is `Source::next()` supposed to generate? Is it even
intended to be called directly (vs `Randomizer::generateBytes(int
$length)`)?

No, this is not intended to be used directly.
In fact, as many people have pointed out, I think it would be more
appropriate to merge it into a single class.

As per the original implementation (ext/orng), I didn't think it needed to
be separated originally. One of the comments on a previous RFC suggested
that it would probably be better to separate them, and I took that into
account.

> 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?

This is for backward compatibility. If it is not needed, we would like to
eliminate the implementation itself.
Well, to keep the language core clean, maybe this should be provided by an
appropriate external extension.

- `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.

You're absolutely right. I would like to remove this.

> 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()`?

As a matter of fact, we believe that the functions provided by this class
are sufficient for `mt_rand()`, `shuffle()`, `str_shuffle()`, and
`array_rand()` equivalents. At most, the equivalent of `random_byte()`
might be provided.
I'll reorganize these.

Thanks for the detailed remarks. Based on these, I would like to clean up
the RFC.

Regards,
Go Kudo

2021=E5=B9=B45=E6=9C=8819=E6=97=A5(=E6=B0=B4) 18:46 Guilliam Xavier <guilli=
am.xavier@gmail.com>:

>
>
> 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/wa=
nt
> 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 retu=
rn
> -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 =3D n=
ew
> XorShift128Plus($seed);` *and* `$rng =3D new Randomizer($source);` [even =
if
> $source (and $seed) could be inlined, that's still several "steps"], *the=
n*
> 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 undesirab=
le
> 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.st=
ub.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?) an=
d
> an *optional* seed (int, default time()?) and construct the source
> internally (and don't expose it)? So we could do just `$rng =3D 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
>

--00000000000072766405c2c247ca--