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

[David Holmes]

Hi Gerard,

On 6/08/2020 12:49 am, gerard ziemski wrote:
> hi all,
> 
> Please review this new test that replaces a closed (and flawed) one, 
> which was causing time out issues on Mac OS X.
> 
> The new test takes the main characteristic of the old test, which is 
> creating a large number of threads, but in a robust and deterministic 
> way - we create as many threads as the process allows and expect the VM 
> to throw OOM exception to let us know when we reach the limit - then we 
> perform a simple work unit for each thread (setting its name and 
> verifying it back), then simply exit.

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.

> To make it more robust we use the Thread.wait(TIMEOUT) instead of the 
> Thread.wait() as in the original.

It doesn't actually make it more robust as we need to ensure the TIMEOUT 
is compatible with the jtreg timeout that is applied, which is itself 
subject to a timeout adjustment factor to allow for slower machines. If 
you just use await() you don't need to care about timeouts or time value 
parsing etc and it simplifies the test. Timeouts are best handled at the 
jtreg level.

Overall copying the test from the existing nsk stress tests makes it 
more complicated than necessary and also perpetuates some poor design 
choices made in those tests (or at least choices that may have made 
sense for a single known testing environment but which do not 
generalise). In particular the goal of this test should be to check that 
hitting the OS limit does not cause a crash or other strange behaviour; 
it isn't about ensuring a minimum number of threads can be created by 
the JVM. By setting an artificial minimum you just set the test up to 
fail in a new test environment (such as a for a small, memory 
constrained device).

I've said before that calling yield() is unnecessary and may not even do 
anything useful, but if you insist of using it it is a static method so 
should be invoked as Thread.yield()  (all static Thread methods apply to 
the current thread).

> To keep things nice and simple we use CountDownLatch objects for 
> synchronization between the main and worker threads (thank you David).

  173     for (int i = limit; i < NUMBER_OF_THREADS; i++) {
  174       doneSignal.countDown();
  175     }

The main thread shouldn't call countDown on the doneSignal. It could 
also simply join() them rather than using the doneSignal.

Thanks,
David
-----

> bug link at https://bugs.openjdk.java.net/browse/JDK-8222671
> open webrev at http://cr.openjdk.java.net/~gziemski/8222671_rev1
> testing: passes Mach5 hs-tier1,2,3,4,5
> 
> 
> cheers