> Could you share some example of where you use it? It looks like mt_rand() could be replaced by mt_rand(0, getrandmax()), but that is not the case, mt_rand() with a specified range is an implementation that generates random numbers until the desired value is obtained, which may unintentionally advance the random state. This can be undesirable for pseudo-random numbers with periodicity.
Also, shouldn't compatibility with mt_rand() be maintained? Currently, NumberGenerator\MT19937 is fully compatible with mt_rand(), do We need to drop it? > $random->getNumberGenerator()->generate() to access the raw RNG stream. Indeed. I think nextInt() can be removed from Random, since it currently returns exactly the same value. (However, I think NumberGenerator::generate() should be kept). Regards, Go Kudo 2021年7月6日(火) 23:35 Nikita Popov <nikita....@gmail.com>: > On Fri, Jul 2, 2021 at 3:58 PM Go Kudo <zeriyo...@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-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.) >> > > 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 >> 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 >>>> >>>
