(13) RFR (S): 8224555: vmTestbase/nsk/jvmti/scenarios/contention/TC02/tc02t001/TestDescription.java failed

[Alex Menkov]

Looks good to me too.

--alex

On 06/21/2019 11:38, serguei.spitsyn at oracle.com wrote:
> Hi Dan,
> 
> 
> On 6/21/19 06:44, Daniel D. Daugherty wrote:
>> On 6/21/19 1:24 AM, serguei.spitsyn at oracle.com wrote:
>>> Please, review the test bug fix for:
>>> https://bugs.openjdk.java.net/browse/JDK-8224555
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8224555-mon-events2-test.1/
>>
>> test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/contention/TC02/tc02t001.java
>>     L99:             for (int j = 0; j < 1000; j++) {
>>         Why a literal '1000' this time? You could use "timeout / 20"
>>         based on:
>>
>>         L120:         timeout = argHandler.getWaitTime() * 60 * 1000;
>>
>>         That seems to be what all the other timeout logic is based on.
> 
> Good suggestion, thanks.
> Taken.
> 
> 
>>
>> test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/contention/TC02/tc02t001/tc02t001.cpp
>>     No comments.
>>
>> Thumbs up. I don't need a new webrev if you decide to make the
>> minor change above.
> 
> Thanks you for review, Dan!
> Serguei
> 
> 
>>
>> Dan
>>
>>>
>>>
>>> Summary:
>>>   The test sleeps for 1 sec in hope to get a contention on the 
>>> monitor tc02t001Thread.M.
>>>   It seems, this is not enough when the JFR is enabled.
>>>   The fix uses a better approach to ensure events are really happen.
>>>   This approach is similar to the on in the fix of 8223736 (just 
>>> reviewed).
>>>
>>>   Also, the class line number sensitive tc02t001Thread is moved to 
>>> the beginning of
>>>   the file to make it independent from the rest of the file.
>>>
>>>
>>> Testing:
>>>   A mach5 submission is in progress.
>>>
>>>
>>> Thanks,
>>> Serguei
>>
>