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 >>> >>
