[VOTE] Object scoped RNG implementation

php.internals

zeriyoshi

5 years ago
Hello everyone. Object scoped RNG RFC vote is now open. https://wiki.php.net/rfc/object_scope_prng The deadline for voting is April 15. Previous discussions can be viewed below. https://externals.io/message/112765 https://externals.io/message/112819 https://externals.io/message/113720 Regards, Go Kudo

Max Semenik

5 years ago
The "Proposed Voting Choices" section title needs an update. чт, 1 апр. 2021 г., 07:39 Go Kudo <zeriyoshi@gmail.com>:

zeriyoshi

5 years ago
I fixed it. https://wiki.php.net/rfc/object_scope_prng Thank you. 2021年4月1日(木) 15:23 Max Semenik <maxsem.wiki@gmail.com>:

Guilliam Xavier

5 years ago
On Thu, Apr 1, 2021 at 6:39 AM Go Kudo <zeriyoshi@gmail.com> wrote:
> Hello everyone. > > Object scoped RNG RFC vote is now open. > https://wiki.php.net/rfc/object_scope_prng > > The deadline for voting is April 15. > > Previous discussions can be viewed below. > > https://externals.io/message/112765 > https://externals.io/message/112819 > https://externals.io/message/113720 > > Regards, > Go Kudo >
Hello, If I understand correctly, this would - provide us with performant object-encapsulated (isolated from global state) equivalents of the existing `mt_srand()`/`mt_rand()` and `random_int()` functions as well as a new "XorShift128+"; along with some global functions for usability, notably `rng_bytes()` and `rng_int()` respectively similar to `random_bytes()` and `random_int()` but with an additional (first) RNG argument - enable us to use them (the objects), as well as other (extension or userland) implementations of the `RNG\RNGInterface`, for the existing `array_rand()`, `shuffle()` and `str_shuffle()` functions (instead of the default internal MT implementation) ? I can't vote (yes), but I just suggest you complete the stubs in the RFC with what can been seen at < https://github.com/php/php-src/pull/6568/files?file-filters%5B%5D=.php#diff-d469bf38690924187e7b9db4a8ab199a99394b404bb12325a789141c56edb12f> and below, showing that the class `OS` has the minimal interface while classes `MT19937` and `XorShift128Plus` have a public `__construct(int $seed )` and implement the magic `__serialize()`/`__unserialize()`. Best regards,
-- Guilliam Xavier

zeriyoshi

5 years ago
Hi Xavier
> If I understand correctly, this would > - provide us with performant object-encapsulated (isolated from global
state) equivalents of the existing `mt_srand()`/`mt_rand()` and `random_int()` functions as well as a new "XorShift128+"; along with some global functions for usability, notably `rng_bytes()` and `rng_int()` respectively similar to `random_bytes()` and `random_int()` but with an additional (first) RNG argument
> - enable us to use them (the objects), as well as other (extension or
userland) implementations of the `RNG\RNGInterface`, for the existing `array_rand()`, `shuffle()` and `str_shuffle()` functions (instead of the default internal MT implementation) Yes, that's right.
> I can't vote (yes), but I just suggest you complete the stubs in the RFC
with what can been seen at < https://github.com/php/php-src/pull/6568/files?file-filters%5B%5D=.php#diff-d469bf38690924187e7b9db4a8ab199a99394b404bb12325a789141c56edb12f> and below, showing that the class `OS` has the minimal interface while classes `MT19937` and `XorShift128Plus` have a public `__construct(int $seed)` and implement the magic `__serialize()`/`__unserialize()`. I've added it to the RFC. Thank you!

Unnamed Person

5 years ago
On Wed, Mar 31, 2021 at 10:39 PM Go Kudo <zeriyoshi@gmail.com> wrote:
> > Hello everyone. > > Object scoped RNG RFC vote is now open. > https://wiki.php.net/rfc/object_scope_prng > > The deadline for voting is April 15. > > Previous discussions can be viewed below. > > https://externals.io/message/112765 > https://externals.io/message/112819 > https://externals.io/message/113720 > > Regards, > Go Kudo
Sorry if I missed an optional "intent to vote soon" thread. I voted no simply based on the fact that namespaced things are going into `ext/standard`. We have an open RFC discussion on this issue, which has had discussion as recently as 10 days ago. I cannot vote "yes" on anything involved with namespaces in this context in good conscience. At the same time, we shouldn't block things indefinitely.

zeriyoshi

5 years ago
Hi Morrison.
> Sorry if I missed an optional "intent to vote soon" thread.
I thought I had announced it here, but I did not indicate the specific dates. I apologize for that. https://externals.io/message/113720
> simply based on the fact that namespaced things are going
into`ext/standard`. Yes, I feel that the use of namespaces is an area that needs to be carefully discussed. The goal of this RFC is just to get such a feature into core, and we are not particularly concerned about namespaces. (In other words, we think it's fine to change RNG\RNGInterface to RNGInterface and RNG\XorShift128Plus to XorShift128PlusRNG and not use namespaces.) As per the RFC, we believe this feature is necessary for approaches like Fiber and Swoole, which have more complex execution order and state management. What if you have a problem with this feature being implemented in `ext/standard`? I think this is unavoidable since `shuffle()`, `array_rand()`, and `str_suffle()` are implemented in `ext/standard`. The original RFC suggested quarantining them as `ext/rng` to avoid this (PECL orng is mostly based on the original idea), but the global state dependent `shuffle()`, `array_rand()`, and `str_shuffle()` are implemented in `ext/standard`. ext/standard` are implemented and not deprecated, the possibility of unintentionally writing state-dependent code increases in the future. Let me know what you think. 2021年4月2日(金) 4:19 Levi Morrison <levi.morrison@datadoghq.com>:

Kamil Tekiela

5 years ago
Hi Go Kudo, First, let me say that I believe we need such implementation in PHP and I would like to see object scoped RNG as part of the standard. However, I have voted no for a number of reasons. Let me list them from the perspective of a noob PHP user. - I really do not understand why we are introducing new functions. Can't the classes implement the necessary methods to get integers, doubles, and string of bytes? As a new user I would be completely overwhelmed by all these functions: rand(), mt_rand(), rng_int(), rng_next(), rng_next64(). Which one should I use? What is the difference between rng_next() and rng_next64? - As soon as I left the RFC page I forgot what the new classes were called. I still can't tell from memory what's their name. I understand what they mean, but they are definitely not friendly names. - What's the difference between MT19937 and XorShift128Plus? They are different algorithms but which one should I pick? I tested the implementation locally and I see no difference in performance. - I am not a fan of adding a new optional parameter to shuffle() and friends. I'd prefer to have a method in the class that I can pass an array to. - What is the default seed? Do I have to provide a seed each time? Why can't the seed be done automatically? - Signed? Unsigned? As far as I know, PHP doesn't have unsigned integers. What's the real-life purpose of this flag? - I don't see any use in supporting userland implementations. Why can't they create separate libraries? I don't know about performance, but if someone wants to have custom RNG then I don't think they worry about performance. - When using the functions the performance was 50% worse than when calling ->next() directly. Is this right or is the implementation going to be optimized further? The fastest way to get a random number seems to be mt_rand() based on my tests. I would rather like to see a single class called RNG/Random that implements RNG/RandomInterface. The constructor of the class would take 2 arguments. The first is the algorithm with a default either MT or XORShift. The second is an optional seed. If no seed is provided then the seed is generated automatically like in mt_srand(). The class would then implement methods like: nextInt(), nextDouble(), nextBytes(), arrayShuffle(), stringShuffle(), randomArrayKeys(). I would keep the standard functions as they are. Let them use MT by default. We could even deprecate them in future if this takes off. This would make it painfully obvious what the class does and how to use it. No more procedural code. I would also make the class final so that you can't inherit from it, but that is highly opinion-based. Now that I have written this, I read previous conversations and it looks to me like what I would like is what you had previously. I'm sorry if I complain too much, but I would like to see something like this implemented, just not like you are proposing right now. It is too messy for me and I know I wouldn't like it if I had to use it. Regards, Kamil

Marco Pivetta

5 years ago
On Fri, Apr 2, 2021, 23:56 Kamil Tekiela <tekiela246@gmail.com> wrote:
> Hi Go Kudo, > > First, let me say that I believe we need such implementation in PHP and I > would like to see object scoped RNG as part of the standard. However, I > have voted no for a number of reasons. Let me list them from the > perspective of a noob PHP user. > > - I really do not understand why we are introducing new functions. Can't > the classes implement the necessary methods to get integers, doubles, and > string of bytes? As a new user I would be completely overwhelmed by all > these functions: rand(), mt_rand(), rng_int(), rng_next(), rng_next64(). > Which one should I use? What is the difference between rng_next() > and rng_next64? > - As soon as I left the RFC page I forgot what the new classes were called. > I still can't tell from memory what's their name. I understand what they > mean, but they are definitely not friendly names. > - What's the difference between MT19937 and XorShift128Plus? They are > different algorithms but which one should I pick? I tested the > implementation locally and I see no difference in performance. > - I am not a fan of adding a new optional parameter to shuffle() and > friends. I'd prefer to have a method in the class that I can pass an array > to. > - What is the default seed? Do I have to provide a seed each time? Why > can't the seed be done automatically? > - Signed? Unsigned? As far as I know, PHP doesn't have unsigned integers. > What's the real-life purpose of this flag? > - I don't see any use in supporting userland implementations. Why can't > they create separate libraries? I don't know about performance, but if > someone wants to have custom RNG then I don't think they worry about > performance. > - When using the functions the performance was 50% worse than when calling > ->next() directly. Is this right or is the implementation going to be > optimized further? The fastest way to get a random number seems to be > mt_rand() based on my tests. > > I would rather like to see a single class called RNG/Random that implements > RNG/RandomInterface. The constructor of the class would take 2 arguments. > The first is the algorithm with a default either MT or XORShift. The second > is an optional seed. If no seed is provided then the seed is generated > automatically like in mt_srand(). The class would then implement methods > like: nextInt(), nextDouble(), nextBytes(), arrayShuffle(), > stringShuffle(), randomArrayKeys(). I would keep the standard functions as > they are. Let them use MT by default. We could even deprecate them in > future if this takes off. > > This would make it painfully obvious what the class does and how to use it. > No more procedural code. I would also make the class final so that you > can't inherit from it, but that is highly opinion-based. > Now that I have written this, I read previous conversations and it looks to > me like what I would like is what you had previously. > > I'm sorry if I complain too much, but I would like to see something like > this implemented, just not like you are proposing right now. It is too > messy for me and I know I wouldn't like it if I had to use it. >
Changed my vote to "no" after this in-detail review. Overall, I was conflicted, and voted "yes" especially because we do really need a better and reproducible seeding solution for RNG, and the current API is insufficient. The API proposed so far is quite terrible though, and if the vote passed, we'd have to stick with it for an infinity of years.

Larry Garfield

5 years ago
On Fri, Apr 2, 2021, at 4:56 PM, Kamil Tekiela wrote:
> Hi Go Kudo, > > First, let me say that I believe we need such implementation in PHP and I > would like to see object scoped RNG as part of the standard. However, I > have voted no for a number of reasons. Let me list them from the > perspective of a noob PHP user. > > - I really do not understand why we are introducing new functions. Can't > the classes implement the necessary methods to get integers, doubles, and > string of bytes? As a new user I would be completely overwhelmed by all > these functions: rand(), mt_rand(), rng_int(), rng_next(), rng_next64(). > Which one should I use? What is the difference between rng_next() > and rng_next64? > - As soon as I left the RFC page I forgot what the new classes were called. > I still can't tell from memory what's their name. I understand what they > mean, but they are definitely not friendly names. > - What's the difference between MT19937 and XorShift128Plus? They are > different algorithms but which one should I pick? I tested the > implementation locally and I see no difference in performance. > - I am not a fan of adding a new optional parameter to shuffle() and > friends. I'd prefer to have a method in the class that I can pass an array > to. > - What is the default seed? Do I have to provide a seed each time? Why > can't the seed be done automatically? > - Signed? Unsigned? As far as I know, PHP doesn't have unsigned integers. > What's the real-life purpose of this flag? > - I don't see any use in supporting userland implementations. Why can't > they create separate libraries? I don't know about performance, but if > someone wants to have custom RNG then I don't think they worry about > performance. > - When using the functions the performance was 50% worse than when calling > ->next() directly. Is this right or is the implementation going to be > optimized further? The fastest way to get a random number seems to be > mt_rand() based on my tests. > > I would rather like to see a single class called RNG/Random that implements > RNG/RandomInterface. The constructor of the class would take 2 arguments. > The first is the algorithm with a default either MT or XORShift. The second > is an optional seed. If no seed is provided then the seed is generated > automatically like in mt_srand(). The class would then implement methods > like: nextInt(), nextDouble(), nextBytes(), arrayShuffle(), > stringShuffle(), randomArrayKeys(). I would keep the standard functions as > they are. Let them use MT by default. We could even deprecate them in > future if this takes off. > > This would make it painfully obvious what the class does and how to use it. > No more procedural code. I would also make the class final so that you > can't inherit from it, but that is highly opinion-based. > Now that I have written this, I read previous conversations and it looks to > me like what I would like is what you had previously. > > I'm sorry if I complain too much, but I would like to see something like > this implemented, just not like you are proposing right now. It is too > messy for me and I know I wouldn't like it if I had to use it. > > Regards, > Kamil
I also didn't pay close attention to the previous discussion, but reading the RFC I agree with all of this. The functionality proposed is good and needed, but the API is a mess. What Kamil suggests is far better, and fully commits to being OOPy. Given that the use case is for situations where you need a predictable and repeatable random sequence, such as Fibers or threads or such, going all-in on an object seems like the correct approach. One thing I'm not 100% clear on from the RFC, does this also deprecate random_int()/random_bytes()? Those are (AFAIK) unseeded, so they seem like they'd continue to serve their current purpose, but it's not clear from the RFC. Voting no for now, but I would welcome a resubmission with a cleaner API, even in this cycle. --Larry Garfield

Aaron Piotrowski

5 years ago
> On Apr 2, 2021, at 5:49 PM, Larry Garfield <larry@garfieldtech.com> wrote: > > On Fri, Apr 2, 2021, at 4:56 PM, Kamil Tekiela wrote: >> Hi Go Kudo, >> >> First, let me say that I believe we need such implementation in PHP and I >> would like to see object scoped RNG as part of the standard. However, I >> have voted no for a number of reasons. Let me list them from the >> perspective of a noob PHP user. >> >> - I really do not understand why we are introducing new functions. Can't >> the classes implement the necessary methods to get integers, doubles, and >> string of bytes? As a new user I would be completely overwhelmed by all >> these functions: rand(), mt_rand(), rng_int(), rng_next(), rng_next64(). >> Which one should I use? What is the difference between rng_next() >> and rng_next64? >> - As soon as I left the RFC page I forgot what the new classes were called. >> I still can't tell from memory what's their name. I understand what they >> mean, but they are definitely not friendly names. >> - What's the difference between MT19937 and XorShift128Plus? They are >> different algorithms but which one should I pick? I tested the >> implementation locally and I see no difference in performance. >> - I am not a fan of adding a new optional parameter to shuffle() and >> friends. I'd prefer to have a method in the class that I can pass an array >> to. >> - What is the default seed? Do I have to provide a seed each time? Why >> can't the seed be done automatically? >> - Signed? Unsigned? As far as I know, PHP doesn't have unsigned integers. >> What's the real-life purpose of this flag? >> - I don't see any use in supporting userland implementations. Why can't >> they create separate libraries? I don't know about performance, but if >> someone wants to have custom RNG then I don't think they worry about >> performance. >> - When using the functions the performance was 50% worse than when calling >> ->next() directly. Is this right or is the implementation going to be >> optimized further? The fastest way to get a random number seems to be >> mt_rand() based on my tests. >> >> I would rather like to see a single class called RNG/Random that implements >> RNG/RandomInterface. The constructor of the class would take 2 arguments. >> The first is the algorithm with a default either MT or XORShift. The second >> is an optional seed. If no seed is provided then the seed is generated >> automatically like in mt_srand(). The class would then implement methods >> like: nextInt(), nextDouble(), nextBytes(), arrayShuffle(), >> stringShuffle(), randomArrayKeys(). I would keep the standard functions as >> they are. Let them use MT by default. We could even deprecate them in >> future if this takes off. >> >> This would make it painfully obvious what the class does and how to use it. >> No more procedural code. I would also make the class final so that you >> can't inherit from it, but that is highly opinion-based. >> Now that I have written this, I read previous conversations and it looks to >> me like what I would like is what you had previously. >> >> I'm sorry if I complain too much, but I would like to see something like >> this implemented, just not like you are proposing right now. It is too >> messy for me and I know I wouldn't like it if I had to use it. >> >> Regards, >> Kamil > > I also didn't pay close attention to the previous discussion, but reading the RFC I agree with all of this. The functionality proposed is good and needed, but the API is a mess. What Kamil suggests is far better, and fully commits to being OOPy. Given that the use case is for situations where you need a predictable and repeatable random sequence, such as Fibers or threads or such, going all-in on an object seems like the correct approach. > > One thing I'm not 100% clear on from the RFC, does this also deprecate random_int()/random_bytes()? Those are (AFAIK) unseeded, so they seem like they'd continue to serve their current purpose, but it's not clear from the RFC. > > Voting no for now, but I would welcome a resubmission with a cleaner API, even in this cycle. > > --Larry Garfield >
I too voted no for similar reasons as Larry, Marco, Levi, and others. The API needs improvement and using a new namespace without precedent or a plan is problematic. Overall I’m in favor of an object-based approach and encourage you to rework the API and bring it to a vote for PHP 8.1. Cheers, Aaron Piotrowski

zeriyoshi

5 years ago
This RFC seems to have a lot of problems. But I would like to answer the question first.
> - I really do not understand why we are introducing new functions. Can't
the classes implement the necessary methods to get integers, doubles, and string of bytes? As a new user I would be completely overwhelmed by all these functions: rand(), mt_rand(), rng_int(), rng_ This was done with the intention of lightening the burden of RNG implementation in PHP. At the time of the discussion, it was assumed that RNGInterface would also provide alternative functions such as `shuffle()` and `array_rand()`.
> - As soon as I left the RFC page I forgot what the new classes were
called. I still can't tell from memory what's their name. I understand what they mean, but they are definitely not friendly names. The class name is based on the random number generation algorithm. As the namespace is RNG, I think this is straightforward enough.
> What's the difference between MT19937 and XorShift128Plus? They are
different algorithms but which one should I pick? I tested the implementation locally and I see no difference in performance. Basically, it assumes that XorShift128+ is used. MT19937 is implemented in case you need to reproduce the behavior of traditional MT.
> I am not a fan of adding a new optional parameter to shuffle() and
friends. I'd prefer to have a method in the class that I can pass an array to. As I have said many times before, this was the original implementation plan. The PECL orng extension provides functionality that is largely based on this.
> Signed? Unsigned? As far as I know, PHP doesn't have unsigned integers.
What's the real-life purpose of this flag? The idea is to use `RNG\MT19937` and `rng_next()` to make them behave the same as `mt_srand()` and `mt_rand()`. This will allow all `mt_srand()` to be replaced by the new object-based RNG implementation.
> I don't see any use in supporting userland implementations. Why can't
they create separate libraries? I don't know about performance, but if someone wants to have custom RNG then I don't think they worry about performance. This can be useful when you want to generate results from a pre-determined random number table. See this test case. https://github.com/php/php-src/blob/6834426452b151c36e4d1e292ad997d6bcf139ae/ext/standard/tests/rng/function_rng_bytes_string.phpt
> When using the functions the performance was 50% worse than when calling
->next() directly. Is this right or is the implementation going to be optimized further? The fastest way to get a random number seems to be mt_rand() based on my tests. This is the cost of increased versatility. I'm not so convinced about this either. Calling `next64()` in a 32bit environment will give incorrect results. `rng_next64()` determines the execution environment beforehand, and throws ValueError if it is not available. Since it is difficult to expect such "error checking" from a userland implementation, this is the way it is. This RFC ballot should be rescinded. There was too little discussion beforehand. I will create a new proposal based on the feedback I received. I would appreciate your active input so that this does not happen during the voting phase. 2021年4月3日(土) 6:56 Kamil Tekiela <tekiela246@gmail.com>: