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

[serguei.spitsyn at oracle.com]

Alex, Chris and David,

The updated webrev is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8231595-jvmti-susp-tlist.3/

This version changes are:
   - the "first" and "last" are passed to the test to set the suspenderIndex
   - fixed typo at line 120
   - the ThreadToSuspend.run() loop is simplified
   - fixed typo in check_jvmti_status in the loop of resumeTestedThreads()

Probably, just one sanity check would be enough.

Thanks,
Serguei


On 10/1/19 2:20 PM, serguei.spitsyn at oracle.com wrote:
> Hi Chris,
>
> On 10/1/19 12:46 PM, Chris Plummer wrote:
>> Hi Serguei,
>>
>> If someone changes THREADS_COUNT, then SuspenderIndex would no longer 
>> do what we want. I suggest passing in something like "first" and "last".
>
> Okay, I'll update it this way.
>
>>  120 *  - main thread registers tested threads withing the native 
>> agent library
>>
>> Should be "within".
>
> Good catch, will fix it.
>
>> I think you should add a comment above all the "n" and "i" logic 
>> explaining what it is for, although TBH, I don't see how this is 
>> making the method "hot" and therefore trigger compilation. The loop 
>> alone should be enough for that. In fact I would think the more 
>> iterations through the loop, the sooner it would be compiled, and 
>> this extra logic just slows down the iteration rate.
>
> Agreed.
> I'll try to simplify it.
>
> Thanks,
> Serguei
>
>
>> thanks,
>>
>> Chris
>>
>> On 9/30/19 10:45 PM, serguei.spitsyn at oracle.com wrote:
>>> Hi Chris, David and Alex,
>>>
>>> The updated webrev is:
>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8231595-jvmti-susp-tlist.2/ 
>>>
>>>
>>>
>>> It includes the changes:
>>>   - added a general comment explaining the test logic suggested by 
>>> David
>>>   - setAllThreadsReady() is static now, the obsolete comment before 
>>> it is deleted
>>>   - now the test is run twice: with the suspender thread first and 
>>> last in the list
>>>   - removed the local variable "success" in the run() method
>>>   - several native agent methods return "void" now
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>> On 9/30/19 13:49, Chris Plummer wrote:
>>>> 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
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
>