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

[David Holmes]

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
>