RFR (M): 8231595 [TEST] develop a test case for SuspendThreadList including current thread

[serguei.spitsyn at oracle.com]

On 9/30/19 13:25, Chris Plummer wrote:
> On 9/30/19 1:21 PM, serguei.spitsyn at oracle.com wrote:
>> Hi Chris,
>>
>> Thank you for reviewing this!
>>
>>
>> On 9/28/19 12:33, Chris Plummer wrote:
>>> Hi Serguei,
>>>
>>> Overall looks good. A few questions:
>>>
>>> I don't understand the need for all the 'i' and 'n' theatrics in the 
>>> shouldFinish loop. Can you explain and also add a comment?
>>
>> I used this part from one of the old SuspendThreadList nsk tests in 
>> the vmTestbase.
>> As I understand it, the point was to make sure the JVMTI 
>> SuspendThreadList works well
>> wen the top frames executed on tested threads have been compiled.
>> These code is needed to make the run() method hot.
>> I can add a comment if you think it is not clear.
>>
>>
>>>
>>> Is this comment right?
>>>
>>>  193     // set thread is not ready again
>>>  194     public void setAllThreadsReady() {
>>>  195         allThreadsReady = true;
>>>  196     }
>>
>> Nice catch.
>> The comment is not needed anymore.
>> Is a leftover from previous test version.
>>
>>>
>>> Also, shouldn't "setAllThreadsReady()" be static?
>>
>> Right. It has to be static. Will fix it.
>>
>>
>>> Do you think it would be useful to also run the test with the last 
>>> thread in the list being the suspender thread?
>>
>> I'm not sure it is worth to do it.
>> It'd add more complexity into the test.
>> We could try to make the suspender thread to be random though.
> I don't think random is good. Makes it hard to reproduce an issue if 
> it turns up. I was thinking just rerun the test for each possible 
> suspender.

Good idea to rerun the test and pass the suspender thread index in the 
arguments.
Do you think, two runs would be good enough or we also need an index 
somewhere in the middle?

Thanks,
Serguei

>
> thanks,
>
> Chris
>>
>> Thanks,
>> Serguei
>>
>>>
>>> thanks,
>>>
>>> Chris
>>>
>>> On 9/27/19 6:25 PM, serguei.spitsyn at oracle.com wrote:
>>>> Please, review fix for test enhancement:
>>>>   https://bugs.openjdk.java.net/browse/JDK-8231595
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8231595-jvmti-susp-tlist.1/ 
>>>>
>>>>
>>>> Summary:
>>>>   New test is a coverage for the JVMTI bug:
>>>>     https://bugs.openjdk.java.net/browse/JDK-8217762
>>>>       SuspendThreadList won't work correctly if the current thread 
>>>> is not last in the list
>>>>
>>>>   It provides a prove the bug JDK-8217762 does not exist
>>>>   as the test is passed with the current implementation.
>>>>
>>>> Thanks,
>>>> Serguei
>>>
>>>
>>
>
>