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

David Holmes david.holmes at oracle.com
Fri May 1 04:52:20 UTC 2020 

On 1/05/2020 2:30 pm, Igor Ignatyev wrote:
>> On Apr 30, 2020, at 9:14 PM, David Holmes <david.holmes at oracle.com 
>> <mailto:david.holmes at oracle.com>> wrote:
>>
>> 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><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.
> true, yet almost every hotspot-X-dev alias received their porting of 
> 'use reproducible random in hotspot X tests' a couple weeks ago, which 
> all have references to the same email thread ;) . in any case, I 
> apologize I should have provided enough context in each of new RFR.
> 
>>
>> 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)?
> it's more the question for the author of particular test. I didn't plan 
> to (significantly) change behavior of the tests, so I tried not to 
> convert any non "randomness" tests to "randomness" tests.

Okay. But making changes in this area it just draws attention to the 
different way we do things, and it is n't obvious why there are 
differnet ways.

>>
>>> 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. 
> right, that's why I think it's better/safer to use something similar to 
> LocalRandom in all tests w/o thinking if it's multi-threaded or not.
> 
>> So your changes from Math.random are not going to be reproducible due 
>> to different threads being involved.
> really? could you please point me to the test/tests which used 
> Math.random and now have multiple threads using the same instance of 
> Random?

Apologies. On examining the numeric* tests in detail it seems the RNG is 
only used during the setup phase.

>>
>>>>
>>>> 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. 
> sorry, I missed 'not', it should have been 'hence it would not be really 
> reproducible'
> 
>> Secondly, LocalRandom is a wrapper for ThreadLocalRandom.current() 
>> _but_ you have no control over the seed so seems not so useful for 
>> achieving reproducability!
> I guess you haven't updated your workspace, LocalRandom is now (after 
> 8242314) using ThreadLocal<Random>:
> 
>> private final static ThreadLocal<Random> random = new ThreadLocal<>() {
>>         protected Random initialValue() {
>>             // each thread gets its own seed independendly on the 
>> order they
>>             // use LocalRandom, yet it depends on the order threads 
>> are created.
>>             // main thread uses original seed
>>             return new Random(Utils.SEED ^ 
>> (Thread.currentThread().getId() - 1));
>>         }
>>     };

Ah yes - sorry - my local repo on my laptop (only used for code 
browsing) was missing that change. Okay that makes a big difference :)

However I will note that using the thread ID as part of the seed may not 
lead to reproducible results if the thread creation itself is not 
deterministic/reproducible.

Thanks,
David

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

More information about the hotspot-dev mailing list