[RFC] Random Extension 5.x + Random Extension Improvement

[zeriyoshi]

RFC: https://wiki.php.net/rfc/rng_extension
Additional RFC: https://wiki.php.net/rfc/random_extension_improvement
Internals Threads:

• https://externals.io/message/118163 (latest)
• https://externals.io/message/117994 (2nd RFC)
• https://externals.io/message/117939 (1st RFC)
• https://externals.io/message/117840 (1st RFC)
• https://externals.io/message/117295 (1st RFC)

TODO

• Fix PCG64 algorithm (pcg_setseq_128 -> pcg_oneseq_128) PoC is done
• Fix incompatibility in Random\Engine\MersenneTwister with MT_RAND_PHP flags
• Structure overhaul and refactoring
• Make each Engine implementation a separate C file ([RFC] Random Extension 5.x + Random Extension Improvement #8094 (review))
• Improve RFC Implementation (https://wiki.php.net/rfc/random_extension_improvement)
  • Engine implementations to final
  • Add Random\Randomizer::pickArrayKeys(array $array, int $num): array
  • Rename Random\Randomizer::shuffleString() to Random\Randomizer::shuffleBytes()
  • Rename Random\Engine\PCG64 to Random\Engine\Pcg64OneseqXslRr64
  • Rename Random\Engine\MersenneTwister to Random\Engine\Mt19937
  • Implement Random\Engine\Xoshiro256StarStar
• Code review
• Merge into master

[TimWolla]

I've found one more issue: Serialization of MersenneTwister is not correct:

<?php

use Random\Engine\MersenneTwister;

$g1 = new MersenneTwister(1, MT_RAND_PHP);
$g2 = unserialize(serialize($g1));

for ($i = 0; $i < 10000; $i++) {
  if ($g1->generate() !== $g2->generate()) {
    var_dump($i);
    exit;
  }
}
echo "Success";

[zeriyoshi]

I've found one more issue: Serialization of MersenneTwister is not correct:

<?php

use Random\Engine\MersenneTwister;

$g1 = new MersenneTwister(1, MT_RAND_PHP);
$g2 = unserialize(serialize($g1));

for ($i = 0; $i < 10000; $i++) {
  if ($g1->generate() !== $g2->generate()) {
    var_dump($i);
    exit;
  }
}
echo "Success";

fixed: 007b02c

[zeriyoshi]

@TimWolla
I have sent a pre-announcement of the start of the RFC voting to the Internals ML.
If you have any additional suggestions, I would be happy to reply to the email.

Sorry for the delay in responding. Thank you!

[cmb69]

Should the extension be listed in EXTENSIONS and have a CREDITS file to be listed in phpinfo?

Indeed, it should.

[zeriyoshi]

@TimWolla @cmb69
In this case, who should I designate as the main maintainer? I am willing to do ongoing maintenance, but not the main committer currently.

[cmb69]

In this case, who should I designate as the main maintainer?

Don't spend too much time thinking about that. The info in that file is rather informal. Just add yourself as primary maintainer, and also @TimWolla if he wants that. That also doesn't mean that you are responsible for the extension; it's more like a hint that you may be the best person to reach out if there are any issues with that extension.

[bukka]

Looks good to me for what it's worth.

[bukka]

@zeriyoshi Could you squash it please?

[TimWolla]

Just add yourself as primary maintainer, and also @TimWolla if he wants that.

@zeriyoshi @cmb69 Sure, feel free to list me as a co-maintainer (use either [email protected] or [email protected]). Significant parts of the design of the extension are based on my suggestions and I'll be happy at the very least to review future evaluations of the extension.

[guilliamxavier]

A couple nits

[bukka]

@zeriyoshi Just a reminder that the feature is in 24 hours and something so it would be good get it merged soon. Is there anything else that you want to do before merging? As I noted, the squashing that to a single commit or couple standalone commits if you prefer would be appreciated. I can handle the merging if no one else wants to do that.

[zeriyoshi]

@zeriyoshi Just a reminder that the feature is in 24 hours and something so it would be good get it merged soon. Is there anything else that you want to do before merging? As I noted, the squashing that to a single commit or couple standalone commits if you prefer would be appreciated. I can handle the merging if no one else wants to do that.

@bukka
I will do this immediately.
I was working on a personal tasks, as some discussions had not been completed and existing bugs were causing CI to fail as well.
For the time being, I will squash it through CI. Please wait.

[zeriyoshi]

@bukka @TimWolla @cmb69 @bwoebi @kocsismate @guilliamxavier

All commits are combined into one. However, some records of the parts that were suggestion have been lost along the way. However, I think this is not a problem because we can refer to it on the pull request. What do you think?

Also, I have worked around the Assertion Failed problem on Windows by adding a workaround. This is already a separate issue and should be addressed separately.

As far as I can see, this PR is already ready to merge. What do you think?

I will create a follow-up MR as soon as this is merged to fix the issues mentioned in the PR and ML.

• https://externals.io/message/118163#118269
• https://github.com/php/php-src/pull/8094/files#r919693108
• https://github.com/php/php-src/pull/8094/files#r917308676

[zeriyoshi]

P.S. Branches before being squashed can be found at: https://github.com/colopl/php-src/tree/upstream_rfc/scoped_rng_backup

[zeriyoshi]

CI passed 😎

[TimWolla]

Tests are unmodified and I eyeballed the newest changes, this still LGTM and IMO is ready to merge.

[TimWolla]

@bukka Will you handle the merge, please? 😃

[bukka]

This has been merged via 4d8dd8d - just moved some bits from NEWS to UPGRADING otherwise it was perfectly ready. Thanks! Great work!

[guilliamxavier]

Sorry I just noticed: here was introduced a Git warning "No newline at end of file"