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

[Daniel D. Daugherty]

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