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

Igor Ignatyev igor.ignatyev at oracle.com
Fri May 1 05:20:52 UTC 2020 


> On Apr 30, 2020, at 9:52 PM, David Holmes <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 <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.
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!

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

so,.. can I count you as a reviewer?
-- 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