Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:115211
MIME-Version: 1.0
References: <CABTDVhD9SKNVFrt8NMj4BqdHmsMpSt0DH=SMmb-9C905rgc7PA@mail.gmail.com>
In-Reply-To: <CABTDVhD9SKNVFrt8NMj4BqdHmsMpSt0DH=SMmb-9C905rgc7PA@mail.gmail.com>
Date: Tue, 29 Jun 2021 16:00:57 +0200
Message-ID: <CAF+90c9LNqgF9JJgXY_cNbX4hY9hU2QaCNViNpy25YFLeKgzLQ@mail.gmail.com>
To: Go Kudo <zeriyoshi@gmail.com>
Cc: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000f6cd2305c5e809b8"
Subject: Re: [PHP-DEV] [RFC] Add Random Extension (before: Add Random class)
From: nikita.ppv@gmail.com (Nikita Popov)

--000000000000f6cd2305c5e809b8
Content-Type: text/plain; charset="UTF-8"

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() 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-bit
and string is smaller than 4G). Is that correct? The only APIs that would
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 question
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 is
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 make
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 = new Random(); $random->nextInt();'
> ==95522== Memcheck, a memory error detector
> ==95522== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
> ==95522== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright
> info
> ==95522== Command: ./sapi/cli/php -r $random\ =\ new\ Random();\
> $random-\>nextInt();
> ==95522==
> ==95522==
> ==95522== HEAP SUMMARY:
> ==95522==     in use at exit: 1,286 bytes in 32 blocks
> ==95522==   total heap usage: 28,445 allocs, 28,413 frees, 4,333,047 bytes
> allocated
> ==95522==
> ==95522== LEAK SUMMARY:
> ==95522==    definitely lost: 0 bytes in 0 blocks
> ==95522==    indirectly lost: 0 bytes in 0 blocks
> ==95522==      possibly lost: 0 bytes in 0 blocks
> ==95522==    still reachable: 1,286 bytes in 32 blocks
> ==95522==         suppressed: 0 bytes in 0 blocks
> ==95522== Rerun with --leak-check=full to see details of leaked memory
> ==95522==
> ==95522== For counts of detected and suppressed errors, rerun with: -v
> ==95522== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
>
> # Fail
> $ valgrind ./sapi/cli/php -r '$random = new Random(); $random->nextInt()
> === $random->nextInt();'
> ==95395== Memcheck, a memory error detector
> ==95395== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
> ==95395== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright
> info
> ==95395== Command: ./sapi/cli/php -r $random\ =\ new\ Random();\
> $random-\>nextInt()\ ===\ $random-\>nextInt();
> ==95395==
> ==95395== Conditional jump or move depends on uninitialised value(s)
> ==95395==    at 0x966925: ZEND_IS_IDENTICAL_SPEC_VAR_VAR_HANDLER
> (zend_vm_execute.h:27024)
> ==95395==    by 0x99AC27: execute_ex (zend_vm_execute.h:57236)
> ==95395==    by 0x99C902: zend_execute (zend_vm_execute.h:59026)
> ==95395==    by 0x8DB6B4: zend_eval_stringl (zend_execute_API.c:1191)
> ==95395==    by 0x8DB861: zend_eval_stringl_ex (zend_execute_API.c:1233)
> ==95395==    by 0x8DB8D6: zend_eval_string_ex (zend_execute_API.c:1243)
> ==95395==    by 0xA4DAE4: do_cli (php_cli.c:995)
> ==95395==    by 0xA4E8E2: main (php_cli.c:1366)
> ==95395==
> ==95395==
> ==95395== HEAP SUMMARY:
> ==95395==     in use at exit: 1,286 bytes in 32 blocks
> ==95395==   total heap usage: 28,445 allocs, 28,413 frees, 4,333,070 bytes
> allocated
> ==95395==
> ==95395== LEAK SUMMARY:
> ==95395==    definitely lost: 0 bytes in 0 blocks
> ==95395==    indirectly lost: 0 bytes in 0 blocks
> ==95395==      possibly lost: 0 bytes in 0 blocks
> ==95395==    still reachable: 1,286 bytes in 32 blocks
> ==95395==         suppressed: 0 bytes in 0 blocks
> ==95395== Rerun with --leak-check=full to see details of leaked memory
> ==95395==
> ==95395== For counts of detected and suppressed errors, rerun with: -v
> ==95395== Use --track-origins=yes to see where uninitialised values come
> from
> ==95395== 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 being
> 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
>

--000000000000f6cd2305c5e809b8--