[RFC] Under Discussion: Add Random class and RandomNumberGenerator interface

It was good! I was finally able to put some things together.
Thank you for participating in the discussion.

typo

Thanks, I fixed it!

InvalidArgumentException

I think InvalidArgumentException is included in the ext/spl extension, but
is it safe to use it from ext/standard?
If it is available, then it is certainly more appropriate.

Regards,
Go Kudo

2021年6月2日(水) 17:51 Jordi Boggiano j.boggiano@seld.be:

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.

Overall this looks much better! From a PHP userland perspective I can't
see any huge problem at first glance.

Just a few notes:

• "Random class can be serialized or cloned if the algorithm supports
  it." It isn't clear to me how that support is defined in a userland
  implementation? Simply by implementing __serialize/__unserialize?
• The __unserialize docblock has two typos (Useriialize and in
  instead of if)
• If an object is passed to new Random($obj), probably it should throw
  an InvalidArgumentException if a seed is also passed in, as I assume in
  that case it would be otherwise be ignored.

Best,
Jordi

--
Jordi Boggiano
@seldaek - https://seld.be

--

To unsubscribe, visit: https://www.php.net/unsub.php

Sorry, I missed the first question.

"Random class can be serialized or cloned if the algorithm supports it."
It isn't clear to me how that support is defined in a userland
implementation? Simply by implementing __serialize/__unserialize?

Userland implementations can be serialize / unserialize using the standard
PHP serialization mechanism.

For example, the following:

class UserRNG implements RandomNumberGenerator
{
    protected int $current = 0;

    public function generate(): int
    {
        return ++$this->current;
    }
}

$random = new Random(new UserRNG());
$random->nextInt();
$random_serialized = serialize($random);
var_dump($random_serialized);
//
O:6:"Random":2:{i:0;a:1:{s:11:"Randomrng";O:7:"UserRNG":1:{s:10:"*current";i:1;}}i:1;N;}

$random_unserialized = unserialize($random_serialized);
var_dump($random->nextInt() === $random_unserialized->nextInt()); // true

Regards,
Go Kudo

2021年6月2日(水) 17:51 Jordi Boggiano j.boggiano@seld.be:

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.

Overall this looks much better! From a PHP userland perspective I can't
see any huge problem at first glance.

Just a few notes:

• "Random class can be serialized or cloned if the algorithm supports
  it." It isn't clear to me how that support is defined in a userland
  implementation? Simply by implementing __serialize/__unserialize?
• The __unserialize docblock has two typos (Useriialize and in
  instead of if)
• If an object is passed to new Random($obj), probably it should throw
  an InvalidArgumentException if a seed is also passed in, as I assume in
  that case it would be otherwise be ignored.

Best,
Jordi

--
Jordi Boggiano
@seldaek - https://seld.be

--

To unsubscribe, visit: https://www.php.net/unsub.php

Hello Go Kudo,

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#114680
? 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-unfriendly" 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 = XorShift128Plus::class, ?int $seed = 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 the
voting phase next week.
Of course, I will contact you separately.

However, I was looking back at the implementation and found only one point
of concern.

With the current implementation, the results of the following example will
match.

$one = new Random();
$one->nextInt();
$two = clone $one;

var_dump($one->nextInt() === $two->nextInt()); // true

But, this is not true for user implementations.

class UserRNG implements RandomNumberGenerator
{
    protected int $current = 0;

    public function generate(): int
    {
        return ++$this->current;
    }
}

$rng = new UserRNG();
$one = new Random($rng);
$one->nextInt();
$two = clone $one;

var_dump($one->nextInt() === $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 see
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 = clone $this->algo;)?

Regards,
Go Kudo

2021年6月1日(火) 23:28 Go Kudo zeriyoshi@gmail.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 of
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 and
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 provided.

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, even
the sign bit is random).

As mentioned above, PHP does not have unsigned integers, which means that
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,

--
Guilliam Xavier