On Tue, Jul 6, 2021 at 4:54 PM Go Kudo <zeriyo...@gmail.com> wrote: > > 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). >
Yes, I think that would be best. As the value returned by nextInt() is already available through generate(), and use cases for nextInt() probably aren't common, I don't think we need the separate nextInt() method. We can always add it later if it turns out to be a common requirement (but not the other way around...) Regards, Nikita > 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 >>>>> >>>>
