Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:115325
MIME-Version: 1.0
References: <CABTDVhD9SKNVFrt8NMj4BqdHmsMpSt0DH=SMmb-9C905rgc7PA@mail.gmail.com>
 <CAF+90c9LNqgF9JJgXY_cNbX4hY9hU2QaCNViNpy25YFLeKgzLQ@mail.gmail.com> <CABTDVhCJge1sodik-xqdGqJ1seU6xHWjwXhUSQK_-+h5Lqyqdg@mail.gmail.com>
In-Reply-To: <CABTDVhCJge1sodik-xqdGqJ1seU6xHWjwXhUSQK_-+h5Lqyqdg@mail.gmail.com>
Date: Tue, 6 Jul 2021 16:35:02 +0200
Message-ID: <CAF+90c9Az5LDAY7yarJVEv6B+hyK6rTo_o_YQv9yc+VBqkn0_A@mail.gmail.com>
To: Go Kudo <zeriyoshi@gmail.com>
Cc: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000c0eb3b05c67554a6"
Subject: Re: [PHP-DEV] [RFC] Add Random Extension (before: Add Random class)
From: nikita.ppv@gmail.com (Nikita Popov)

--000000000000c0eb3b05c67554a6
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Jul 2, 2021 at 3:58 PM Go Kudo <zeriyoshi@gmail.com> wrote:

> >  * The first bit is just clarification. After a cursory look at the
> implementation, my understanding is that the getInt(), shuffleArray() and
> shuffleString() APIs will always produce consistent results on 32-bit and
> 64-bit, as long as your inputs are 32-bit as well (i.e., min/max are 32-b=
it
> and string is smaller than 4G). Is that correct? The only APIs that would
> exhibit different behavior are nextInt() and getBytes(), right?
>
> Yes. I do not want to break the compatibility of the implementation. I
> would prefer to be able to migrate code that uses the current internal
> state.
>
> >  * Looking at the implementation, nextInt() performs a >> 1 operation o=
n
> the RNG result. I assume the motivation is to get back a non-negative
> number. But why do we want that? The "nextInt()" name doesn't really
> indicate that it's a positive number. I think more generally, my question
> here may be "Why does this method exist at all? When would you use it
> instead of getInt()?"
>
> This was to allow for future forward compatibility. When PHP_INT_SIZE
> exceeds 8, the result will be incompatible without bit shifting. This is
> similar to the way mt_rand() does bit shifting now.
>
> However, I can agree that such a day will never come in reality. And as
> the comments on GitHub show, there are ways to keep the values compatible
> even if such a time comes.
>
> After thinking about it for a while, I finally came to the conclusion tha=
t
> there is no benefit to this other than to make mt_rand() and
> Random\NumberGenerator\MT19937 directly compatible.
> If compatibility is needed, it can be achieved by bit shifting in the PHP
> code, so direct compatibility is probably unnecessary. I will change the
> implementation and remove this option.
>
> > "Why does this method exist at all? When would you use it instead of
> getInt()?"
>
> The case for this would be if you want to get a raw unrounded random
> number sequence as a number. The situations where this is required would
> certainly be limited, but it would be nice to have. (At least, I know of
> several production codes that use the result of mt_rand() with no
> arguments.)
>

Could you share some example of where you use it? Maybe that will help
understand the motivation for it.

Also, I think it's worth pointing out that it's always possible to use
$random->getNumberGenerator()->generate() to access the raw RNG stream.

Regards,
Nikita

>  * I don't really get why we need RandomInterface. I think if the choice
> is between "final + interface" and "non-final without interface", I'd
> prefer the latter (though I'm also happy with "final without interface").
>
> I had completely lost my train of thought on this. The interface makes th=
e
> Random class unextensible. I have removed this.
>
> >  I'm not entirely happy with the naming. Unfortunately, I don't have
> great suggestions either. I think in your current hierarchy, I would make
> the interface Random\NumberGenerator (with implementation in the
> sub-namespace), rather than Random\NumberGenerator\RandomNumberGenerator.
>
> Deep-rooted problem. For now, I'm going to change RandomNumberGenerator t=
o
> Random\NumberGenerator. It's the best one so far.
>
>
> I continue to be plagued by Valgrind warnings and crashes of Windows ZTS
> builds...
> I'd like to make a voting phase that is fixed ...
>
> Regards,
> Go Kudo
>
> 2021=E5=B9=B46=E6=9C=8829=E6=97=A5(=E7=81=AB) 23:01 Nikita Popov <nikita.=
ppv@gmail.com>:
>
>> On Sat, Jun 26, 2021 at 2:40 AM Go Kudo <zeriyoshi@gmail.com> wrote:
>>
>>> Hello Internals.
>>>
>>> RFC has been reorganized for finalization.
>>>
>>> https://wiki.php.net/rfc/rng_extension
>>>
>>> The changes from the previous version are as follows:
>>>
>>> - Changed again to a class-based approach. The argument can be omitted,
>>> in
>>> which case an instance of XorShift128Plus will be created automatically=
.
>>> - Future scope was specified in the RFC and the functionality was
>>> separated
>>> as a Random extension.
>>> - Changed to separate it as a Random extension and use the appropriate
>>> namespace.
>>> - In order to extend the versatility of the final class, Random, a
>>> RandomInterface has been added, similar in approach to the
>>> DateTimeInterface.
>>>
>>
>> The updated proposal looks quite nice :) I think this is close to done.
>> Some small bits of feedback:
>>
>>  * The first bit is just clarification. After a cursory look at the
>> implementation, my understanding is that the getInt(), shuffleArray() an=
d
>> shuffleString() APIs will always produce consistent results on 32-bit an=
d
>> 64-bit, as long as your inputs are 32-bit as well (i.e., min/max are 32-=
bit
>> and string is smaller than 4G). Is that correct? The only APIs that woul=
d
>> exhibit different behavior are nextInt() and getBytes(), right?
>>  * Looking at the implementation, nextInt() performs a >> 1 operation on
>> the RNG result. I assume the motivation is to get back a non-negative
>> number. But why do we want that? The "nextInt()" name doesn't really
>> indicate that it's a positive number. I think more generally, my questio=
n
>> here may be "Why does this method exist at all? When would you use it
>> instead of getInt()?"
>>  * Another bit of clarification: For the user-defined RNG, which range i=
s
>> generate() expected to return? I assume that it must return the native
>> integer size, i.e. 32-bit on 32-bit and 64-bit on 64-bit?
>>  * I don't really get why we need RandomInterface. I think if the choice
>> is between "final + interface" and "non-final without interface", I'd
>> prefer the latter (though I'm also happy with "final without interface")=
.
>>  * I'm not entirely happy with the naming. Unfortunately, I don't have
>> great suggestions either. I think in your current hierarchy, I would mak=
e
>> the interface Random\NumberGenerator (with implementation in the
>> sub-namespace), rather than Random\NumberGenerator\RandomNumberGenerator=
.
>>
>> Regards,
>> Nikita
>>
>> I've done a tidy implementation to make this final, but I'm currently
>>> suffering from error detection by Valgrind for unknown reasons.
>>>
>>> Implementation is here: https://github.com/php/php-src/pull/7079
>>>
>>> This can be reproduced with the following code.
>>>
>>> ```sh
>>> # Success
>>> $ valgrind ./sapi/cli/php -r '$random =3D new Random();
>>> $random->nextInt();'
>>> =3D=3D95522=3D=3D Memcheck, a memory error detector
>>> =3D=3D95522=3D=3D Copyright (C) 2002-2017, and GNU GPL'd, by Julian Sew=
ard et al.
>>> =3D=3D95522=3D=3D Using Valgrind-3.14.0 and LibVEX; rerun with -h for c=
opyright
>>> info
>>> =3D=3D95522=3D=3D Command: ./sapi/cli/php -r $random\ =3D\ new\ Random(=
);\
>>> $random-\>nextInt();
>>> =3D=3D95522=3D=3D
>>> =3D=3D95522=3D=3D
>>> =3D=3D95522=3D=3D HEAP SUMMARY:
>>> =3D=3D95522=3D=3D     in use at exit: 1,286 bytes in 32 blocks
>>> =3D=3D95522=3D=3D   total heap usage: 28,445 allocs, 28,413 frees, 4,33=
3,047
>>> bytes
>>> allocated
>>> =3D=3D95522=3D=3D
>>> =3D=3D95522=3D=3D LEAK SUMMARY:
>>> =3D=3D95522=3D=3D    definitely lost: 0 bytes in 0 blocks
>>> =3D=3D95522=3D=3D    indirectly lost: 0 bytes in 0 blocks
>>> =3D=3D95522=3D=3D      possibly lost: 0 bytes in 0 blocks
>>> =3D=3D95522=3D=3D    still reachable: 1,286 bytes in 32 blocks
>>> =3D=3D95522=3D=3D         suppressed: 0 bytes in 0 blocks
>>> =3D=3D95522=3D=3D Rerun with --leak-check=3Dfull to see details of leak=
ed memory
>>> =3D=3D95522=3D=3D
>>> =3D=3D95522=3D=3D For counts of detected and suppressed errors, rerun w=
ith: -v
>>> =3D=3D95522=3D=3D ERROR SUMMARY: 0 errors from 0 contexts (suppressed: =
0 from 0)
>>>
>>> # Fail
>>> $ valgrind ./sapi/cli/php -r '$random =3D new Random(); $random->nextIn=
t()
>>> =3D=3D=3D $random->nextInt();'
>>> =3D=3D95395=3D=3D Memcheck, a memory error detector
>>> =3D=3D95395=3D=3D Copyright (C) 2002-2017, and GNU GPL'd, by Julian Sew=
ard et al.
>>> =3D=3D95395=3D=3D Using Valgrind-3.14.0 and LibVEX; rerun with -h for c=
opyright
>>> info
>>> =3D=3D95395=3D=3D Command: ./sapi/cli/php -r $random\ =3D\ new\ Random(=
);\
>>> $random-\>nextInt()\ =3D=3D=3D\ $random-\>nextInt();
>>> =3D=3D95395=3D=3D
>>> =3D=3D95395=3D=3D Conditional jump or move depends on uninitialised val=
ue(s)
>>> =3D=3D95395=3D=3D    at 0x966925: ZEND_IS_IDENTICAL_SPEC_VAR_VAR_HANDLE=
R
>>> (zend_vm_execute.h:27024)
>>> =3D=3D95395=3D=3D    by 0x99AC27: execute_ex (zend_vm_execute.h:57236)
>>> =3D=3D95395=3D=3D    by 0x99C902: zend_execute (zend_vm_execute.h:59026=
)
>>> =3D=3D95395=3D=3D    by 0x8DB6B4: zend_eval_stringl (zend_execute_API.c=
:1191)
>>> =3D=3D95395=3D=3D    by 0x8DB861: zend_eval_stringl_ex (zend_execute_AP=
I.c:1233)
>>> =3D=3D95395=3D=3D    by 0x8DB8D6: zend_eval_string_ex (zend_execute_API=
.c:1243)
>>> =3D=3D95395=3D=3D    by 0xA4DAE4: do_cli (php_cli.c:995)
>>> =3D=3D95395=3D=3D    by 0xA4E8E2: main (php_cli.c:1366)
>>> =3D=3D95395=3D=3D
>>> =3D=3D95395=3D=3D
>>> =3D=3D95395=3D=3D HEAP SUMMARY:
>>> =3D=3D95395=3D=3D     in use at exit: 1,286 bytes in 32 blocks
>>> =3D=3D95395=3D=3D   total heap usage: 28,445 allocs, 28,413 frees, 4,33=
3,070
>>> bytes
>>> allocated
>>> =3D=3D95395=3D=3D
>>> =3D=3D95395=3D=3D LEAK SUMMARY:
>>> =3D=3D95395=3D=3D    definitely lost: 0 bytes in 0 blocks
>>> =3D=3D95395=3D=3D    indirectly lost: 0 bytes in 0 blocks
>>> =3D=3D95395=3D=3D      possibly lost: 0 bytes in 0 blocks
>>> =3D=3D95395=3D=3D    still reachable: 1,286 bytes in 32 blocks
>>> =3D=3D95395=3D=3D         suppressed: 0 bytes in 0 blocks
>>> =3D=3D95395=3D=3D Rerun with --leak-check=3Dfull to see details of leak=
ed memory
>>> =3D=3D95395=3D=3D
>>> =3D=3D95395=3D=3D For counts of detected and suppressed errors, rerun w=
ith: -v
>>> =3D=3D95395=3D=3D Use --track-origins=3Dyes to see where uninitialised =
values come
>>> from
>>> =3D=3D95395=3D=3D ERROR SUMMARY: 1 errors from 1 contexts (suppressed: =
0 from 0)
>>> ```
>>>
>>> However, the detection is internal to the Zend VM and the cause has not
>>> been identified. From the code, it looks like memory management is bein=
g
>>> done properly.
>>>
>>> I have a somewhat tricky way of allocating memory to make the process
>>> common, do I need to give some hints to Valgrind?
>>>
>>> If you know, I would appreciate your advice on this issue.
>>>
>>> Regards,
>>> Go Kudo
>>>
>>

--000000000000c0eb3b05c67554a6--