RFR 8222671 : thread_large/thread_large.java times out on MacOSX

[Igor Ignatyev]

Dan/Gerard,

we use '-ea -esa'  in some of our configurations, yet not in all configurations and not all people use these configurations when they run tests. also some people run tests directly thru jtreg (or even w/o jtreg), hence adding it into make won't really help. therefore the tests shouldn't depend on -ea/-esa.

generally speaking, `assert` makes sense only for product code where you might want some checks to be disabled in production runs, for tests you really want all checks to be always done.

-- Igor
 

> On Aug 18, 2020, at 6:31 AM, Daniel D. Daugherty <daniel.daugherty at oracle.com> wrote:
> 
> This caught my eye:
> 
> > We never run tests with Java asserts enabled so this is "dead" code.
> 
> I'm pretty certain that I've seen test tasks in Mach5 with enable assertion
> options passed. I don't have an example at my fingertips though...
> 
> Dan
> 
> 
> On 8/18/20 1:15 AM, David Holmes wrote:
>> Hi Gerard,
>> 
>> On 18/08/2020 1:28 am, gerard ziemski wrote:
>>> Thank you David and Igor for the feedback!
>>> 
>>> I rewrote the test again, this time basing it off test/hotspot/jtreg/runtime/Thread/* tests.
>>> 
>>> For macOS and Linux we create threads until we hit a process limit thread count. Windows, however, allows unlimited (?) number of threads, though the process gradually slows down as the count grows, so I had to add time limit and I set it to 5 sec. I chose 5 sec because the value is well below jtreg time out (i.e. 120s), but still quite above how long my laptop needs to hit the process thread count limit (i.e. my Mac laptop needs about 1 sec to hit that limit).
>>> 
>>> Otherwise there are no other hardwired values or assumptions (I hope). What do you think of the test now?
>> 
>> Not unreasonable :) It could still have tried for an upper limit of 10000 instead of using the time-limit, but it makes no real difference.
>> 
>> A few nits/suggestions:
>> 
>> Please add an @bug line to the test comment.
>> 
>>  49         if ((this.index % 250) == 0) {
>> 
>> Outside of the constructor it is poor style to use this.x to refer to fields methods (unless needed due to local variable shadowing). This applies throughout the test.
>> 
>>  54         doWork();
>> 
>> Not sure why you factored things out the way you did - everything could go in run().
>> 
>>  61       setName(""+this.index);
>> 
>> This could go in constructor, but I presume it adds something to the workload? Is the use of ""+index deliberate in that regard (generate string concatenation work), rather than using String.valueOf(index)?
>> 
>>  65       assert(this.index == Integer.parseInt(getName()));
>> 
>> We never run tests with Java asserts enabled so this is "dead" code.
>> 
>>  87           // Windows path
>> 
>> Windows path or system with very large ulimit?
>> 
>>   92     } catch (OutOfMemoryError e) {
>> 
>> I suggest checking this is actually the "unable to create native thread" OOME rather than a general Java OOME (if run with a small heap for example you might get OOME growing the ArrayList).
>> 
>>  110     String cmd = System.getProperty("sun.java.command");
>>  111     if (cmd != null && !cmd.startsWith("com.sun.javatest.regtest.agent.MainWrapper")) {
>>  112       // Exit with success in a non-JavaTest environment:
>>  113       System.exit(0);
>>  114     }
>> 
>> I never noticed when this was used in the little set of runtime/Thread tests! I don't know what the rationale was but it is completely unnecessary as a normal return from main would be the equivalent of exit(0) - so the tests work fine without System.exit when run standalone. Please don't emulate this part of the existing tests. I'll file a RFE to remove it from the other tests.
>> 
>> I won't be around for any further follow up for a week, so feel free to proceed once Igor is okay with things.
>> 
>> Thanks,
>> David
>> -----
>> 
>>> bug link at https://bugs.openjdk.java.net/browse/JDK-8222671
>>> open webrev at http://cr.openjdk.java.net/~gziemski/8222671_rev2
>>> testing: Mach5 hs-tier1,2,3,4,5 in progress...
>>> 
>>> 
>>> 
>>> 
>>> On 8/7/20 2:41 PM, Igor Ignatyev wrote:
>>>> 
>>>> 
>>>>> On Aug 6, 2020, at 12:33 AM, David Holmes <david.holmes at oracle.com <mailto:david.holmes at oracle.com>> wrote:
>>>>> 
>>>>> Basing the new test around the existing thread stress tests was not unreasonable but I don't think we're actively adding to those tests. I would check with Igor whether it is better to just add this under runtime/thread rather than vmTestBase.
>>>> 
>>>> Hi Gerard,
>>>> 
>>>> David is right, we are trying to get rid of vmTestbase directory and move all tests into corresponding component directories cleaning them up at the same time. so runtime/thread directory sound like a much better location for a new test.
>>>> 
>>>> I don't think you need to cary all this baggage of 'VM testbase keywords', 'converted from ...' and the rest, I'd suggest you just use the text from 'DESCRIPTION' as @summary. I'd also remove System.exit call and would just make thread009::run to return bool. it's also unclear to me if someone is ever going to change THREADS_EXPECTED , TIMEOUT, and DEBUG_MODE values, I'd just hardcode them in the test and get rid of parsing. it seems that debug output won't clutter output, so you might want to always print them and remove DEBUG_MODE field.
>>>> 
>>>> using `assert` in test code isn't a good idea as it will work only if someone runs test w/ `-ea` which not all people do. it's better to replace w/ `if (p) throw new Error(...)`  or use jdk.test.lib.Asserts class.
>>>> 
>>>> Cheers,
>>>> -- Igor
>>> 
>