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

David Holmes david.holmes at oracle.com
Fri May 1 05:52:21 UTC 2020 

On 1/05/2020 3:20 pm, Igor Ignatyev wrote:
>> On Apr 30, 2020, at 9:52 PM, David Holmes <david.holmes at oracle.com 
>> <mailto:david.holmes at oracle.com>> wrote:
>>
>> 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><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><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.
> I guess that some people decided to use fixed seed so their tests are 
> more-or-less stable to themselves and results from different runs can be 
> compared; yet others decided that they would like their tests to cover 
> different paths in different runs. and I really don't know which 
> solution is better, but I believe that solution which allows you to get 
> the best of both is the best. that's to say, I'll definitely file a task 
> to look into all places we use fixed seed in hotspot tests and see if it 
> makes sense to switch to a "random" seed value, and will work on it as 
> part of switching to CI-run/build-based seed or, as you called it in 
> another thread, "one blessed seed". thank you for all these discussions!

Sounds good. Sorry for drawing this out so much.

>>
>>>>
>>>>> 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.
> true, yet in most cases it's still better than prev. solution, and it's 
> never worse. and in all of my runs w/ instrumented builds, thread 
> creation and hence the seeds were deterministic.

Definitely better than before. :)

I would have to wonder whether dynamic compiler threads would be the 
mostly likely source of disruption.

> so,.. can I count you as a reviewer?

Of course :)

Thanks,
David

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