On Sat, Jun 26, 2021 at 2:40 AM Go Kudo <zeriyo...@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 >
