RFR 8222671 : thread_large/thread_large.java times out on MacOSX
Daniel D. Daugherty
daniel.daugherty at oracle.com
Tue Aug 18 13:31:58 UTC 2020
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
>>
More information about the hotspot-runtime-dev
mailing list