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

[Chris Plummer]

On 9/30/19 1:30 PM, serguei.spitsyn at oracle.com wrote:
> 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?
Maybe just first and last indices would be good. I'm not sure something 
in the middle helps any.

Chris
>
> 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
>>>>
>>>>
>>>
>>
>>
>