RFR 8222671 : thread_large/thread_large.java times out on MacOSX
David Holmes
david.holmes at oracle.com
Tue Aug 18 05:15:02 UTC 2020
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
>
More information about the hotspot-runtime-dev
mailing list