RFR(S) : 8243429 : use reproducible random in :vmTestbase_nsk_stress

[David Holmes]

On 1/05/2020 1:27 pm, Igor Ignatyev wrote:
> Hi David,
> 
>> On Apr 30, 2020, at 7:34 PM, David Holmes <david.holmes at oracle.com 
>> <mailto:david.holmes at oracle.com>> wrote:
>>
>> Hi Igor,
>>
>> On 1/05/2020 3:15 am, Igor Ignatyev wrote:
>>> http://cr.openjdk.java.net/~iignatyev/8243429/webrev.00
>>>> 82 lines changed: 41 ins; 4 del; 37 mod;
>>
>> The patch files in your webrevs are not generating properly due to the 
>> use of mq. can you fix that?
> unfortunately, I've already squashed all the patches together, so I was 
> able to upload only the final version -- 
> http://cr.openjdk.java.net/~iignatyev/8243429/webrev.00/8243429.patch
>>
>>> Hi all,
>>> could you please review this small patch?
>>> from JBS:
>>>> this subtask is to use j.t.l.Utils.getRandomInstance() as a random 
>>>> number generator, where applicable, in :vmTestbase_nsk_stress test 
>>>> group and marking the tests which make use of "randomness" with a 
>>>> proper k/w.
>>
>> I'm a bit confused about the strategy here:
>>
>> vmTestbase/nsk/stress/except/except012.java
>>
>> -            Random random = new Random(0);
>> +            Random random = new Random(42);
>>
>> but:
>>
>> vmTestbase/nsk/stress/jni/jnistress002.java
>>
>> -    static Random myRandom = new Random();
>> +    Random myRandom = new Random(Utils.getRandomInstance().nextLong());
>>
>> why are these random instances seeded completely differently?
> 
> because they were different before my patch, except012 used a fixed seed 
> (so it doesn't qualify to be marked w/ randomness k/w), and jnistress002 
> used a "random" seed; after the patch except012 still uses a fixed (but 
> a "well-known" and easier to grep) seed value, and jnistress002 uses 
> random, yet reproducible/configurable seed. I've explained that in my 
> previous RFRs for hotspot tests outside of vmTestbase, e.g. 
> https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2020-April/037828.html 

Not everyone on hotspot-dev is also on hotspot-compiler-dev.

I'm trying to understand why we still have different usage patterns. 
IIUC there's no reason that except012 couldn't be seeded the same way as 
jnistress002 - right? Then it would qualify for "randomness" keyword. 
What is it about any given test that determines whether we seed the RNG 
differently on each test run versus fixing it at 42 (for example)?

> 
> also, in jnistress002, we can't use Utils.getRandomInstance() as there 
> would multiple threads (JNIter002) using the same RNG. we could change 
> it to use LocalRandom, I must admit I was doing different parts of these 
> patches on different days, and given their tediousness I might have 
> became less consistent in my choices b/w using LocalRandom and using a 
> "threadlocal" instance of Random. I, personally, think we need to switch 
> to LocalRandom in all such cases, or even better to replace 
> Utils.getRandomInstance() w/ something akin to LocalRandom, but such 
> change is somewhat orthogonal to the series of 'use reproducible random' 
> patches, so I'd prefer to do it separately.

Sure but it remains confusing to see such a variety of cases. 
Utils.getRandomInstance() is a shared RNG so you never want to use that 
directly in different threads if you truly want reproducability - the 
most you can use it for is the source of seeds for other RNGs - but even 
that is non-deterministic is different threads are involved. So your 
changes from Math.random are not going to be reproducible due to 
different threads being involved.

>>
>> And looking at:
>>
>> vmTestbase/nsk/stress/jni/gclocker/gcl001.java
>>
>> you changed it to use the LocalRandom utility rather than creating its 
>> own random instance. Why was that applicable here but not in other tests?
>>
>> The changes from Math.random to a local RNG from getRandomInstance() 
>> seem okay- but again why that versus LocalRandom? Do we even need 
>> LocalRandom?
> 
> (although I guess you've already figured that out from other patches, 
> I'll answer here for completeness)
> 
> vmTestbase/nsk/stress/jni/gclocker/gcl001.java used the same instance of 
> RNG in multiple threads, hence it would be really reproducible if we 
> just switched to getRandomInstance(), LocalRandom is needed exactly for 
> that -- provide reproducible random in multi-thread use cases.

I'm not sure if you wrote that exactly as intended. 
Utils.getRandomInstance() does not produce a RNG that gives reproducible 
results across threads, so we should, it seems, never use it directly in 
any multi-threaded stress test. Secondly, LocalRandom is a wrapper for 
ThreadLocalRandom.current() _but_ you have no control over the seed so 
seems not so useful for achieving reproducability!

David
-----

> 
> -- Igor
> 
>>
>> Thanks,
>> David
>>
>>> testing: : vmTestbase_nsk_stress test group
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8243429
>>> webrevs:
>>> - code changes: 
>>> http://cr.openjdk.java.net/~iignatyev//8243429/webrev.00.code
>>>> 58 lines changed: 41 ins; 4 del; 13 mod;
>>> - adding k/w: http://cr.openjdk.java.net/~iignatyev//8243429/webrev.00.kw
>>>> 11 lines changed: 0 ins; 0 del; 11 mod;
>>> - full: http://cr.openjdk.java.net/~iignatyev//8243429/webrev.00
>>>> 82 lines changed: 41 ins; 4 del; 37 mod;
>>> Thanks,
>>> -- Igor
>