> * 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?
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 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()?" 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 that 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.) > * 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 the 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 to 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年6月29日(火) 23:01 Nikita Popov <nikita....@gmail.com>: > 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 >> >
